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

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

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