From: Patrick Steinhardt <ps@pks•im>
To: Christian Couder <christian.couder@gmail•com>
Cc: git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>,
Taylor Blau <me@ttaylorr•com>,
Karthik Nayak <karthik.188@gmail•com>,
Elijah Newren <newren@gmail•com>,
Christian Couder <chriscool@tuxfamily•org>
Subject: Re: [PATCH 6/9] list-objects-filter-options: support 'auto' mode for --filter
Date: Wed, 7 Jan 2026 11:05:31 +0100 [thread overview]
Message-ID: <aV4v6_DFJtraLlPI@pks.im> (raw)
In-Reply-To: <20251223111113.47473-7-christian.couder@gmail.com>
On Tue, Dec 23, 2025 at 12:11:10PM +0100, Christian Couder wrote:
> 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.
> 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
> 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.
> 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.
> @@ -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.
> @@ -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?
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.
Patrick
next prev parent reply other threads:[~2026-01-07 10:05 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-23 11:11 [PATCH 0/9] Implement `promisor.storeFields` and `--filter=auto` Christian Couder
2025-12-23 11:11 ` [PATCH 1/9] promisor-remote: refactor initialising field lists Christian Couder
2025-12-23 11:11 ` [PATCH 2/9] promisor-remote: allow a client to store fields Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt
2026-02-04 10:20 ` Christian Couder
2025-12-23 11:11 ` [PATCH 3/9] clone: make filter_options local to cmd_clone() Christian Couder
2025-12-23 11:11 ` [PATCH 4/9] fetch: make filter_options local to cmd_fetch() Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt
2025-12-23 11:11 ` [PATCH 5/9] doc: fetch: document `--filter=<filter-spec>` option Christian Couder
2025-12-26 13:33 ` Jean-Noël AVILA
2026-02-04 11:19 ` Christian Couder
2025-12-23 11:11 ` [PATCH 6/9] list-objects-filter-options: support 'auto' mode for --filter Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt [this message]
2026-02-04 10:21 ` Christian Couder
2025-12-23 11:11 ` [PATCH 7/9] list-objects-filter-options: implement auto filter resolution Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt
2026-02-04 10:29 ` Christian Couder
2026-02-11 11:48 ` Patrick Steinhardt
2026-02-12 10:07 ` Christian Couder
2025-12-23 11:11 ` [PATCH 8/9] promisor-remote: keep advertised filter in memory Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt
2026-02-04 10:57 ` Christian Couder
2026-02-11 11:48 ` Patrick Steinhardt
2026-02-11 16:59 ` Junio C Hamano
2026-02-12 10:07 ` Christian Couder
2025-12-23 11:11 ` [PATCH 9/9] fetch-pack: wire up and enable auto filter logic Christian Couder
2026-01-07 10:05 ` Patrick Steinhardt
2026-02-04 11:06 ` Christian Couder
2026-02-04 11:08 ` [PATCH v2 0/8] Implement `promisor.storeFields` and `--filter=auto` Christian Couder
2026-02-04 11:08 ` [PATCH v2 1/8] promisor-remote: refactor initialising field lists Christian Couder
2026-02-04 11:08 ` [PATCH v2 2/8] promisor-remote: allow a client to store fields Christian Couder
2026-02-04 11:08 ` [PATCH v2 3/8] clone: make filter_options local to cmd_clone() Christian Couder
2026-02-04 11:08 ` [PATCH v2 4/8] fetch: make filter_options local to cmd_fetch() Christian Couder
2026-02-04 11:08 ` [PATCH v2 5/8] doc: fetch: document `--filter=<filter-spec>` option Christian Couder
2026-02-11 11:48 ` Patrick Steinhardt
2026-02-12 10:06 ` Christian Couder
2026-02-04 11:08 ` [PATCH v2 6/8] list-objects-filter-options: support 'auto' mode for --filter Christian Couder
2026-02-04 11:08 ` [PATCH v2 7/8] promisor-remote: keep advertised filters in memory Christian Couder
2026-02-04 11:08 ` [PATCH v2 8/8] fetch-pack: wire up and enable auto filter logic Christian Couder
2026-02-11 11:48 ` Patrick Steinhardt
2026-02-12 10:07 ` Christian Couder
2026-02-12 10:08 ` [PATCH v3 0/9] Implement `promisor.storeFields` and `--filter=auto` Christian Couder
2026-02-12 10:08 ` [PATCH v3 1/9] promisor-remote: refactor initialising field lists Christian Couder
2026-02-12 10:08 ` [PATCH v3 2/9] promisor-remote: allow a client to store fields Christian Couder
2026-02-12 10:08 ` [PATCH v3 3/9] clone: make filter_options local to cmd_clone() Christian Couder
2026-02-12 10:08 ` [PATCH v3 4/9] fetch: make filter_options local to cmd_fetch() Christian Couder
2026-02-12 10:08 ` [PATCH v3 5/9] doc: fetch: document `--filter=<filter-spec>` option Christian Couder
2026-02-12 10:08 ` [PATCH v3 6/9] list-objects-filter-options: support 'auto' mode for --filter Christian Couder
2026-02-14 2:35 ` Jeff King
2026-02-16 13:26 ` Christian Couder
2026-02-12 10:08 ` [PATCH v3 7/9] promisor-remote: keep advertised filters in memory Christian Couder
2026-02-12 10:08 ` [PATCH v3 8/9] promisor-remote: change promisor_remote_reply()'s signature Christian Couder
2026-02-13 11:25 ` Patrick Steinhardt
2026-02-12 10:08 ` [PATCH v3 9/9] fetch-pack: wire up and enable auto filter logic Christian Couder
2026-02-13 11:26 ` Patrick Steinhardt
2026-02-13 11:26 ` [PATCH v3 0/9] Implement `promisor.storeFields` and `--filter=auto` Patrick Steinhardt
2026-02-16 13:23 ` [PATCH v4 " Christian Couder
2026-02-16 13:23 ` [PATCH v4 1/9] promisor-remote: refactor initialising field lists Christian Couder
2026-02-16 13:23 ` [PATCH v4 2/9] promisor-remote: allow a client to store fields Christian Couder
2026-02-16 13:23 ` [PATCH v4 3/9] clone: make filter_options local to cmd_clone() Christian Couder
2026-02-16 13:23 ` [PATCH v4 4/9] fetch: make filter_options local to cmd_fetch() Christian Couder
2026-02-16 13:23 ` [PATCH v4 5/9] doc: fetch: document `--filter=<filter-spec>` option Christian Couder
2026-02-16 13:23 ` [PATCH v4 6/9] list-objects-filter-options: support 'auto' mode for --filter Christian Couder
2026-02-16 13:23 ` [PATCH v4 7/9] promisor-remote: keep advertised filters in memory Christian Couder
2026-02-16 13:23 ` [PATCH v4 8/9] promisor-remote: change promisor_remote_reply()'s signature Christian Couder
2026-02-16 13:23 ` [PATCH v4 9/9] fetch-pack: wire up and enable auto filter logic Christian Couder
2026-04-27 12:41 ` [PATCH v2 0/8] Auto-configure advertised remotes via URL allowlist Christian Couder
2026-04-27 12:41 ` [PATCH v2 1/8] t5710: simplify 'mkdir X' followed by 'git -C X init' Christian Couder
2026-04-27 12:41 ` [PATCH v2 2/8] urlmatch: change 'allow_globs' arg to bool Christian Couder
2026-04-27 12:41 ` [PATCH v2 3/8] urlmatch: add url_normalize_pattern() helper Christian Couder
2026-04-27 12:41 ` [PATCH v2 4/8] promisor-remote: add 'local_name' to 'struct promisor_info' Christian Couder
2026-05-04 11:46 ` Toon Claes
2026-04-27 12:41 ` [PATCH v2 5/8] promisor-remote: introduce promisor.acceptFromServerUrl Christian Couder
2026-04-27 12:41 ` [PATCH v2 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl Christian Couder
2026-05-08 12:45 ` Toon Claes
2026-05-19 15:24 ` Christian Couder
2026-05-11 13:10 ` Toon Claes
2026-05-19 15:25 ` Christian Couder
2026-04-27 12:41 ` [PATCH v2 7/8] promisor-remote: auto-configure unknown remotes Christian Couder
2026-05-11 13:06 ` Toon Claes
2026-05-19 15:25 ` Christian Couder
2026-04-27 12:41 ` [PATCH v2 8/8] doc: promisor: improve acceptFromServer entry Christian Couder
2026-04-27 13:00 ` [PATCH v2 0/8] Auto-configure advertised remotes via URL allowlist Christian Couder
2026-05-19 15:38 ` [PATCH v3 " Christian Couder
2026-05-19 15:38 ` [PATCH v3 1/8] t5710: simplify 'mkdir X' followed by 'git -C X init' Christian Couder
2026-05-19 15:38 ` [PATCH v3 2/8] urlmatch: change 'allow_globs' arg to bool Christian Couder
2026-05-19 15:38 ` [PATCH v3 3/8] urlmatch: add url_normalize_pattern() helper Christian Couder
2026-05-19 15:38 ` [PATCH v3 4/8] promisor-remote: add 'local_name' to 'struct promisor_info' Christian Couder
2026-05-20 0:12 ` Junio C Hamano
2026-05-27 15:33 ` Christian Couder
2026-05-19 15:38 ` [PATCH v3 5/8] promisor-remote: introduce promisor.acceptFromServerUrl Christian Couder
2026-05-19 15:38 ` [PATCH v3 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl Christian Couder
2026-05-23 15:17 ` Kristoffer Haugsbakk
2026-05-27 15:37 ` Christian Couder
2026-05-19 15:38 ` [PATCH v3 7/8] promisor-remote: auto-configure unknown remotes Christian Couder
2026-05-19 15:38 ` [PATCH v3 8/8] doc: promisor: improve acceptFromServer entry Christian Couder
2026-05-27 14:08 ` [PATCH v4 0/8] Auto-configure advertised remotes via URL allowlist Christian Couder
2026-05-27 14:08 ` [PATCH v4 1/8] t5710: simplify 'mkdir X' followed by 'git -C X init' Christian Couder
2026-05-27 14:08 ` [PATCH v4 2/8] urlmatch: change 'allow_globs' arg to bool Christian Couder
2026-05-27 14:08 ` [PATCH v4 3/8] urlmatch: add url_normalize_pattern() helper Christian Couder
2026-05-27 14:08 ` [PATCH v4 4/8] promisor-remote: add 'local_name' to 'struct promisor_info' Christian Couder
2026-05-27 14:08 ` [PATCH v4 5/8] promisor-remote: introduce promisor.acceptFromServerUrl Christian Couder
2026-05-27 14:08 ` [PATCH v4 6/8] promisor-remote: trust known remotes matching acceptFromServerUrl Christian Couder
2026-05-27 14:08 ` [PATCH v4 7/8] promisor-remote: auto-configure unknown remotes Christian Couder
2026-05-27 14:08 ` [PATCH v4 8/8] doc: promisor: improve acceptFromServer entry Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aV4v6_DFJtraLlPI@pks.im \
--to=ps@pks$(echo .)im \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=newren@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox