Re: [PATCH v5 2/5] promisor-remote: allow a server to advertise more fields

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2025, 18:53 UTC
Message-ID: <xmqqtt35xtng.fsf@gitster.g>
In-Reply-To: <CAP8UFD1wYXrEC2VYBHTL7NS1ksSa0oiBgB3Ua=6V1SnP=+RMsQ@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 5 quoted lines

> Ok, I have changed it to the following in v6:
>
>    To allow clients to make more informed decisions about which promisor
>    remotes they accept, let's make it possible to pass more information
>    by introducing a new "promisor.sendFields" configuration variable.

A meta observation, as I haven't even thought if the above proposal makes sense or not, but a reponse for v5 like the above that comes nearly at the same minite as v6 highly discourages any response to this message that may want to say "oh, that is not a good idea because ...".

Show 14 quoted lines

>> By making it easier for casual humans who manually write the
>> configuration variable (presumably while testing) and allowing both
>> comma and space as separator, this design decision is forcing one
>> more rule to worry about for those who are writing the parser for
>> the value.  There may be some existing configuration variables with
>> such a "leninent" syntax, but I'd rather see us not make the mess
>> even worse.
>
> We indeed have a number of configuration variables accepting lists of
> items separated by both comma and space. As we cannot fix those easily
> for backward compatibility and they still make things a bit simpler
> for users, my opinion is that we'd better bite the bullet and make
> sure we have a simple and hard-to-misuse standard way to parse such
> lists.

The position is understandable (I am not saying agreeable), but if one takes such a position, this series will get even longer, by requiring such a refactoring and new set of helper functions in the front of the series, to avoid making things even worse than they currently are. If you want to punt on that "simple standard way" and leave it outside the series, then please do not add such syntax to the variables in this series.

Show 43 quoted lines

>> I know the name "field" was discussed in earlier iterations, but
>> these three lines together with "For example" in a later paragraph,
>> it hints to me that this mechanism is to choose variable-value pairs
>> for which among remotes.<name>.* variables to send after name=<name>
>> and url=<url>; is my understanding correct?  If so, can we clarify
>> the paragraphs around here even more so that I do not even have to
>> ask this question?
>
> Yeah, it seems to me that there was previously a sentence about what
> fields are, but it looks like I removed it by mistake. Anyway, before
> the paragraph about what the "promisor.sendFields" configuration
> variable contains, I have added the following in v6:
>
>    On the server side, information about a remote `foo` is stored in
>    configuration variables named `remote.foo.<variable-name>`. To make
>    it clearer and simpler, we use `field` and `field name` like this:
>
>      * `field name` refers to the <variable-name> part of such a
>        configuration variable, and
>
>      * `field` refers to both the `field name` and the value of such a
>        configuration variable.
>
>> What do we call the third-level of a variable name in the
>> configuration file?  The description on the "--regexp" option in
>> "git config --help" hints one:
>>
>>     With `get`, interpret the name as a regular expression. Regular
>>     expression matching is currently case-sensitive and done against
>>     a canonicalized version of the key in which section and variable
>>     names are lowercased, but subsection names are not.
>>
>> So a for remote.origin.partialCloneFilter, "remote" is the section
>> name, "origin" is the subsection name, and "partialCloneFilter" is
>> the variable name.
>
> Yeah, I thought that "variable name" could be confusing, so I prefered
> to use another word, "field", to talk about this.
> ...
> I'd rather avoid talking about "variable name" other than to explain
> what "field name" and "field" are. I think it would be too confusing,
> especially if the name of the new config options are still
> "promisor.sendFields" and "promisor.checkFields".

Sorry, the argument does not make much sense to me.

After all, the end-users who follow the documentation needs to know which *VARIABLES* (in remotes.<name><VARIABLE>) to set to affect the value that this mechanism lets them send out. And the configuration variable to control which of remotes.<name>.<VARIABLE> are sent out is a new thing, and it itself calls it a FIELD. Whatever name that new thing chooses to call itself, the fact is that it is about what the users have long known as configuration variables.

So, what makes this whole thing more confusing than necessary, to me, looks like your use of the word "field" in the first place. The use of word "field" in "sendFields" and "checkFields" are much much more recent than the concept of "configuration variables" that has long been established in users' minds (as far as I know, these "fields" are not even in any released versions), so I do not see why we want to keep it and force users learn yet another word. Just fix the name of these new configuration variables, explain that they are used to name other configuration variables, and be done it without uttering "field" even once, and we would be good and less confusing, wouldn't we? Or am I being too naive and forgetting some already established use of sendFields and checkFields?