Re: [PATCH 3/4] connect: advertise OS version

>
> On Mon, Jan 6, 2025 at 5:37 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> > As some issues that can happen with a Git client can be operating system
> > specific, it can be useful for a server to know which OS a client is
> > using. In the same way it can be useful for a client to know which OS
> > a server is using.
> >
> > Let's introduce a new protocol (`os-version`) allowing Git clients and
> > servers to exchange operating system information. The protocol is
> > controlled by the new `transfer.advertiseOSVersion` config option.
> >
> > Add the `transfer.advertiseOSVersion` config option to address
> > privacy concerns issue. It defaults to `true` and can be changed to
> > `false`. When enabled, this option makes clients and servers send each
> > other the OS name (e.g., "Linux" or "Windows"). The information is
> > retrieved using the 'sysname' field of the `uname(2)` system call.
> >
> > However, there are differences between `uname(1)` (command-line utility)
> > and `uname(2)` (system call) outputs on Windows. These discrepancies
> > complicate testing on Windows platforms. For example:
> >   - `uname(1)` output: MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64\
> >   .2024-02-14.20:17.UTC.x86_64
> >   - `uname(2)` output: Windows.10.0.20348
> >
> > Until a good way to test the feature on Windows is found, the
> > transfer.advertiseOSVersion is set to false on Windows during testing.
>
> This is because the uname(2) you mention above is not actually
> system-supplied but is instead faked up Git itself for the Git for
> Windows port. See git/compat/mingw.c:uname().
>
> The typical way to work around this sort of issue is to ensure that
> you check Git against Git itself instead of checking Git against
> "system". To do so, you would implement a new "test-util" command, say
> `test-util uname`, in git/t/helpers/test-uname.c which internally
> calls the same uname() function that other parts of Git call. Doing so
> ensures consistency of output.
>
> Whether or not it makes sense to go through that extra work for this
> particular case is a different question.

>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> > diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
> > @@ -123,9 +123,19 @@ test_expect_success 'git receive-pack --advertise-refs: v1' '
> >  test_expect_success 'git upload-pack --advertise-refs: v2' '
> > +       printf "agent=FAKE" >agent_and_os_name &&
> > +       if test_have_prereq WINDOWS
> > +       then
> > +               # We do not use test_config here so that any tests below can reuse
> > +               # the "expect" file from this test
> > +               git config transfer.advertiseOSVersion false
>
> Should this have a comment explaining why you're disabling
> transfer.advertiseOSVersion, in particular that you found uname() on
> Windows unreliable, thus need to disable the check for this case?
>
> The comment you did compose exposes a fragility of the tests: in
> particular that subsequent tests rely upon a side-effect of this test.
> The fact that you had to include a special comment explaining the
> problem argues for a cleaner solution, such as splitting out part of
> this code into a separate test which comes before this one:
> specifically, a "setup"-type test which creates the "expect" file
> which gets reused by multiple tests.

>
> > +       else
> > +               printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_os_name
> > +       fi &&
> > diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> > @@ -8,13 +8,23 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> >  test_expect_success 'test capability advertisement' '
> > +       printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_os_name &&
> > +       if test_have_prereq WINDOWS
> > +       then
> > +               # We do not use test_config here so that tests below will be able to reuse
> > +               # the expect.base and expect.trailer files
> > +               git config transfer.advertiseOSVersion false
>
> Ditto.
>
> > +       else
> > +               printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_os_name
> > +       fi &&