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

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 19, 2025 at 04:12:56PM +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.
> ++
> +When a field is part of this list and a corresponding
> +"remote.foo.<field>" config variable is set on the server to a
> +non-empty value, then the field and its value will be sent when
> +advertising the promisor remote "foo". This list has no effect unless
> +the "promisor.advertise" config variable is set to "true", and the
> +"name" and "url" fields are always advertised regardless of this
> +setting.

I think this documentation should be clarified to explicitly talk about
"field names". In v2 I misread these paragraphs to mean that the admin
is expected to configure name-value pairs because you say "fields" here,
and that term is specified elsewhere to be such a pair.

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 94e87f2f48..cde4079d8c 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -314,6 +314,73 @@ static int allow_unsanitized(char ch)
>  	return ch > 32 && ch < 127;
>  }
>  
> +static const char promisor_field_filter[] = "partialCloneFilter";
> +static const char promisor_field_token[] = "token";

Curious. Why aren't these declared as mere string constants (static
const char *)? It might be a bit more idiomatic to have these as
all-uppercase defines to make it obvious that those aren't a local
variable.

    #define PROMISOR_FIELD_FILTER "partialCloneFilter"
    #define PROMISOR_FIELD_TOKEN  "token"

> @@ -326,6 +393,8 @@ struct promisor_info {
>  	struct promisor_info *next;
>  	const char *name;
>  	const char *url;
> +	const char *filter;
> +	const char *token;
>  };
>  
>  static void promisor_info_list_free(struct promisor_info *p)

Yup, this now follows my suggestion.

> @@ -336,12 +405,45 @@ static void promisor_info_list_free(struct promisor_info *p)
>  		next = p->next;
>  		free((char *)p->name);
>  		free((char *)p->url);
> +		free((char *)p->filter);
> +		free((char *)p->token);
>  		free(p);
>  	}
>  }
>  
> -/* Prepare a 'struct promisor_info' linked list with config information. */
> -static struct promisor_info *promisor_config_info_list(struct repository *repo)
> +static void set_one_field(struct promisor_info *p,
> +			  const char *field, const char *value)
> +{
> +	if (!strcasecmp(field, promisor_field_filter))
> +		p->filter = xstrdup(value);
> +	else if (!strcasecmp(field, promisor_field_token))
> +		p->token = xstrdup(value);
> +	else
> +		BUG("Invalid field '%s'", field);

s/Invalid/invalid/

Patrick