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

From: Jean-Noël Avila <[email-masked]>
Date: Fri, 27 Jun 2025 20:47:53 +0200

Le 25/06/2025 à 14:50, Christian Couder a écrit :
> For now the "promisor-remote" protocol capability can only pass "name"
> and "url" information from a server to a client in the form
> "name=<remote_name>,url=<remote_url>".
> 
> Let's make it possible to pass more information by introducing a new
> "promisor.sendFields" configuration variable. This variable should
> contain a comma or space separated list of field names that will be
> looked up in the configuration of the remote on the server to find the
> values that will be passed to the client.
> 
> Only a set of predefined fields are allowed. The only fields in this
> set are "partialCloneFilter" and "token". The "partialCloneFilter"
> field specifies the filter definition used by the promisor remote,
> and the "token" field can provide an authentication credential for
> accessing it.
> 
> For example, if "promisor.sendFields" is set to "partialCloneFilter",
> and the server has the "remote.<name>.partialCloneFilter" config
> variable set to a value for a remote, then that value will be passed
> in the form "partialCloneFilter=<value>" after the "name" and "url"
> fields.
> 
> A following commit will allow the client to use the information to
> decide if it accepts the remote or not. For now the client doesn't do
> anything with the additional information it receives.
> 
> Signed-off-by: Christian Couder <[email-masked]>
> ---
>  Documentation/config/promisor.adoc    |  22 +++++
>  Documentation/gitprotocol-v2.adoc     |  59 +++++++++---
>  promisor-remote.c                     | 134 ++++++++++++++++++++++++--
>  t/t5710-promisor-remote-capability.sh |  31 ++++++
>  4 files changed, 221 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> index 2638b01f83..beb8f65518 100644
> --- a/Documentation/config/promisor.adoc
> +++ b/Documentation/config/promisor.adoc
> @@ -9,6 +9,28 @@ promisor.advertise::
>  	"false", which means the "promisor-remote" capability is not
>  	advertised.
>  
> +promisor.sendFields::
> +	A comma or space separated list of additional remote related
> +	field names. A server will send these field names and the
> +	associated field values from its configuration when
> +	advertising its promisor remotes using the "promisor-remote"
> +	capability, see linkgit:gitprotocol-v2[5]. Currently, only the
> +	"partialCloneFilter" and "token" field names are supported.
> ++
> +* "partialCloneFilter": contains the partial clone filter
> +  used for the remote.
> ++
> +* "token": contains an authentication token for the remote.
> ++

This kind of text structure calls a description list instead and you can
already use backquotes:

`partialCloneFilter`:: contains the partial clone filter
> +  used for the remote.
> ++
> +`token`:: contains an authentication token for the remote.

> +When a field name is part of this list and a corresponding
> +"remote.foo.<field name>" config variable is set on the server to a

Please no space in placeholders: <field-name>

> +non-empty value, then the field name and value will be sent when
> +advertising the promisor remote "foo".
> ++
> +This list has no effect unless the "promisor.advertise" config
> +variable is set to "true", and the "name" and "url" fields are always
> +advertised regardless of this setting.
> +

More generally, I am a bit annoyed by the usage of the "will" auxiliary
when not expressing a true future. For an international audience, this
can be misleading. The plain language[1] philosophy mandates to not use
auxiliaries other than where they are required (no convoluted sentences).

Would it make sense to start a style guide to help writing consistent
documentation that targets people whose first language is not English?
Being an non native speaker, I often find our docs too literate, with
lengthy sentences.

[1] https://en.wikipedia.org/wiki/Plain_language

>  promisor.acceptFromServer::
>  	If set to "all", a client will accept all the promisor remotes
>  	a server might advertise using the "promisor-remote"
> diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
> index 9a57005d77..0583fafa09 100644
> --- a/Documentation/gitprotocol-v2.adoc
> +++ b/Documentation/gitprotocol-v2.adoc
> @@ -785,33 +785,59 @@ retrieving the header from a bundle at the indicated URI, and thus
>  save themselves and the server(s) the request(s) needed to inspect the
>  headers of that bundle or bundles.
>  
> -promisor-remote=<pr-infos>
> +promisor-remote=<pr-info>
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~

