Re: [PATCH v2 4/6] t5701: add setup test to remove side-effect dependency

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Jan 20, 2025, 17:32 UTC
Message-ID: <CAPSxiM9qRQ2HuTJDmhq_xeCRmn+yUvjXokwEwJE0S4av9Y-TKg@mail.gmail.com>
In-Reply-To: <xmqq4j1xkzir.fsf@gitster.g>

On Sat, Jan 18, 2025 at 1:02 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> 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"

Show 13 quoted lines

>
> 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.