From: Christian Couder Date: Mon, 19 May 2025 14:11:24 GMT Subject: Re: [PATCH v2 2/3] promisor-remote: allow a server to advertise more fields Message-ID: In-Reply-To: On Wed, May 7, 2025 at 2:45 PM Karthik Nayak wrote: > > Christian Couder 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= > > +promisor-remote= > > ~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > 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 should be of the > > +instead of this repository. In this case 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!