On Mon, Jul 21, 2025 at 8:53 PM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

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

I am not sure why it discourages reponses. People can still reply to
the v6 patch or to the discussion that started from the v5 patch or
both. The discussion from the v5 patch is not obsolete if some
comments have not been properly addressed.

Also the fact that the replies come at the same time as a new version,
means that reviewers don't need to context switch as much as if there
were for example some replies at one time, then a few days later
replies to the rest of the reviews, then a few days later a new
version. So for reviewers who don't like to context switch much it
should be better if everything is sent at once.

quoted

quoted

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.

I'd like it if there was a consensus on doing such a refactoring
before starting to work on it. So if you are fine with that
refactoring, I am willing to either add it as part of this series, or
work on it after this series is merged (like for the recent
cc/t9350-cleanup patch), as you prefer.

I couldn't know your and others' opinion on it (and I am still not
sure about it) before we started discussing it. Sometimes for example
you are also fine with just leaving over those kinds of bits. So for
now in v7, I haven't changed this, but let me know and I will make the
necessary changes in v8 or later in separate patches.

quoted

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.

quoted

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,

You mean using "promisor.sendVariables" and "promisor.checkVariables"
instead of "promisor.sendFields" and "promisor.checkFields", and using
"variable" instead of "field" everywhere in the documentation and the
code?

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?

I just think it will be confusing because after being sent over
through the protocol, they are no longer variable in the sense that
the client can't (and shouldn't) change them. They could also be
confused with environment variables, so perhaps
"promisor.sendConfigVariables" and "promisor.checkConfigVariables",
but the names will start to be long especially if we have to say that
they are from the server when processed on the client side.

Also I think it's better to use a different name because they are used
differently, serve different purposes and have different rules applied
to them in the context of the promisor-remote protocol capability.

Anyway if that's what is prefered, I will make the change. For now in
the v7, I haven't changed this.