Re: [PATCH v2 5/6] connect: advertise OS version

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2025, 22:22 UTC
Message-ID: <xmqq4j1xjd2m.fsf@gitster.g>
In-Reply-To: <20250117104639.65608-6-usmanakinyemi202@gmail.com>

Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

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

Hmph. The other end may be running different version of Git, and the version difference of _our_ software is probably more relevant. For that matter, they may even be running something entirely different from our software, like Gerrit. So I am not sure I am convinced that os-version thing is a good thing to have with that paragraph.

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

The last sentence is redundant and can safely removed. The next paragraph describes it better than "is controlled by".

Show 5 quoted lines

> Add the `transfer.advertiseOSVersion` config option to address
> privacy concerns. 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.

Add "or its equivalent" at the end.

macOS may have one, but it probably is not quite correct to say that Windows have uname system call (otherwise we wouldn't be emulating it on top of GetVersion ourselves).

Show 13 quoted lines

> 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
>
> On Windows, uname(2) is not actually system-supplied but is instead
> already faked up by Git itself. We could have overcome the test issue
> on Windows by implementing a new `uname` subcommand in `test-tool`
> using uname(2), but except uname(2), which would be tested against
> itself, there would be nothing platform specific, so it's just simpler
> to disable the tests on Windows.

OK.

Show 6 quoted lines

> +transfer.advertiseOSVersion::
> +	When `true`, the `os-version` capability is advertised by clients and
> +	servers. It makes clients and servers send to each other a string
> +	representing the operating system name, like "Linux" or "Windows".
> +	This string is retrieved from the `sysname` field of the struct returned
> +	by the uname(2) system call. Defaults to true.

Presumably, both ends of the connection independently choose whether they enable or disable this variable, so we have 2x2=4 combinations (here, versions of Git before the os-version capability support is introduced behave the same way as an installation with this configuration variable set to false).

And among these four combinations, only one of them results in "send to each other", but the description above is fuzzy.

Show 10 quoted lines

> diff --git a/connect.c b/connect.c
> index 10fad43e98..6d5792b63c 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -492,6 +492,9 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)
>  	if (server_supports_v2("agent"))
>  		packet_write_fmt(fd_out, "agent=%s", git_user_agent_sanitized());
>  
> +	if (server_supports_v2("os-version") && advertise_os_version(the_repository))
> +		packet_write_fmt(fd_out, "os-version=%s", os_version_sanitized());

Not a new problem, because the new code is pretty-much a straight copy from the existing "agent" code, but do we ever use unsanitized versions of git-user-agent and os-version? If not, I am wondering if we should sanitize immediately when we obtain the raw string and keep it, get rid of _santized() function from the public API, and make anybody calling git_user_agent() and os_version() to get sanitized safe-to-use strings.

I see http.c throws git_user_agent() without doing any sanitization at the cURL library, but it may be a mistake that we may want to fix (outside the scope of this topic). Since the contrast between the os_version() vs the os_version_sanitized() is *new* in this series, however, we probably would want to get it right from the beginning.

So the question is again, do we ever need to use os_version() that is a raw string that may require sanitizing? I do not think of any offhand.