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

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Jan 8, 2025, 13:06 UTC
Message-ID: <CAPSxiM_0h7OyQO-Of8YhcOt4KbtuoKXe111ZCvsLf5y+OgCHaw@mail.gmail.com>
In-Reply-To: <xmqqfrlvzzcd.fsf@gitster.g>

On Mon, Jan 6, 2025 at 9:52 PM Junio C Hamano <gitster@pobox.com> wrote:

Hi Junio,

Show 12 quoted lines

> Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
> > +
> > +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.
>
> Shouldn't `sysname` be typeset as a literal, just like `true` and
> `os-version`?

I will do that in the next iteration. Thank you.

Show 15 quoted lines

>
> > +os-version
> > +~~~~~~~~~~
> > +
> > +In the same way as the `agent` capability above, the server can
> > +advertise the `os-version` capability with a value `X` (in the form
> > +`os-version=X`) to notify the client that the server is running an
> > +operating system that can be identified by `X`. The client may
>
> Hmph.  I am not sure what's the value of mentioning 'X' here.  To me
>
>     ... can advertise the `os-version` capability to notify the kind
>     of operating system it is running on.
>
> conveys the same thing with much fewer bytes.

Yeah, it is better, I will use it in the next iteration.

Show 11 quoted lines

>
> > +optionally send its own `os-version` string by including the
> > +`os-version` capability with a value `Y` (in the form `os-version=Y`)
> > +in its request to the server (but it MUST NOT do so if the server did
> > +not advertise the os-version capability). The `X` and `Y` strings may
> > +contain any printable ASCII characters except space (i.e., the byte
>
> This is misleading.  ASCII printable characters range from 33 to 126
> (inclusive), but by saying "except space", the readers are led to
> believe that the author of this documentation thinks ASCII 32 is
> printable, too.

Thanks for this, I will make changes in the next iteration.

Show 5 quoted lines

>
> About 'X' and 'Y', we can just say "the value of this capability may
> consist of ASCII printable characters (from 33 to 126 inclusive)" or
> something.
>

Noted. Thank you.

Show 12 quoted lines

> Is there a need for a registry of canonical os-version strings?  One
> reason why you would want this user-settable (as opposed to being
> derived from "uname -s") is that a system that is presumably the
> same in end-user perception can call itself in different names (your
> Windows/MINGW64 example) and having the users set it to a string
> chosen from a small repertoire, the other end would be able to
> identify them more easily.  I do not think it is a necessarily a
> good idea to limit what value the users can set to this
> configuration variable, but at least with a published guideline on
> calling various types of systems (and an explanation on the reason
> why we publish such a guideline), users would make an informed
> decision when picking what string to send.

We plan to implement another config option `osVersion.format`, which allow users to fully customize the string sent to the other side using placeholders, similar to how git for-each-ref uses %() syntax. The user would be able to set it to the string they want i.e "Linux" or "Windows" (without any placeholder) and would be sent as-is. So, the `osVersion.format` should satisfy this need. I will ensure to document this option to tell that it can be used like this and will give a small list of `os-version` strings that can be used in this way.

Show 11 quoted lines

>
> > +# Trim and replace each character with ascii code below 32 or above
> > +# 127 (included) using a dot '.' character.
> > +# Octal intervals \001-\040 and \177-\377
> > +# corresponds to decimal intervals 1-32 and 127-255
> > +test_redact_non_printables () {
> > +    tr -d "\n" | tr "[\001-\040][\177-\377]" "."
> > +}
>
> Just being curious.  Do we need to worry about carriage-returns not
> just line-feeds, and if not why?

The function `tr "[\001-\040][\177-\377]" "."` already replace the carriage-returns with "." the redact_non_printables() will also replace it with ".". Carriage-returns octal code is 015 and decimal code of 13. So, we do not need to worry about it.

>
> Thanks.

Thank you. Usman.