From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 07 May 2025 12:44:59 GMT
Subject: Re: [PATCH v2 2/3] promisor-remote: allow a server to advertise more fields
Message-ID: <CAOLa=ZSwzvfQ8MupFzuMEpHCKJRY8p06m-FfyE-deqOsgK_feg@mail.gmail.com>
In-Reply-To: <20250429145243.992252-3-christian.couder@gmail.com>

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

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

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

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

[snip]

Apart from the nits, the patch looks good :)