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

> > +static int is_valid_field(struct string_list_item *item, void *cb_data)
> > +{
> > +     const char *field = item->string;
> > +     const char *config_key = (const char *)cb_data;
> > +
> > +     if (!is_known_field(field)) {
> > +             warning(_("unsupported field '%s' in '%s' config"), field, config_key);
> > +             return 0;
> > +     }
> > +     return 1;
> > +}
>
> Ok, so if the server has an unknown sendField value configured, the
> value is ignored by the server and a warning printed.

> > +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) {
>
> We could use `repo_config_get_string()` here instead and wire the nearby
> `struct repository`, but as the rest of the file is not doing so it's
> not really a big deal.

> > +             string_list_split_in_place(fields_list, fields, ", ", -1);
> > +             filter_string_list(fields_list, 0, is_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");
> > +             initialized = 1;
> > +     }
> > +
> > +     return &fields_list;
> > +}
>
> Are there scenarios where `fields_sent()` is getting invoked more than
> once? My understanding is that this is invoked only when the capability
> is being advertised. Regardless, I wonder if we really need the static
> initialization here.

> > diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
> > index cb061b1f35..27c32b2573 100755
> > --- a/t/t5710-promisor-remote-capability.sh
> > +++ b/t/t5710-promisor-remote-capability.sh
> > @@ -295,6 +295,38 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
> >       check_missing_objects server 1 "$oid"
> >  '
> >
> > +test_expect_success "clone with promisor.sendFields" '
> > +     git -C server config promisor.advertise true &&
> > +     test_when_finished "rm -rf client" &&
> > +
> > +     git -C server remote add otherLop "https://invalid.invalid"  &&
> > +     git -C server config remote.otherLop.token "fooBar" &&
> > +     git -C server config remote.otherLop.stuff "baz" &&
> > +     git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
> > +     test_when_finished "git -C server remote remove otherLop" &&
> > +     git -C server config promisor.sendFields "partialCloneFilter, token" &&
>
> This configuration results in the unsupported field warning message
> being printed. This is because of the SP following the comma resulting
> in an empty value in the middle after the string split.