Subject: Re: [PATCH 7/9] list-objects-filter-options: implement auto filter resolution

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 7 Jan 2026 11:05:36 +0100

On Tue, Dec 23, 2025 at 12:11:11PM +0100, Christian Couder wrote:
> In a following commit, we will need to aggregate filters from multiple
> accepted promisor remotes into a single filter.

Ah, interesting. I was always operating under the assumption that when
the server advertises multiple promisors, the client will pick only one
of them. And that made me wonder how the client knows which one to pick
in the first place.

But of course it's possible to just pick _all_ of them by combining the
filter.

> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index f13ae5caeb..4a9c1991c1 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -230,6 +230,41 @@ static void filter_spec_append_urlencode(
>  		     filter->filter_spec.buf + orig_len);
>  }
>  
> +char *list_objects_filter_combine(const struct string_list *specs)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!specs->nr)
> +		return NULL;
> +
> +	if (specs->nr == 1)
> +		return xstrdup(specs->items[0].string);
> +
> +	strbuf_addstr(&buf, "combine:");
> +
> +	for (size_t i = 0; i < specs->nr; i++) {
> +		const char *spec = specs->items[i].string;
> +		if (i > 0)
> +			strbuf_addch(&buf, '+');
> +
> +		strbuf_addstr_urlencode(&buf, spec, allow_unencoded);

Shouldn't we use `filter_spec_append_urlencode()` to do this?

> +	}
> +
> +	return strbuf_detach(&buf, NULL);
> +}

I'm surprised we didn't have such a function yet.

> +void list_objects_filter_resolve_auto(struct list_objects_filter_options *filter_options,
> +	char *new_filter, struct strbuf *errbuf)
> +{
> +	if (filter_options->choice != LOFC_AUTO)
> +		return;

I wonder whether we should rather `BUG()` in case the filter is not an
"auto" filter. Otherwise it's easy to get the callsite wrong, as the
user may expect that the filter gets resolved tdo the new filter, but
it's actually not because the original filter wasn't an "auto" filter in
the first place.

> +	list_objects_filter_release(filter_options);
> +
> +	if (new_filter)
> +		gently_parse_list_objects_filter(filter_options, new_filter, errbuf);
> +}

So as menitoned in a preceding commit `list_objects_filter_release()`,
will retain the `allow_auto` option. But when resolving "auto" filters
I'd expect us to not accept "auto" in the resolved filter anymore.
Otherwise, if `new_filter` was "auto", we'd still end up with an auto
filter, wouldn't we? I'd rather expect us to abort in that case.

Patrick