Be careful to adjust the length of the underline to the one of the title

>  
>  The server may advertise some promisor remotes it is using or knows
 >  about to a client which may want to use them as its promisor
remotes,> -instead of this repository. In this case <pr-infos> should be
of the
> +instead of this repository. In this case <pr-info> should be of the
>  form:
>  
> -	pr-infos = pr-info | pr-infos ";" pr-info
> +	pr-info = pr-fields | pr-info ";" pr-info
>  
> -	pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
> +	pr-fields = field-name "=" field-value | pr-fields "," pr-fields
>  
> -where `pr-name` is the urlencoded name of a promisor remote, and
> -`pr-url` the urlencoded URL of that promisor remote.
> +where all the `field-name` and `field-value` in a given `pr-fields`
> +are field names and values related to a single promisor remote.
>  
> -In this case, if the client decides to use one or more promisor
> -remotes the server advertised, it can reply with
> -"promisor-remote=<pr-names>" where <pr-names> should be of the form:
> +The server MUST advertise at least the "name" and "url" field names
> +along with the associated field values, which are the name of a valid
> +remote and its URL, in each `pr-fields`. The "name" and "url" fields
> +MUST appear first in each pr-fields, in that order.
>  
> -	pr-names = pr-name | pr-names ";" pr-name
> +After these mandatory fields, the server MAY advertise the following
> +optional fields in any order:
> +
> +- "partialCloneFilter": The filter specification used by the remote.
> +Clients can use this to determine if the remote's filtering strategy
> +is compatible with their needs (e.g., checking if both use "blob:none").
> +It corresponds to the "remote.<name>.partialCloneFilter" config setting.
> +
> +- "token": An authentication token that clients can use when
> +connecting to the remote. It corresponds to the "remote.<name>.token"
> +config setting.
> +

This list can be turned into a description list.

> +No other fields are defined by the protocol at this time. Clients MUST
> +ignore fields they don't recognize to allow for future protocol
> +extensions.
> +
> +For now, the client can only use information transmitted through these
> +fields to decide if it accepts the advertised promisor remote. In the
> +future that information might be used for other purposes though.
> +
> +Field values MUST be urlencoded.
> +
> +If the client decides to use one or more promisor remotes the server
> +advertised, it can reply with "promisor-remote=<pr-names>" where
> +<pr-names> should be of the form:
> +
> +	pr-names = pr-name | pr-names ";" pr-names

Here the syntax used is not compatible with synopsis. Would it make
sense to uniformize it, or is BNF ok?

>  
>  where `pr-name` is the urlencoded name of a promisor remote the server
>  advertised and the client accepts.
>  
> -Note that, everywhere in this document, `pr-name` MUST be a valid
> -remote name, and the ';' and ',' characters MUST be encoded if they
> -appear in `pr-name` or `pr-url`.
> +Note that, everywhere in this document, the ';' and ',' characters
> +MUST be encoded if they appear in `pr-name` or `field-value`.
>  
>  If the server doesn't know any promisor remote that could be good for
>  a client to use, or prefers a client not to use any promisor remote it
> @@ -822,9 +848,10 @@ In this case, or if the client doesn't want to use any promisor remote
>  the server advertised, the client shouldn't advertise the
>  "promisor-remote" capability at all in its reply.
>  
> -The "promisor.advertise" and "promisor.acceptFromServer" configuration
> -options can be used on the server and client side to control what they
> -advertise or accept respectively. See the documentation of these
> +On the server side, the "promisor.advertise" and "promisor.sendFields"
> +configuration options can be used to control what it advertises. On
> +the client side, the "promisor.acceptFromServer" configuration option
> +can be used to control what it accepts. See the documentation of these
>  configuration options for more information.
>  
Thanks

Jean-Noël