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 2/9] promisor-remote: allow a client to store fields
Date: Wed, 7 Jan 2026 11:05:16 +0100 [thread overview]
Message-ID: <aV4v3JwW0S-c9Dn4@pks.im> (raw)
In-Reply-To: <20251223111113.47473-3-christian.couder@gmail.com>
On Tue, Dec 23, 2025 at 12:11:06PM +0100, Christian Couder wrote:
> A previous commit allowed a server to pass additional fields through
> the "promisor-remote" protocol capability after the "name" and "url"
> fields, specifically the "partialCloneFilter" and "token" fields.
>
> Another previous commit, c213820c51 (promisor-remote: allow a client
> to check fields, 2025-09-08), has made it possible for a client to
> decide if it accepts a promisor remote advertised by a server based
> on these additional fields.
>
> Often though, it would be interesting for the client to just store in
> its configuration files these additional fields passed by the server,
> so that it can use them when needed.
>
> For example if a token is necessary to access a promisor remote, that
> token could be updated frequently only on the server side and then
> passed to all the clients through the "promisor-remote" capability,
> avoiding the need to update it on all the clients manually.
>
> Storing the token on the client side makes sure that the token is
> available when the client needs to access the promisor remotes for a
> lazy fetch.
I guess another use case is that a client performs a fresh clone and
doesn't know anything about the remote's promisors yet, right? In that
case, the client may want to tell git-clone(1) to accept any of the
remote's advertised promisors, store it and then use that promisor's
filter to perform the actual clone.
> In the same way, if it appears that it's better to use a different
> filter to access a promisor remote, it could be helpful if the client
> could automatically use it.
>
> To allow this, let's introduce a new "promisor.storeFields"
> configuration variable.
>
> Like "promisor.checkFields" and "promisor.sendFields", it should
> contain a comma or space separated list of field names. Only the
> "partialCloneFilter" and "token" field names are supported for now.
>
> When a server advertises a promisor remote, for example "foo", along
> with for example "token=XXXXX" to a client, and on the client side
> "promisor.storeFields" contains "token", then the client will store
> XXXXX for the "remote.foo.token" variable in its configuration file
> and reload its configuration so it can immediately use this new
> configuration variable.
>
> A message is emitted on stderr to warn users when the config is
> changed.
>
> Note that even if "promisor.acceptFromServer" is set to "all", a
> promisor remote has to be already configured on the client side for
> some of its config to be changed. In any case no new remote is
> configured and no new URL is stored.
Hm, okay, so that's not yet part of this series. I assume this is going
to be part of a subsequent patch series then?
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 5d8151cedb..8d6d2d7b76 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -403,6 +403,14 @@ static struct string_list *fields_checked(void)
> return initialize_fields_list(&fields_list, &initialized, "promisor.checkFields");
> }
>
> +static struct string_list *fields_stored(void)
> +{
> + static struct string_list fields_list = STRING_LIST_INIT_NODUP;
> + static int initialized;
> +
> + return initialize_fields_list(&fields_list, &initialized, "promisor.storeFields");
> +}
I'm a bit worried about all the function-local state that we're
accumulating in those functions. Wouldn't it be preferable if we instead
had a `struct promisor_remote` that encapsulates the information?
> @@ -692,6 +700,132 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
> return info;
> }
>
> +static bool store_one_field(struct repository *repo, const char *remote_name,
> + const char *field_name, const char *field_key,
> + const char *advertised, const char *current)
> +{
> + if (advertised && (!current || strcmp(current, advertised))) {
> + char *key = xstrfmt("remote.%s.%s", remote_name, field_key);
> +
> + fprintf(stderr, _("Storing new %s from server for remote '%s'.\n"
> + " '%s' -> '%s'\n"),
> + field_name, remote_name,
> + current ? current : "",
> + advertised);
> +
> + repo_config_set_worktree_gently(repo, key, advertised);
Why do we store this information in the current per-worktree config? I'd
expect that this should be stored in the local config.
> + free(key);
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/* Check that a filter is valid by parsing it */
> +static bool valid_filter(const char *filter, const char *remote_name)
> +{
> + struct list_objects_filter_options filter_opts = LIST_OBJECTS_FILTER_INIT;
> + struct strbuf err = STRBUF_INIT;
> + int res = gently_parse_list_objects_filter(&filter_opts, filter, &err);
> +
> + if (res)
> + warning(_("invalid filter '%s' for remote '%s' "
> + "will not be stored: %s"),
> + filter, remote_name, err.buf);
> +
> + list_objects_filter_release(&filter_opts);
> + strbuf_release(&err);
> +
> + return !res;
> +}
> +
> +/* Check that a token doesn't contain any control character */
> +static bool valid_token(const char *token, const char *remote_name)
> +{
> + const char *c = token;
> +
> + for (; *c; c++)
> + if (iscntrl(*c)) {
Makes sense. I was also wondering about whether we want to check for
non-space whitespace characters, like newlines.
> + warning(_("invalid token '%s' for remote '%s' "
> + "will not be stored"),
> + token, remote_name);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +struct store_info {
> + struct repository *repo;
> + struct string_list config_info;
> + bool store_filter;
> + bool store_token;
> +};
> +
> +static struct store_info *new_store_info(struct repository *repo)
This should be called `store_info_new()` according to our coding
guidelines.
> +{
> + struct string_list *fields_to_store = fields_stored();
> + struct store_info *s = xmalloc(sizeof(*s));
> +
> + s->repo = repo;
> +
> + string_list_init_nodup(&s->config_info);
> + promisor_config_info_list(repo, &s->config_info, fields_to_store);
> + string_list_sort(&s->config_info);
> +
> + s->store_filter = !!string_list_lookup(fields_to_store, promisor_field_filter);
> + s->store_token = !!string_list_lookup(fields_to_store, promisor_field_token);
> +
> + return s;
> +}
> +
> +static void free_store_info(struct store_info *s)
Likewise, this would be `store_info_free()`.
> diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
> index 023735d6a8..a726af214a 100755
> --- a/t/t5710-promisor-remote-capability.sh
> +++ b/t/t5710-promisor-remote-capability.sh
> @@ -360,6 +360,55 @@ test_expect_success "clone with promisor.checkFields" '
> check_missing_objects server 1 "$oid"
> '
>
> +test_expect_success "clone with promisor.storeFields=partialCloneFilter" '
> + 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 remote.lop.token "fooXXX" &&
> + git -C server config remote.lop.partialCloneFilter "blob:limit=8k" &&
> +
> + test_config -C server promisor.sendFields "partialCloneFilter, token" &&
> + test_when_finished "rm trace" &&
> +
> + # Clone from server to create a client
> + GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
> + -c remote.lop.promisor=true \
> + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
> + -c remote.lop.url="file://$(pwd)/lop" \
> + -c remote.lop.token="fooYYY" \
> + -c remote.lop.partialCloneFilter="blob:none" \
> + -c promisor.acceptfromserver=All \
> + -c promisor.storeFields=partialcloneFilter \
> + --no-local --filter="blob:limit=5k" server client 2>err &&
Onet thing that's missing in these tests is to verify that a subsequent
git-fetch(1) updates the configuration.
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 [this message]
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
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=aV4v3JwW0S-c9Dn4@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