On Tue, May 27, 2025 at 9:51 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> 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.
Yeah, right, in the v4, I have clarified the doc here and in other
places to talk about "field names" when relevant.
> > 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 *)?
Using "static const char *" is a bit less efficient, as it uses an
additional pointer to point to the literal string.
> 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"
This has been discussed a few times in the past on the mailing list
but I can't find any references to such discussions now.
As far as I recall the first discussion I had about this was with
Dscho who was in favor of using "static const char []" because it's
more type safe than "#define ..." and more efficient than "static
const char *".
> > -/* 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/
I have made this change in the v4.
Thanks for your review!
