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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 29, 2025 at 04:52:42PM +0200, Christian Couder wrote:
> diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> index 2638b01f83..71311b70c8 100644
> --- a/Documentation/config/promisor.adoc
> +++ b/Documentation/config/promisor.adoc
> @@ -9,6 +9,24 @@ promisor.advertise::
>  	"false", which means the "promisor-remote" capability is not
>  	advertised.
>  
> +promisor.sendFields::
> +	A comma or space separated list of additional remote related
> +	fields that a server will send while advertising its promisor
> +	remotes using the "promisor-remote" capability, see
> +	linkgit:gitprotocol-v2[5]. Currently, only the
> +	"partialCloneFilter" and "token" fields are supported. The
> +	"partialCloneFilter" field contains the partial clone filter
> +	used for the remote, and the "token" field contains an
> +	authentication token for the remote.
> ++

Should we maybe convert this into a list of accepted fields? Makes it
easier to extend going forward.

Furthermore, should we maybe refactor this to match the restrictive
design where valid fields are explicitly specified? In other words,
should we have separate config keys for each of the accepted fields now?

Also, shouldn't this setting be per promisor remote that we want to
advertise? I expect that servers will want to send different partial
clone filters for each of the advertised remotes, and they may also want
to send different tokens. So it seems a bit too inflexible to only have
a single, global "sendFields" configuration that covers all promisors.

> diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
> index 5598c93e67..b4648a7ce6 100644
> --- a/Documentation/gitprotocol-v2.adoc
> +++ b/Documentation/gitprotocol-v2.adoc
> @@ -785,33 +785,52 @@ 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>
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
>  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 = fld-name "=" fld-value | pr-fields "," pr-fields

Tiny nit, but can we maybe spell out "fld" fully? It doesn't buy us that
much to abbreviate "field", and it did cause my reading to trip.

> -where `pr-name` is the urlencoded name of a promisor remote, and
> -`pr-url` the urlencoded URL of that promisor remote.
> +where all the `fld-name` and `fld-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`.
>  
> -	pr-names = pr-name | pr-names ";" pr-name
> +The server MAY advertise the following optional fields:
> +
> +- "partialCloneFilter": Filter used for partial clone, corresponding
> +  to the "remote.<name>.partialCloneFilter" config setting.
> +- "token": Authentication token for the remote, corresponding
> +  to the "remote.<name>.token" config setting.

I think we should define semantics of these fields more closely. What
exactly is the consequence of a partial clone filter being defined? Does
it mean that this promisor remote should only be used in case we do have
the exact same filter passed to git-clone(1)? Does it mean that the
remote only contains objects that would've been filtered _out_ by such a
filter?

Furthermore, we should specify how the token is supposed to be passed to
the remote.

> +No other fields are defined by the protocol at this time. Clients SHOULD
> +ignore fields they don't recognize to allow for future protocol extensions.

Shouldn't we require clients to ignore unknown fields? Otherwise, if
it's only optional to ignore them, we still can't introduce new fields
in the future without breaking existing clients that chose to ignore
this guidance.

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 24d0e70132..70abec4c24 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -314,6 +314,84 @@ static int allow_unsanitized(char ch)
>  	return ch > 32 && ch < 127;
>  }
>  
> +/*
> + * List of field names allowed to be used in the "promisor-remote"
> + * protocol capability. Each field should correspond to a configurable
> + * property of a remote that can be relevant for the client.
> + */
> +static const char *allowed_fields[] = {
> +	"partialCloneFilter", /* Filter used for partial clone */
> +	"token",              /* Authentication token for the remote */
> +	NULL
> +};
> +
> +/*
> + * Check if 'field' is in the list of allowed field names for the
> + * "promisor-remote" protocol capability.
> + */
> +static int is_allowed_field(const char *field)
> +{
> +	const char **p;
> +
> +	for (p = allowed_fields; *p; p++)
> +		if (!strcasecmp(*p, field))
> +			return 1;
> +	return 0;
> +}

Nit: it is a bit funny that we talk about allowed fields here, but
the recommendation is to just ignore unknown fields. So maybe this
should instead be called "known_fields".

Patrick