| Message ID | 20250117104639.65608-5-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce os-version Capability with Configurable Options | expand |
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > -test_expect_success 'test capability advertisement' ' > +test_expect_success 'setup to generate files with expected content' ' > + printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_osversion && Is this required to be "printf" and not "echo", if so why? "git version" could contain any character if the builder gives a custom version string by saving it in the "version" file (we use the mechanism when we create a distribution tarball, for example). What happens if it contains say "%s" or something? If you _really_ need to use printf, you'd want to do so more like: printf "agent=git/%s" "$(git version | cut ...)" Is it required that agent_and_osversion lack the terminating LF? The use of printf without terminating "\n" at the end of the format string hints the readers that it is the case. If you did not intend that, perhaps doing printf "agent=git/%s\n" "$(git version | cut ...)" would avoid misleading them.
On Sat, Jan 18, 2025 at 1:02 AM Junio C Hamano <gitster@pobox.com> wrote: > > Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > > > -test_expect_success 'test capability advertisement' ' > > +test_expect_success 'setup to generate files with expected content' ' > > + printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_osversion && > > Is this required to be "printf" and not "echo", if so why? > > "git version" could contain any character if the builder gives a > custom version string by saving it in the "version" file (we use the > mechanism when we create a distribution tarball, for example). What > happens if it contains say "%s" or something? There is not any requirement to use "printf" here, I did not think about this case before, I will change it to "echo" > > If you _really_ need to use printf, you'd want to do so more like: > > printf "agent=git/%s" "$(git version | cut ...)" > > Is it required that agent_and_osversion lack the terminating LF? > The use of printf without terminating "\n" at the end of the format > string hints the readers that it is the case. If you did not intend > that, perhaps doing > > printf "agent=git/%s\n" "$(git version | cut ...)" > > would avoid misleading them. Yeah, that is true, I could not notice this as the next commit of the patch series was able to fix it. I will change it to "echo", with this, it will be better. Thank you.
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > Yeah, that is true, I could not notice this as the next commit of the > patch series > was able to fix it. I will change it to "echo", with this, it will be better. If we want to prepare ourselves against any arbitrary garbage the builder may throw at us, using printf with _fixed_ format and feed the potentially arbitrary garbage as its parameter to be interpolated is the safest approach, so writing it as printf "agent=git/%s\n" "$(git version | cut ...)" would signal the readers that whoever wrote it knew what they were doing and was being extra careful. THanks.
On Tue, Jan 21, 2025 at 1:22 AM Junio C Hamano <gitster@pobox.com> wrote: > > Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > > > Yeah, that is true, I could not notice this as the next commit of the > > patch series > > was able to fix it. I will change it to "echo", with this, it will be better. > > If we want to prepare ourselves against any arbitrary garbage the > builder may throw at us, using printf with _fixed_ format and feed > the potentially arbitrary garbage as its parameter to be > interpolated is the safest approach, so writing it as > > printf "agent=git/%s\n" "$(git version | cut ...)" > > would signal the readers that whoever wrote it knew what they were > doing and was being extra careful. > > THanks. Yeah, I will add this in the next iteration. Thanks.
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index de904c1655..0c0a5b2aec 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -7,14 +7,17 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh -test_expect_success 'test capability advertisement' ' +test_expect_success 'setup to generate files with expected content' ' + printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_osversion && + test_oid_cache <<-EOF && wrong_algo sha1:sha256 wrong_algo sha256:sha1 EOF + cat >expect.base <<-EOF && version 2 - agent=git/$(git version | cut -d" " -f3) + $(cat agent_and_osversion) ls-refs=unborn fetch=shallow wait-for-done server-option @@ -23,6 +26,9 @@ test_expect_success 'test capability advertisement' ' cat >expect.trailer <<-EOF && 0000 EOF +' + +test_expect_success 'test capability advertisement' ' cat expect.base expect.trailer >expect && GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
Currently, the "test capability advertisement" test creates some files with expected content which are used by other tests below it. To remove that side-effect from this test, let's split up part of it into a "setup"-type test which creates the files with expected content which gets reused by multiple tests. This will be useful in a following commit. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com> --- t/t5701-git-serve.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)