On Mon, Jul 21, 2025 at 8:53 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Christian Couder <christian.couder@xxxxxxxxx> writes: > > > 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. > >> 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. > > 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, 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.
