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

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

> 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?

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