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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2025, 23:17 UTC
Message-ID: <CAPig+cQJM1TOad=buG0h2-3OnxEH6SUBz23y+wYq9GeUzhMwaA@mail.gmail.com>
In-Reply-To: <20250106103713.1452035-4-usmanakinyemi202@gmail.com>

On Mon, Jan 6, 2025 at 5:37 AM Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:

Show 24 quoted lines

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

Show 12 quoted lines

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

Show 12 quoted lines

> +       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 &&