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

[Christian Couder <christian.couder@gmail.com>]

On Wed, May 21, 2025 at 10:36 PM Justin Tobler <jltobler@gmail.com> wrote:
>
> On 25/05/19 04:12PM, Christian Couder wrote:

> > +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.

Yes.

> > +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.

Yeah, removing use of the_repository in this file is left for another
future patch series.

> > +             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.

I am not sure if `fields_sent()` could be invoked more than once (by
the same process), but I think it's very risky to rely on it being
called just once, and I think it doesn't cost much to be safe.

> > 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.

Yeah, right, in the v4 I have added a call to
string_list_remove_empty_items() to avoid spurious empty values and
related warnings.

> > +     test_when_finished "git -C server config unset promisor.sendFields" &&
>
> I think we could use `test_config` which would automatically unset the
> configuration after the test.

Yeah, `test_config` is used in v4 to avoid this test_when_finished.

Thanks for your review!