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

[Christian Couder <christian.couder@gmail.com>]

On Wed, May 7, 2025 at 2:45 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> [snip]
>
> > 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
> >
>
> From this, it seems like the order of the fields shouldn't matter, but
> this is not the case.

I would prefer to keep the simpler version as I find these grammar
notations not very easy to understand.

In v3 there is:

=> The "name" and "url" fields MUST appear first in each pr-fields, in
that order.

And I think it's enough, especially since the parsing is relaxed so it
will work if they are not in this order.

> wouldn't it be better to say:
>
>   pr-infos = pr-info | pr-infos ";" pr-info
>
>   pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
>   pr-info = pr-info | pr-info "," fld-name "=" fld-value

Could this be interpreted to mean that it is Ok to have "name="
pr-name "," "url=" pr-url several times in a single pr-info?

This seems to me really more complex than needed, and I think it's
better to keep the simple version and then clarify things with the
above sentence.

> [snip]
>
> > 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;
> > +}
> > +
> > +static int valid_field(struct string_list_item *item, void *cb_data)
> > +{
>
> Nit: Shouldn't this be `is_valid_field` similar to `is_allowed_field`?

Yeah, I have renamed it `is_valid_field`

> > +     const char *field = item->string;
> > +     const char *config_key = (const char *)cb_data;
> > +
> > +     if (!is_allowed_field(field)) {
>
> Nit: Can't we just inline this?

We could, but I think the is_allowed_field() makes sense on its own
too, while the current one is very specific to be used in
filter_string_list(). So I prefer keeping is_allowed_field() separate.

> > +             warning(_("unsupported field '%s' in '%s' config"), field, config_key);
> > +             return 0;
> > +     }
> > +     return 1;
> > +}
> > +
> > +static char *fields_from_config(struct string_list *fields_list, const char *config_key)
> > +{
> > +     char *fields = NULL;
> > +
> > +     if (!git_config_get_string(config_key, &fields) && *fields) {
> > +             string_list_split_in_place(fields_list, fields, ", ", -1);
> > +             filter_string_list(fields_list, 0, valid_field, (void *)config_key);
> > +     }
> > +
> > +     return fields;
> > +}
> > +
> > +static struct string_list *fields_sent(void)
> > +{
> > +     static struct string_list fields_list = STRING_LIST_INIT_NODUP;
> > +     static int initialized = 0;
> > +
> > +     if (!initialized) {
> > +             fields_list.cmp = strcasecmp;
> > +             fields_from_config(&fields_list, "promisor.sendFields");
>
> Nit: Here too, can't this be inlined? While the modularity is nice, I'm
> not sure the redirection is warranted for such small functions with very
> specific usecases.

In a follow up patch in this series we reuse fields_from_config() so I
think it's better to keep it separate.

> [snip]
>
> Apart from the nits, the patch looks good :)

Thanks for the review!