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
