Re: [PATCH 2/9] promisor-remote: allow a client to store fields

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

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

quoted

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.

Actually there are two issues with this.

The first one is the security issue with the client adding a new
promisor to its config that I will discuss below.

The second one is the fact that it's better if the filter suggested by
the server is used right away during the initial clone, but you have
to pass a `--filter=<filter-spec>` to the clone option in the first
place when you start the initial clone and the filter suggested by the
server might be different than the one you pass. This is why the
second part of the series implements `--filter=auto`.

quoted

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.

By the way I have removed this in the version 2 I am going to send
soon, as it could be misleading.

quoted

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?

My opinion is that we should indeed work on that in a future separate
series, as it could be very useful in setups where clients trust the
server, like corporate setups. For now I prefer to keep things safe by
default and not make it possible.

quoted

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?

I don't think we have a good standard way to manage information from
the config yet. Some suggestions have been made about using a new
struct for some config options, for example in:

https://lore.kernel.org/git/8899016f-eeef-404b-8da6-ff3a90e81cea@gmail.com/ (local)

and perhaps such a good standard way to manage config information will
result from these efforts, but I think it's too early to be sure.

In the meantime, I don't think it's a good idea to spend time on a
specialized way to do it just for promisor remotes.

quoted

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

Right, repo_config_set_gently() is used now instead.

quoted

+             free(key);
+
+             return true;
+     }

[...]

quoted

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

Fine, `store_info_new()` and `store_info_free()` are now used as you suggest.

quoted

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.

Ok, I have added a test using `git fetch`.

Thanks.