Re: [PATCH 6/9] list-objects-filter-options: support 'auto' mode for --filter

On Wed, Jan 7, 2026 at 11:05 AM Patrick Steinhardt [off-list ref] wrote:

On Tue, Dec 23, 2025 at 12:11:10PM +0100, Christian Couder wrote:

quoted

In a following commit, we are going to allow passing "auto" as a
<filterspec> to the `--filter=<filterspec>` option, but only for some
commands. Other commands that support the `--filter=<filterspec>`
option should still die() when 'auto' is passed.

Okay. I assume the idea is that the user can eventually say `git clone
--filter=auto`, and Git would automatically pick the best filter
advertised by the remote. Sounds reasonable to me.

Yeah, that's the idea.

quoted

Let's set up the "list-objects-filter-options.{c,h}" infrastructure to
support that:

- Add a new `unsigned int allow_auto_filter : 1;` flag to
  `struct list_objects_filter_options` which specifies if "auto" is
  accepted or not.
- Change gently_parse_list_objects_filter() to parse "auto" if it's
  accepted.
- Make sure we die() if "auto" is combined with another filter.
- Update list_objects_filter_release() to preserve the
  allow_auto_filter flag, as this function is often called (via
  opt_parse_list_objects_filter) to reset the struct before parsing a
  new value.

Let's also update `list-objects-filter.c` to recognize the new
`LOFC_AUTO` choice. Since "auto" must be resolved to a concrete filter
before filtering actually begins, initializing a filter with
`LOFC_AUTO` is invalid and will trigger a BUG().

Note that ideally combining "auto" with "auto" could be allowed, but in
practice, it's probably not worth the added code complexity. And if we
really want it, nothing prevents us to allow it in future work.

I guess the question is what this would even mean, and I cannot think
of any benefit to allow `--filter=combine:auto+auto`. So agreed

We could allow `--filter=combine:auto+auto` to mean the same as just
`--filter=auto`. But I also don't see a benefit to allow this now.

quoted

If we ever want to give a meaning to combining "auto" with a different
filter too, nothing prevents us to do that in future work either.

So basically the case where the user knows that they definitely don't
want blobs, and in addition they want to pick the best filter advertised
by the server? Yeah, that sounds like it could eventually be a nice
addition.

Yeah, but I think it's also not needed for now.

quoted

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 7420bf81fe..f13ae5caeb 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c

@@ -52,7 +54,17 @@ int gently_parse_list_objects_filter(
      if (filter_options->choice)
              BUG("filter_options already populated");

-     if (!strcmp(arg, "blob:none")) {
+     if (!strcmp(arg, "auto")) {
+             if (!filter_options->allow_auto_filter) {
+                     strbuf_addstr(
+                             errbuf,
+                             _("'auto' filter not supported by this command"));

Tiny nit: the indentation looks a bit weird here.

I have changed it. Hope it's better now.

quoted

@@ -146,10 +158,20 @@ static int parse_combine_subfilter(

      decoded = url_percent_decode(subspec->buf);

-     result = has_reserved_character(subspec, errbuf) ||
-             gently_parse_list_objects_filter(
+     result = has_reserved_character(subspec, errbuf);
+     if (result)
+             goto cleanup;
+
+     result = gently_parse_list_objects_filter(
                      &filter_options->sub[new_index], decoded, errbuf);
+     if (result)
+             goto cleanup;
+
+     result = (filter_options->sub[new_index].choice == LOFC_AUTO);
+     if (result)
+             strbuf_addstr(errbuf, _("an 'auto' filter cannot be combined"));

Nit: let's maybe also add the `goto cleanup` here. I'm not a fan of
leaving it away for the final statement as it makes it easy to forget
backfilling it in case this function needs to be extended in the future.

Ok, I have added the `goto cleanup`.

quoted

@@ -317,6 +345,7 @@ void list_objects_filter_release(
      struct list_objects_filter_options *filter_options)
 {
      size_t sub;
+     unsigned int allow_auto_filter = filter_options->allow_auto_filter;

      if (!filter_options)
              return;

@@ -326,6 +355,7 @@ void list_objects_filter_release(
              list_objects_filter_release(&filter_options->sub[sub]);
      free(filter_options->sub);
      list_objects_filter_init(filter_options);
+     filter_options->allow_auto_filter = allow_auto_filter;
 }

Why do we do this extra step to restore the `allow_auto_filter` option
here? Are there any callers that reuse the filter after it has been
released?

As you noticed below, list_objects_filter_release() doesn't just
release resources but actually resets the state. That's because the
filter options are indeed reused during command-line parsing.

In cmd_clone() a single `struct list_objects_filter_options` called
"filter_options" is declared and then pointers to it are passed to a
number of functions. In particular, opt_parse_list_objects_filter()
handles the `--no-filter` case by calling
list_objects_filter_set_no_filter() which calls
list_objects_filter_release().

So yeah, if the user runs something like `git fetch --no-filter
--filter=auto`, then "filter_options" is reused when `--filter=auto`
is processed, so after it has been released.

Also note that the `allow_auto_filter` field is a configuration bit
set by the command (e.g., cmd_fetch) before parsing begins. It
indicates that the command supports the 'auto' mode. It's not data
provided by users, so it doesn't change depending on which filter
related options are passed.

I have added the following to the commit message:

"Also note that the new `allow_auto_filter` flag depends on the command,
not user choices, so it should be reset to the command default when
`struct list_objects_filter_options` instances are reset."

In any case, this function does have clearing semantics as it also knows
to re-init the filter options. So it's somewhat misnamed and really
should be called `list_objects_filter_clear()` according to our coding
guidelines. That's certainly outside the scope of this patch series
though.

Yeah, it can be done separately.