On Mon, Jul 21, 2025 at 10:59 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
> > diff --git a/promisor-remote.c b/promisor-remote.c
> > index ae2c49a0a0..501cb92391 100644
> > --- a/promisor-remote.c
> > +++ b/promisor-remote.c
> > @@ -388,6 +388,20 @@ static struct string_list *fields_sent(void)
> > return &fields_list;
> > }
> >
> > +static struct string_list *fields_checked(void)
> > +{
> > + static struct string_list fields_list = STRING_LIST_INIT_NODUP;
> > + static int initialized = 0;
>
> No need to explicitly 0 initialize "static int"; let BSS take care
> of it. Perhaps we should add an entry to CodingGuidelines if we do
> not have one (#leftoverbits).
Yeah, right. This is fixed in v7 in both fields_checked() and fields_sent().
> > @@ -533,6 +547,61 @@ enum accept_promisor {
> > ACCEPT_ALL
> > };
> >
> > +static int match_field_against_config(const char *field, const char *value,
> > + struct promisor_info *config_info)
> > +{
> > + if (config_info->filter && !strcasecmp(field, promisor_field_filter))
> > + return !strcmp(config_info->filter, value);
> > + else if (config_info->token && !strcasecmp(field, promisor_field_token))
> > + return !strcmp(config_info->token, value);
> > +
> > + return 0;
> > +}
> > +
> > +static int all_fields_match(struct promisor_info *advertised,
> > + struct string_list *config_info,
> > + int in_list)
> > +{
> > + struct string_list* fields = fields_checked();
>
> Asterisk sticks to a variable, not type. I.e.
>
> struct string_list *fields = ...;
Right, fixed in v7.
> > + struct string_list_item *item_checked;
> > +
> > + for_each_string_list_item(item_checked, fields) {
> > + int match = 0;
> > + const char *field = item_checked->string;
> > + const char *value = NULL;
> > + struct string_list_item *item;
> > +
> > + if (!strcasecmp(field, promisor_field_filter))
> > + value = advertised->filter;
> > + else if (!strcasecmp(field, promisor_field_token))
> > + value = advertised->token;
>
> Hmph, together with match_field_against_config(), do we really need
> to have this case insensitive to begin with?
I think so. When comparing config option keys, we use case insensitive
comparisons, so I think it makes sense to use that when comparing
field names too.
When we call fields_checked(), we get a list of field names as they
have been configured in the "promisor.checkFields" config variable. It
is very likely that for "partialCloneFilter" some users may not
camelcase it properly. So if we use strcmp() to check if `field` is
"partialCloneFilter", it might result in things not working as they
expect because of subtle camelcase issues.
> I would suggest making
> it a habit to design the interface minimally, and not making case
> insensitive comparison as default counts one of them.
There is a reason. They are not used by default here.
> If the
> comparison were not case insensitive, we do not even need to have
> this loop; rather we can just look up from the list of the fields
> for an exact string (i.e. promisor_field_filter). I do not know
> offhand what other code will become easier to read and simpler by
> such a change.
I checked other places in the code and I think it's right that in the
"promisor-remote" protocol the field names ("partialCloneFilter" and
"token") should be checked case-sensitively, so in v7 I have made this
change in parse_one_advertised_remote():
@@ promisor-remote.c: static struct promisor_info
*parse_one_advertised_remote(stru
info->name = value;
else if (!strcmp(elem, "url"))
info->url = value;
-+ else if (!strcasecmp(elem, promisor_field_filter))
++ else if (!strcmp(elem, promisor_field_filter))
+ info->filter = value;
-+ else if (!strcasecmp(elem, promisor_field_token))
++ else if (!strcmp(elem, promisor_field_token))
+ info->token = value;
else
free(value);
and I have clarified that in the doc too:
@@ Documentation/gitprotocol-v2.adoc: retrieving the header from a
bundle at the in
+connecting to the remote. It corresponds to the "remote.<name>.token"
+config setting.
+
-+No other fields are defined by the protocol at this time. Clients MUST
-+ignore fields they don't recognize to allow for future protocol
-+extensions.
++No other fields are defined by the protocol at this time. Field names
++are case-sensitive and MUST be transmitted exactly as specified
++above. Clients MUST ignore fields they don't recognize to allow for
++future protocol extensions.
+
+For now, the client can only use information transmitted through these
+fields to decide if it accepts the advertised promisor remote. In the
But when we deal with field names from the config, I think it's much
more user friendly to use case insensitive comparisons.
An alternative implementation might be to canonize the field names in
fields_checked() and fields_sent(), so that for example
"partialclonefilter" would be changed to "partialCloneFilter" there.
We could also use for example an `enum field_id` instead of field
names and convert the field names to those field IDs in
fields_checked(), fields_sent() and perhaps other places. Then yeah we
might use case sensitive comparisons in some places like here and
perhaps simplify other parts of the code. I am open to going in this
direction if that's what you prefer. I am not sure overall the code
will be much simpler though. We might just move complexity around.
Also for now I think using strcasecmp() is not likely to be a
performance issue.
Thanks.
