[PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[Christian Couder]

Recently, I sent a 16 patch long series that makes it possible for
promisor remotes advertised through the "promisor-remote" protocol
capability to be auto-configured on the client side via a URL
allowlist configured using a new `promisor.acceptFromServerUrl`
configuration variable:

https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/

I got the suggestion to split the 16 patches series into two smaller
series, starting with a preparatory series. So here is this
preparatory series. It's a mix of mostly small fixes, refactorings and
cleanups.

High level description of the patches
=====================================

 - Patches 1-4/10 are fixes:

   - Patch 1/10 is the most significant. The others are relatively
     small.
     
   - Patch 4/10 is the only new in this series (while all the others
     were in the previous series). It prepares for Patch 5/10.

 - Patches 5-10/10 are refactorings and cleanups:

   - Patches 5-7/10 are relatively small independents cleanups or
     refactorings.

   - Patches 8-9/10 are related refactorings simplifying the data
     structures used in filter_promisor_remote() and
     promisor_remote_reply().

   - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths
     in the test script.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/23848484597

Range-diff
==========

Sorry, no range-diff as I don't think it would be quite useful because
the number and order of commits has changed a lot.

Christian Couder (10):
  promisor-remote: try accepted remotes before others in get_direct()
  promisor-remote: pass config entry to all_fields_match() directly
  promisor-remote: clarify that a remote is ignored
  promisor-remote: reject empty name or URL in advertised remote
  promisor-remote: refactor should_accept_remote() control flow
  promisor-remote: refactor has_control_char()
  promisor-remote: refactor accept_from_server()
  promisor-remote: keep accepted promisor_info structs alive
  promisor-remote: remove the 'accepted' strvec
  t5710: use proper file:// URIs for absolute paths

 Documentation/gitprotocol-v2.adoc     |   4 +
 promisor-remote.c                     | 207 +++++++++++++++-----------
 t/t5710-promisor-remote-capability.sh | 122 ++++++++++++---
 3 files changed, 223 insertions(+), 110 deletions(-)

-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()

[Christian Couder]

When a server advertises promisor remotes and the client accepts some
of them, those remotes carry the server's intent: 'fetch missing
objects preferably from here', and the client agrees with that for the
remotes it accepts.

However promisor_remote_get_direct() actually iterates over all
promisor remotes in list order, which is the order they appear in the
config files (except perhaps for the one appearing in the
`extensions.partialClone` config variable which is tried last).

This means an existing, but not accepted, promisor remote, could be
tried before the accepted ones, which does not reflect the intent of
the agreement between client and server.

If the client doesn't care about what the server suggests, it should
accept nothing and rely on its remotes as they are already configured.

To better reflect the agreement between client and server, let's make
promisor_remote_get_direct() try the accepted promisor remotes before
the non-accepted ones.

Concretely, let's extract a try_promisor_remotes() helper and call it
twice from promisor_remote_get_direct():

- first with an `accepted_only=true` argument to try only the accepted
  remotes,
- then with `accepted_only=false` to fall back to any remaining remote.

Ensuring that accepted remotes are preferred will be even more
important if in the future a mechanism is developed to allow the
client to auto-configure remotes that the server advertises. This will
in particular avoid fetching from the server (which is already
configured as a promisor remote) before trying the auto-configured
remotes, as these new remotes would likely appear at the end of the
config file, and as the server might not appear in the
`extensions.partialClone` config variable.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/gitprotocol-v2.adoc     |  4 ++
 promisor-remote.c                     | 44 ++++++++++++-----
 t/t5710-promisor-remote-capability.sh | 69 +++++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 13 deletions(-)

diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
index f985cb4c47..4fcb1a7bda 100644
--- a/Documentation/gitprotocol-v2.adoc
+++ b/Documentation/gitprotocol-v2.adoc
@@ -848,6 +848,10 @@ advertised, it can reply with "promisor-remote=<pr-names>" where
 where `pr-name` is the urlencoded name of a promisor remote the server
 advertised and the client accepts.
 
+The promisor remotes that the client accepted will be tried before the
+other configured promisor remotes when the client attempts to fetch
+missing objects.
+
 Note that, everywhere in this document, the ';' and ',' characters
 MUST be encoded if they appear in `pr-name` or `field-value`.
 
diff --git a/promisor-remote.c b/promisor-remote.c
index 96fa215b06..7ce7d22f95 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
+static int try_promisor_remotes(struct repository *repo,
+				struct object_id **remaining_oids,
+				int *remaining_nr, int *to_free,
+				bool accepted_only)
+{
+	struct promisor_remote *r = repo->promisor_remote_config->promisors;
+
+	for (; r; r = r->next) {
+		if (accepted_only != r->accepted)
+			continue;
+		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
+			if (*remaining_nr == 1)
+				continue;
+			*remaining_nr = remove_fetched_oids(repo, remaining_oids,
+							    *remaining_nr, *to_free);
+			if (*remaining_nr) {
+				*to_free = 1;
+				continue;
+			}
+		}
+		return 1; /* all fetched */
+	}
+	return 0;
+}
+
 void promisor_remote_get_direct(struct repository *repo,
 				const struct object_id *oids,
 				int oid_nr)
 {
-	struct promisor_remote *r;
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
@@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo,
 
 	promisor_remote_init(repo);
 
-	for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
-		if (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) {
-			if (remaining_nr == 1)
-				continue;
-			remaining_nr = remove_fetched_oids(repo, &remaining_oids,
-							 remaining_nr, to_free);
-			if (remaining_nr) {
-				to_free = 1;
-				continue;
-			}
-		}
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
+				 &to_free, true))
+		goto all_fetched;
+	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
+				 &to_free, false))
 		goto all_fetched;
-	}
 
 	for (i = 0; i < remaining_nr; i++) {
 		if (is_promisor_object(repo, &remaining_oids[i]))
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 357822c01a..bf0eed9f10 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -166,6 +166,75 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	check_missing_objects server 1 "$oid"
 '
 
+test_expect_success "clone with two promisors but only one advertised" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client unused_lop" &&
+
+	# Create a promisor that will be configured but not be used
+	git init --bare unused_lop &&
+
+	# Clone from server to create a client
+	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
+		-c remote.unused_lop.promisor=true \
+		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
+		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
+		-c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="file://$(pwd)/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client &&
+
+	# Check that "unused_lop" appears before "lop" in the config
+	printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
+	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
+	test_cmp expect actual &&
+
+	# Check that "lop" was tried
+	test_grep " fetch lop " trace &&
+	# Check that "unused_lop" was not contacted
+	# This means "lop", the accepted promisor, was tried first
+	test_grep ! " fetch unused_lop " trace &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "init + fetch two promisors but only one advertised" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client unused_lop" &&
+
+	# Create a promisor that will be configured but not be used
+	git init --bare unused_lop &&
+
+	mkdir client &&
+	git -C client init &&
+	git -C client config remote.unused_lop.promisor true &&
+	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
+	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
+	git -C client config remote.lop.promisor true &&
+	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
+	git -C client config remote.lop.url "file://$(pwd)/lop" &&
+	git -C client config remote.server.url "file://$(pwd)/server" &&
+	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
+	git -C client config promisor.acceptfromserver All &&
+
+	# Check that "unused_lop" appears before "lop" in the config
+	printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect &&
+	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
+	test_cmp expect actual &&
+
+	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
+
+	# Check that "lop" was tried
+	test_grep " fetch lop " trace &&
+	# Check that "unused_lop" was not contacted
+	# This means "lop", the accepted promisor, was tried first
+	test_grep ! " fetch unused_lop " trace &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.53.0.765.g57b94de1f0.dirty

Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()

[Patrick Steinhardt]

On Thu, Apr 02, 2026 at 09:06:04AM +0200, Christian Couder wrote:
> diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
> index f985cb4c47..4fcb1a7bda 100644
> --- a/Documentation/gitprotocol-v2.adoc
> +++ b/Documentation/gitprotocol-v2.adoc
> @@ -848,6 +848,10 @@ advertised, it can reply with "promisor-remote=<pr-names>" where
>  where `pr-name` is the urlencoded name of a promisor remote the server
>  advertised and the client accepts.
>  
> +The promisor remotes that the client accepted will be tried before the
> +other configured promisor remotes when the client attempts to fetch
> +missing objects.
> +
>  Note that, everywhere in this document, the ';' and ',' characters
>  MUST be encoded if they appear in `pr-name` or `field-value`.

Makes sense, thanks for adding this blurb.

> diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
> index 357822c01a..bf0eed9f10 100755
> --- a/t/t5710-promisor-remote-capability.sh
> +++ b/t/t5710-promisor-remote-capability.sh
> @@ -166,6 +166,75 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
>  	check_missing_objects server 1 "$oid"
>  '
>  
> +test_expect_success "clone with two promisors but only one advertised" '
> +	git -C server config promisor.advertise true &&
> +	test_when_finished "rm -rf client unused_lop" &&
> +
> +	# Create a promisor that will be configured but not be used
> +	git init --bare unused_lop &&
> +
> +	# Clone from server to create a client
> +	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
> +		-c remote.unused_lop.promisor=true \
> +		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
> +		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
> +		-c remote.lop.promisor=true \
> +		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
> +		-c remote.lop.url="file://$(pwd)/lop" \
> +		-c promisor.acceptfromserver=All \
> +		--no-local --filter="blob:limit=5k" server client &&
> +
> +	# Check that "unused_lop" appears before "lop" in the config
> +	printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
> +	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
> +	test_cmp expect actual &&
> +
> +	# Check that "lop" was tried
> +	test_grep " fetch lop " trace &&
> +	# Check that "unused_lop" was not contacted
> +	# This means "lop", the accepted promisor, was tried first
> +	test_grep ! " fetch unused_lop " trace &&
> +
> +	# Check that the largest object is still missing on the server
> +	check_missing_objects server 1 "$oid"
> +'
> +
> +test_expect_success "init + fetch two promisors but only one advertised" '
> +	git -C server config promisor.advertise true &&
> +	test_when_finished "rm -rf client unused_lop" &&
> +
> +	# Create a promisor that will be configured but not be used
> +	git init --bare unused_lop &&
> +
> +	mkdir client &&
> +	git -C client init &&

Tiniest nit, not worth rerolling over: this could just be `git init client`.

Patrick

Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()

[Christian Couder]

On Thu, Apr 2, 2026 at 9:46 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Thu, Apr 02, 2026 at 09:06:04AM +0200, Christian Couder wrote:

> > +test_expect_success "init + fetch two promisors but only one advertised" '
> > +     git -C server config promisor.advertise true &&
> > +     test_when_finished "rm -rf client unused_lop" &&
> > +
> > +     # Create a promisor that will be configured but not be used
> > +     git init --bare unused_lop &&
> > +
> > +     mkdir client &&
> > +     git -C client init &&
>
> Tiniest nit, not worth rerolling over: this could just be `git init client`.

I copied this from another test, and I didn't think it was worth it to
add a preparatory patch just to fix this in the other test, so I left
it like this.

It could be a microproject idea to clean things like this in all the
test scripts.

Thanks.

Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> It could be a microproject idea to clean things like this in all the
> test scripts.

Let's not deliberately add extra technical debt.  Preparatory
clean-up is very good.  Even without it, not adding known to be bad
invocation is better than "following the pattern".

Microproject materials are not free, as it still requires review and
application costs.

Thanks.

[PATCH 02/10] promisor-remote: pass config entry to all_fields_match() directly

[Christian Couder]

The `in_list == 0` path of all_fields_match() looks up the remote in
`config_info` by `advertised->name` repeatedly, even though every
caller in should_accept_remote() has already performed this
lookup and holds the result in `p`.

To avoid this useless work, let's replace the `int in_list`
parameter with a `struct promisor_info *config_entry` pointer:

 - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as
   the old `in_list == 1` path did.

 - When non-NULL: match against that single config entry directly,
   avoiding the redundant string_list_lookup() call.

This removes the hidden dependency on `advertised->name` inside
all_fields_match(), which would be wrong if in the future
auto-configured remotes are implemented, as the local config name may
differ from the server's advertised name.

While at it, let's also add a comment before all_fields_match() and
match_field_against_config() to help understand how things work and
help avoid similar issues.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 7ce7d22f95..6c935f855a 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -575,6 +575,12 @@ enum accept_promisor {
 	ACCEPT_ALL
 };
 
+/*
+ * Check if a specific field and its advertised value match the local
+ * configuration of a given promisor remote.
+ *
+ * Returns 1 if they match, 0 otherwise.
+ */
 static int match_field_against_config(const char *field, const char *value,
 				      struct promisor_info *config_info)
 {
@@ -586,9 +592,18 @@ static int match_field_against_config(const char *field, const char *value,
 	return 0;
 }
 
+/*
+ * Check that the advertised fields match the local configuration.
+ *
+ * When 'config_entry' is NULL (ACCEPT_ALL mode), every checked field
+ * must match at least one remote in 'config_info'.
+ *
+ * When 'config_entry' points to a specific remote's config, the
+ * checked fields are compared against that single remote only.
+ */
 static int all_fields_match(struct promisor_info *advertised,
 			    struct string_list *config_info,
-			    int in_list)
+			    struct promisor_info *config_entry)
 {
 	struct string_list *fields = fields_checked();
 	struct string_list_item *item_checked;
@@ -597,7 +612,6 @@ static int all_fields_match(struct promisor_info *advertised,
 		int match = 0;
 		const char *field = item_checked->string;
 		const char *value = NULL;
-		struct string_list_item *item;
 
 		if (!strcasecmp(field, promisor_field_filter))
 			value = advertised->filter;
@@ -607,7 +621,11 @@ static int all_fields_match(struct promisor_info *advertised,
 		if (!value)
 			return 0;
 
-		if (in_list) {
+		if (config_entry) {
+			match = match_field_against_config(field, value,
+							   config_entry);
+		} else {
+			struct string_list_item *item;
 			for_each_string_list_item(item, config_info) {
 				struct promisor_info *p = item->util;
 				if (match_field_against_config(field, value, p)) {
@@ -615,12 +633,6 @@ static int all_fields_match(struct promisor_info *advertised,
 					break;
 				}
 			}
-		} else {
-			item = string_list_lookup(config_info, advertised->name);
-			if (item) {
-				struct promisor_info *p = item->util;
-				match = match_field_against_config(field, value, p);
-			}
 		}
 
 		if (!match)
@@ -640,7 +652,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	const char *remote_url = advertised->url;
 
 	if (accept == ACCEPT_ALL)
-		return all_fields_match(advertised, config_info, 1);
+		return all_fields_match(advertised, config_info, NULL);
 
 	/* Get config info for that promisor remote */
 	item = string_list_lookup(config_info, remote_name);
@@ -652,7 +664,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	p = item->util;
 
 	if (accept == ACCEPT_KNOWN_NAME)
-		return all_fields_match(advertised, config_info, 0);
+		return all_fields_match(advertised, config_info, p);
 
 	if (accept != ACCEPT_KNOWN_URL)
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
@@ -663,7 +675,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	}
 
 	if (!strcmp(p->url, remote_url))
-		return all_fields_match(advertised, config_info, 0);
+		return all_fields_match(advertised, config_info, p);
 
 	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
 		remote_name, p->url, remote_url);
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 03/10] promisor-remote: clarify that a remote is ignored

[Christian Couder]

In should_accept_remote() and parse_one_advertised_remote(), when a
remote is ignored, we tell users why it is ignored in a warning, but we
don't tell them that the remote is actually ignored.

Let's clarify that, so users have a better idea of what's actually
happening.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 6c935f855a..8e062ec160 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
 
 	if (!remote_url || !*remote_url) {
-		warning(_("no or empty URL advertised for remote '%s'"), remote_name);
+		warning(_("no or empty URL advertised for remote '%s', "
+			  "ignoring this remote"), remote_name);
 		return 0;
 	}
 
 	if (!strcmp(p->url, remote_url))
 		return all_fields_match(advertised, config_info, p);
 
-	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
-		remote_name, p->url, remote_url);
+	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
+		  "ignoring this remote"), remote_name, p->url, remote_url);
 
 	return 0;
 }
@@ -722,8 +723,8 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
 	string_list_clear(&elem_list, 0);
 
 	if (!info->name || !info->url) {
-		warning(_("server advertised a promisor remote without a name or URL: %s"),
-			remote_info);
+		warning(_("server advertised a promisor remote without a name or URL: '%s', "
+			  "ignoring this remote"), remote_info);
 		promisor_info_free(info);
 		return NULL;
 	}
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 04/10] promisor-remote: reject empty name or URL in advertised remote

[Christian Couder]

In parse_one_advertised_remote(), we check for a NULL remote name and
remote URL, but not for empty ones. An empty URL seems possible as
url_percent_decode("") doesn't return NULL.

In promisor_config_info_list(), we ignore remotes with empty URLs, so a
Git server should not advertise remotes with empty URLs. It's possible
that a buggy or malicious server would do it though.

So let's tighten the check in parse_one_advertised_remote() to also
reject empty strings at parse time.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8e062ec160..8322349ae8 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
 
 	string_list_clear(&elem_list, 0);
 
-	if (!info->name || !info->url) {
+	if (!info->name || !*info->name || !info->url || !*info->url) {
 		warning(_("server advertised a promisor remote without a name or URL: '%s', "
 			  "ignoring this remote"), remote_info);
 		promisor_info_free(info);
-- 
2.53.0.765.g57b94de1f0.dirty

Re: [PATCH 04/10] promisor-remote: reject empty name or URL in advertised remote

[Patrick Steinhardt]

On Thu, Apr 02, 2026 at 09:06:07AM +0200, Christian Couder wrote:
> In parse_one_advertised_remote(), we check for a NULL remote name and
> remote URL, but not for empty ones. An empty URL seems possible as
> url_percent_decode("") doesn't return NULL.
> 
> In promisor_config_info_list(), we ignore remotes with empty URLs, so a
> Git server should not advertise remotes with empty URLs. It's possible
> that a buggy or malicious server would do it though.
> 
> So let's tighten the check in parse_one_advertised_remote() to also
> reject empty strings at parse time.

Makes sense.

Patrick

[PATCH 05/10] promisor-remote: refactor should_accept_remote() control flow

[Christian Couder]

A previous commit made sure we now reject empty URLs early at parse
time. This makes the existing warning() in case a remote URL is NULL
or empty very unlikely to be useful.

In future work, we also plan to add URL-based acceptance logic into
should_accept_remote().

To adapt to previous changes and prepare for upcoming changes, let's
restructure the control flow in should_accept_remote().

Concretely, let's:

 - Replace the warning() in case of an empty URL with a BUG(), as a
   previous commit made sure empty URLs are rejected early at parse
   time.

 - Move that modified empty-URL check to the very top of the function,
   so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is
   covered.

 - Invert the URL comparison: instead of returning on match and
   warning on mismatch, return early on mismatch and let the match
   case fall through. This opens a single exit path at the bottom of
   the function for future commits to extend.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8322349ae8..5860a3d3f3 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -651,6 +651,11 @@ static int should_accept_remote(enum accept_promisor accept,
 	const char *remote_name = advertised->name;
 	const char *remote_url = advertised->url;
 
+	if (!remote_url || !*remote_url)
+		BUG("no or empty URL advertised for remote '%s'; "
+		    "this remote should have been rejected earlier",
+		    remote_name);
+
 	if (accept == ACCEPT_ALL)
 		return all_fields_match(advertised, config_info, NULL);
 
@@ -669,19 +674,14 @@ static int should_accept_remote(enum accept_promisor accept,
 	if (accept != ACCEPT_KNOWN_URL)
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
 
-	if (!remote_url || !*remote_url) {
-		warning(_("no or empty URL advertised for remote '%s', "
-			  "ignoring this remote"), remote_name);
+	if (strcmp(p->url, remote_url)) {
+		warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
+			  "ignoring this remote"),
+			remote_name, p->url, remote_url);
 		return 0;
 	}
 
-	if (!strcmp(p->url, remote_url))
-		return all_fields_match(advertised, config_info, p);
-
-	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
-		  "ignoring this remote"), remote_name, p->url, remote_url);
-
-	return 0;
+	return all_fields_match(advertised, config_info, p);
 }
 
 static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 06/10] promisor-remote: refactor has_control_char()

[Christian Couder]

In a future commit we are going to check if some strings contain
control characters, so let's refactor the logic to do that in a new
has_control_char() helper function.

It cleans up the code a bit anyway.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 5860a3d3f3..d60518f19c 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -642,6 +642,14 @@ static int all_fields_match(struct promisor_info *advertised,
 	return 1;
 }
 
+static bool has_control_char(const char *s)
+{
+	for (const char *c = s; *c; c++)
+		if (iscntrl(*c))
+			return true;
+	return false;
+}
+
 static int should_accept_remote(enum accept_promisor accept,
 				struct promisor_info *advertised,
 				struct string_list *config_info)
@@ -772,18 +780,14 @@ static bool valid_filter(const char *filter, const char *remote_name)
 	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)) {
-			warning(_("invalid token '%s' for remote '%s' "
-				  "will not be stored"),
-				token, remote_name);
-			return false;
-		}
+	if (has_control_char(token)) {
+		warning(_("invalid token '%s' for remote '%s' "
+			  "will not be stored"),
+			token, remote_name);
+		return false;
+	}
 
 	return true;
 }
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 07/10] promisor-remote: refactor accept_from_server()

[Christian Couder]

In future commits, we are going to add more logic to
filter_promisor_remote() which is already doing a lot of things.

Let's alleviate that by moving the logic that checks and validates the
value of the `promisor.acceptFromServer` config variable into its own
accept_from_server() helper function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index d60518f19c..8d80ef6040 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -862,20 +862,12 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised,
 	return reload_config;
 }
 
-static void filter_promisor_remote(struct repository *repo,
-				   struct strvec *accepted,
-				   const char *info)
+static enum accept_promisor accept_from_server(struct repository *repo)
 {
 	const char *accept_str;
 	enum accept_promisor accept = ACCEPT_NONE;
-	struct string_list config_info = STRING_LIST_INIT_NODUP;
-	struct string_list remote_info = STRING_LIST_INIT_DUP;
-	struct store_info *store_info = NULL;
-	struct string_list_item *item;
-	bool reload_config = false;
-	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
 
-	if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) {
+	if (!repo_config_get_string_tmp(repo, "promisor.acceptfromserver", &accept_str)) {
 		if (!*accept_str || !strcasecmp("None", accept_str))
 			accept = ACCEPT_NONE;
 		else if (!strcasecmp("KnownUrl", accept_str))
@@ -889,6 +881,21 @@ static void filter_promisor_remote(struct repository *repo,
 				accept_str, "promisor.acceptfromserver");
 	}
 
+	return accept;
+}
+
+static void filter_promisor_remote(struct repository *repo,
+				   struct strvec *accepted,
+				   const char *info)
+{
+	struct string_list config_info = STRING_LIST_INIT_NODUP;
+	struct string_list remote_info = STRING_LIST_INIT_DUP;
+	struct store_info *store_info = NULL;
+	struct string_list_item *item;
+	bool reload_config = false;
+	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
+	enum accept_promisor accept = accept_from_server(repo);
+
 	if (accept == ACCEPT_NONE)
 		return;
 
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 08/10] promisor-remote: keep accepted promisor_info structs alive

[Christian Couder]

In filter_promisor_remote(), the instances of `struct promisor_info`
for accepted remotes are dismantled into separate parallel data
structures (the 'accepted' strvec for server names, and
'accepted_filters' for filter strings) and then immediately freed.

Instead, let's keep these instances on an 'accepted_remotes' list.

This way the post-loop phase can iterate a single list to build the
protocol reply, apply advertised filters, and mark remotes as
accepted, rather than iterating three separate structures.

This refactoring also prepares for a future commit that will add a
'local_name' member to 'struct promisor_info'. Since struct instances
stay alive, downstream code will be able to simply read both names
from them rather than needing yet another parallel strvec.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8d80ef6040..74e65e9dd0 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -890,10 +890,10 @@ static void filter_promisor_remote(struct repository *repo,
 {
 	struct string_list config_info = STRING_LIST_INIT_NODUP;
 	struct string_list remote_info = STRING_LIST_INIT_DUP;
+	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 	struct store_info *store_info = NULL;
 	struct string_list_item *item;
 	bool reload_config = false;
-	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
 	enum accept_promisor accept = accept_from_server(repo);
 
 	if (accept == ACCEPT_NONE)
@@ -922,17 +922,10 @@ static void filter_promisor_remote(struct repository *repo,
 			if (promisor_store_advertised_fields(advertised, store_info))
 				reload_config = true;
 
-			strvec_push(accepted, advertised->name);
-
-			/* Capture advertised filters for accepted remotes */
-			if (advertised->filter) {
-				struct string_list_item *i;
-				i = string_list_append(&accepted_filters, advertised->name);
-				i->util = xstrdup(advertised->filter);
-			}
+			string_list_append(&accepted_remotes, advertised->name)->util = advertised;
+		} else {
+			promisor_info_free(advertised);
 		}
-
-		promisor_info_free(advertised);
 	}
 
 	promisor_info_list_clear(&config_info);
@@ -942,24 +935,23 @@ static void filter_promisor_remote(struct repository *repo,
 	if (reload_config)
 		repo_promisor_remote_reinit(repo);
 
-	/* Apply accepted remote filters to the stable repo state */
-	for_each_string_list_item(item, &accepted_filters) {
-		struct promisor_remote *r = repo_promisor_remote_find(repo, item->string);
-		if (r) {
-			free(r->advertised_filter);
-			r->advertised_filter = item->util;
-			item->util = NULL;
-		}
-	}
+	/* Apply accepted remotes to the stable repo state */
+	for_each_string_list_item(item, &accepted_remotes) {
+		struct promisor_info *info = item->util;
+		struct promisor_remote *r = repo_promisor_remote_find(repo, info->name);
 
-	string_list_clear(&accepted_filters, 1);
+		strvec_push(accepted, info->name);
 
-	/* Mark the remotes as accepted in the repository state */
-	for (size_t i = 0; i < accepted->nr; i++) {
-		struct promisor_remote *r = repo_promisor_remote_find(repo, accepted->v[i]);
-		if (r)
+		if (r) {
 			r->accepted = 1;
+			if (info->filter) {
+				free(r->advertised_filter);
+				r->advertised_filter = xstrdup(info->filter);
+			}
+		}
 	}
+
+	promisor_info_list_clear(&accepted_remotes);
 }
 
 void promisor_remote_reply(const char *info, char **accepted_out)
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 09/10] promisor-remote: remove the 'accepted' strvec

[Christian Couder]

In a previous commit, filter_promisor_remote() was refactored to keep
accepted 'struct promisor_info' instances alive instead of dismantling
them into separate parallel data structures.

Let's go one step further and replace the 'struct strvec *accepted'
argument passed to filter_promisor_remote() with a
'struct string_list *accepted_remotes' argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 74e65e9dd0..38fa050542 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -885,12 +885,11 @@ static enum accept_promisor accept_from_server(struct repository *repo)
 }
 
 static void filter_promisor_remote(struct repository *repo,
-				   struct strvec *accepted,
+				   struct string_list *accepted_remotes,
 				   const char *info)
 {
 	struct string_list config_info = STRING_LIST_INIT_NODUP;
 	struct string_list remote_info = STRING_LIST_INIT_DUP;
-	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 	struct store_info *store_info = NULL;
 	struct string_list_item *item;
 	bool reload_config = false;
@@ -922,7 +921,7 @@ static void filter_promisor_remote(struct repository *repo,
 			if (promisor_store_advertised_fields(advertised, store_info))
 				reload_config = true;
 
-			string_list_append(&accepted_remotes, advertised->name)->util = advertised;
+			string_list_append(accepted_remotes, advertised->name)->util = advertised;
 		} else {
 			promisor_info_free(advertised);
 		}
@@ -936,12 +935,10 @@ static void filter_promisor_remote(struct repository *repo,
 		repo_promisor_remote_reinit(repo);
 
 	/* Apply accepted remotes to the stable repo state */
-	for_each_string_list_item(item, &accepted_remotes) {
+	for_each_string_list_item(item, accepted_remotes) {
 		struct promisor_info *info = item->util;
 		struct promisor_remote *r = repo_promisor_remote_find(repo, info->name);
 
-		strvec_push(accepted, info->name);
-
 		if (r) {
 			r->accepted = 1;
 			if (info->filter) {
@@ -950,23 +947,23 @@ static void filter_promisor_remote(struct repository *repo,
 			}
 		}
 	}
-
-	promisor_info_list_clear(&accepted_remotes);
 }
 
 void promisor_remote_reply(const char *info, char **accepted_out)
 {
-	struct strvec accepted = STRVEC_INIT;
+	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 
-	filter_promisor_remote(the_repository, &accepted, info);
+	filter_promisor_remote(the_repository, &accepted_remotes, info);
 
 	if (accepted_out) {
-		if (accepted.nr) {
+		if (accepted_remotes.nr) {
 			struct strbuf reply = STRBUF_INIT;
-			for (size_t i = 0; i < accepted.nr; i++) {
-				if (i)
+			struct string_list_item *item;
+
+			for_each_string_list_item(item, &accepted_remotes) {
+				if (reply.len)
 					strbuf_addch(&reply, ';');
-				strbuf_addstr_urlencode(&reply, accepted.v[i], allow_unsanitized);
+				strbuf_addstr_urlencode(&reply, item->string, allow_unsanitized);
 			}
 			*accepted_out = strbuf_detach(&reply, NULL);
 		} else {
@@ -974,7 +971,7 @@ void promisor_remote_reply(const char *info, char **accepted_out)
 		}
 	}
 
-	strvec_clear(&accepted);
+	promisor_info_list_clear(&accepted_remotes);
 }
 
 void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes)
-- 
2.53.0.765.g57b94de1f0.dirty

[PATCH 10/10] t5710: use proper file:// URIs for absolute paths

[Christian Couder]

In t5710, we frequently construct local file URIs using `file://$(pwd)`.
On Unix-like systems, $(pwd) returns an absolute path starting with a
slash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an
empty host (`file:///tmp/repo`).

However, on Windows, $(pwd) returns a path starting with a drive
letter (e.g., `D:/a/repo`). This results in a 2-slash URI
(`file://D:/a/repo`). Standard URI parsers misinterpret this format,
treating `D:` as the host rather than part of the absolute path.

This is to be expected because RFC 8089 says that the `//` prefix with
an empty local host must be followed by an absolute path starting with
a slash.

While this hasn't broken the existing tests (because the old
`promisor.acceptFromServer` logic relies entirely on strict `strcmp()`
without normalizing the URLs), it will break future commits that pass
these URLs through `url_normalize()` or similar functions.

To future-proof the tests and ensure cross-platform URI compliance,
let's introduce a $PWD_URL helper that explicitly guarantees a leading
slash for the path component, ensuring valid 3-slash `file:///` URIs on
all operating systems.

While at it, let's also introduce $ENCODED_PWD_URL to handle spaces in
directory paths (which is needed for URL glob pattern matching).

Then let's replace all instances of `file://$(pwd)` with $PWD_URL across
the test script, and let's simplify the `ENCODED_URL` constructions in
the `sendFields` and `checkFields` tests to use $ENCODED_PWD_URL
directly.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t5710-promisor-remote-capability.sh | 55 ++++++++++++++++-----------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index bf0eed9f10..3eca6601ca 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -76,6 +76,17 @@ copy_to_lop () {
 	cp "$path" "$path2"
 }
 
+# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn
+# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.
+# On Unix, the path already starts with '/', so this is a no-op.
+pwd_path=$(pwd)
+case "$pwd_path" in
+[a-zA-Z]:*) pwd_path="/$pwd_path" ;;
+esac
+PWD_URL="file://$pwd_path"
+# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.
+ENCODED_PWD_URL="file://$(echo "$pwd_path" | sed "s/ /%20/g")"
+
 test_expect_success "setup for testing promisor remote advertisement" '
 	# Create another bare repo called "lop" (for Large Object Promisor)
 	git init --bare lop &&
@@ -88,7 +99,7 @@ test_expect_success "setup for testing promisor remote advertisement" '
 	initialize_server 1 "$oid" &&
 
 	# Configure lop as promisor remote for server
-	git -C server remote add lop "file://$(pwd)/lop" &&
+	git -C server remote add lop "$PWD_URL/lop" &&
 	git -C server config remote.lop.promisor true &&
 
 	git -C lop config uploadpack.allowFilter true &&
@@ -104,7 +115,7 @@ test_expect_success "clone with promisor.advertise set to 'true'" '
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -119,7 +130,7 @@ test_expect_success "clone with promisor.advertise set to 'false'" '
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -137,7 +148,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=None \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -156,8 +167,8 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C client init &&
 	git -C client config remote.lop.promisor true &&
 	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
-	git -C client config remote.lop.url "file://$(pwd)/lop" &&
-	git -C client config remote.server.url "file://$(pwd)/server" &&
+	git -C client config remote.lop.url "$PWD_URL/lop" &&
+	git -C client config remote.server.url "$PWD_URL/server" &&
 	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
 	git -C client config promisor.acceptfromserver All &&
 	GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
@@ -242,7 +253,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=KnownName \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -257,7 +268,7 @@ test_expect_success "clone with 'KnownName' and different remote names" '
 	# Clone from server to create a client
 	GIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \
 		-c remote.serverTwo.fetch="+refs/heads/*:refs/remotes/lop/*" \
-		-c remote.serverTwo.url="file://$(pwd)/lop" \
+		-c remote.serverTwo.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=KnownName \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -294,7 +305,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownUrl'" '
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -311,7 +322,7 @@ test_expect_success "clone with 'KnownUrl' and different remote urls" '
 	# Clone from server to create a client
 	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)/serverTwo" \
+		-c remote.lop.url="$PWD_URL/serverTwo" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -326,7 +337,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server"
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
 
-	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
+	test_when_finished "git -C server config set remote.lop.url \"$PWD_URL/lop\"" &&
 	git -C server config unset remote.lop.url &&
 
 	# Clone from server to create a client
@@ -335,7 +346,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server"
 	# missing, so the remote name will be used instead which will fail.
 	test_must_fail env 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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -347,7 +358,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
 
-	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
+	test_when_finished "git -C server config set remote.lop.url \"$PWD_URL/lop\"" &&
 	git -C server config set remote.lop.url "" &&
 
 	# Clone from server to create a client
@@ -356,7 +367,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
 	# so the remote name will be used instead which will fail.
 	test_must_fail env 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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -380,13 +391,12 @@ test_expect_success "clone with promisor.sendFields" '
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
 	# Check that fields are properly transmitted
-	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
-	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+	PR1="name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none" &&
 	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
 	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
 	test_grep "clone> promisor-remote=lop;otherLop" trace &&
@@ -411,15 +421,14 @@ test_expect_success "clone with promisor.checkFields" '
 	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.url="$PWD_URL/lop" \
 		-c remote.lop.partialCloneFilter="blob:none" \
 		-c promisor.acceptfromserver=All \
 		-c promisor.checkFields=partialcloneFilter \
 		--no-local --filter="blob:limit=5k" server client &&
 
 	# Check that fields are properly transmitted
-	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
-	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+	PR1="name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none" &&
 	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
 	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
 	test_grep "clone> promisor-remote=lop" trace &&
@@ -449,7 +458,7 @@ test_expect_success "clone with promisor.storeFields=partialCloneFilter" '
 	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.url="$PWD_URL/lop" \
 		-c remote.lop.token="fooYYY" \
 		-c remote.lop.partialCloneFilter="blob:none" \
 		-c promisor.acceptfromserver=All \
@@ -501,7 +510,7 @@ test_expect_success "clone and fetch with --filter=auto" '
 
 	GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
 		-c remote.lop.promisor=true \
-		-c remote.lop.url="file://$(pwd)/lop" \
+		-c remote.lop.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter=auto server client 2>err &&
 
@@ -558,7 +567,7 @@ test_expect_success "clone with promisor.advertise set to 'true' but don't delet
 	# Clone from server to create a client
 	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.url="$PWD_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
-- 
2.53.0.765.g57b94de1f0.dirty

Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> +# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn
> +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.
> +# On Unix, the path already starts with '/', so this is a no-op.
> +pwd_path=$(pwd)
> +case "$pwd_path" in
> +[a-zA-Z]:*) pwd_path="/$pwd_path" ;;
> +esac
> +PWD_URL="file://$pwd_path"
> +# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.
> +ENCODED_PWD_URL="file://$(echo "$pwd_path" | sed "s/ /%20/g")"

Two comments.

 - I was a bit surprised that these are not given as functions but
   as variables, as a caller that chdirs around in the trash
   directory would want a URL that points at its current working
   directory (the expectation is from "pwd" in the name PWD_URL).
   But a variable based interface "Here is the URL that corresponds
   to the trash directory" is OK and probably easier to use than
   "give me the URL corresponding to my current working directory",
   simply because it allows a caller to append some string to it to
   come up with a URL for any subdirectory on its own without
   actually going there.  But in that case, the name PWD_URL would
   become misleading, as it is PWD as of the moment the variable
   gets defined, and the true meaning of the variable is not "URL
   for the current directory", but "URL for the trash directory" is
   more usable definition.

 - Is it sufficient to only special case SP?  My repository may be
   $HOME/w/git.git, for example, and the trash repository may be
   "$HOME/w/git.git/t/trash directory.t5710/", so you need to cope
   with SP between "trash" and "directory" the test framework adds
   (to force you to be careful), but the test framework does not
   control what can be in the leading $HOME part.

Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths

[Christian Couder]

On Thu, Apr 2, 2026 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > +# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn
> > +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.
> > +# On Unix, the path already starts with '/', so this is a no-op.
> > +pwd_path=$(pwd)
> > +case "$pwd_path" in
> > +[a-zA-Z]:*) pwd_path="/$pwd_path" ;;
> > +esac
> > +PWD_URL="file://$pwd_path"
> > +# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.
> > +ENCODED_PWD_URL="file://$(echo "$pwd_path" | sed "s/ /%20/g")"
>
> Two comments.
>
>  - I was a bit surprised that these are not given as functions but
>    as variables, as a caller that chdirs around in the trash
>    directory would want a URL that points at its current working
>    directory (the expectation is from "pwd" in the name PWD_URL).
>    But a variable based interface "Here is the URL that corresponds
>    to the trash directory" is OK and probably easier to use than
>    "give me the URL corresponding to my current working directory",
>    simply because it allows a caller to append some string to it to
>    come up with a URL for any subdirectory on its own without
>    actually going there.  But in that case, the name PWD_URL would
>    become misleading, as it is PWD as of the moment the variable
>    gets defined, and the true meaning of the variable is not "URL
>    for the current directory", but "URL for the trash directory" is
>    more usable definition.

Right. I will use something like the following then:

TRASH_DIRECTORY_URL="file://$pwd_path"
ENCODED_TRASH_DIRECTORY_URL="file://$encoded_path"

>  - Is it sufficient to only special case SP?  My repository may be
>    $HOME/w/git.git, for example, and the trash repository may be
>    "$HOME/w/git.git/t/trash directory.t5710/", so you need to cope
>    with SP between "trash" and "directory" the test framework adds
>    (to force you to be careful), but the test framework does not
>    control what can be in the leading $HOME part.

I think it's quite unlikely for users to have strange characters in
$HOME, but OK let's be cautious and encode a larger set than just
space characters. And to be extra safe, let's also skip all the tests
if we detect a special character not in the larger set.

So something like:

# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),
and those percent-encoded below (% space = ; ,)
case "$pwd_path" in
*[!a-zA-Z0-9_.~/:%\ =,;-]*)
        skip_all="PWD contains unsupported special characters"
        test_done
        ;;
esac

encoded_path=$(printf "%s" "$pwd_path" | sed -e 's/%/%25/g' -e 's/
/%20/g' -e 's/=/%3D/g' -e 's/;/%3B/g' -e 's/,/%2C/g')

Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths

[Christian Couder]

On Fri, Apr 3, 2026 at 10:16 AM Christian Couder
<christian.couder@gmail.com> wrote:

> # Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),
> and those percent-encoded below (% space = ; ,)
> case "$pwd_path" in
> *[!a-zA-Z0-9_.~/:%\ =,;-]*)
>         skip_all="PWD contains unsupported special characters"
>         test_done
>         ;;
> esac

It turned out that dash and perhaps other shells don't support a space
character in a case bracket expressions, so I used the following
instead:

# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),
# and those percent-encoded below (% space = , ;)
rest=$(printf "%s" "$pwd_path" | tr -d 'a-zA-Z0-9_.~/:% =,;-')
if test -n "$rest"
then
    skip_all="PWD contains unsupported special characters"
    test_done
fi

Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[Patrick Steinhardt]

On Thu, Apr 02, 2026 at 09:06:03AM +0200, Christian Couder wrote:
> Recently, I sent a 16 patch long series that makes it possible for
> promisor remotes advertised through the "promisor-remote" protocol
> capability to be auto-configured on the client side via a URL
> allowlist configured using a new `promisor.acceptFromServerUrl`
> configuration variable:
> 
> https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/
> 
> I got the suggestion to split the 16 patches series into two smaller
> series, starting with a preparatory series. So here is this
> preparatory series. It's a mix of mostly small fixes, refactorings and
> cleanups.
> 
> High level description of the patches
> =====================================
> 
>  - Patches 1-4/10 are fixes:
> 
>    - Patch 1/10 is the most significant. The others are relatively
>      small.
>      
>    - Patch 4/10 is the only new in this series (while all the others
>      were in the previous series). It prepares for Patch 5/10.
> 
>  - Patches 5-10/10 are refactorings and cleanups:
> 
>    - Patches 5-7/10 are relatively small independents cleanups or
>      refactorings.
> 
>    - Patches 8-9/10 are related refactorings simplifying the data
>      structures used in filter_promisor_remote() and
>      promisor_remote_reply().
> 
>    - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths
>      in the test script.

Thanks for splitting out these changes. In case anybody else cares,
I've attached the range-diff below.

In any case, I'm happy with these changes. There's a single nit, but I
don't think it's worth rerolling over. Thanks!

Patrick

 1:  aeae4b9971 !  1:  30d0aa8a24 promisor-remote: try accepted remotes before others in get_direct()
    @@ Commit message
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    + ## Documentation/gitprotocol-v2.adoc ##
    +@@ Documentation/gitprotocol-v2.adoc: advertised, it can reply with "promisor-remote=<pr-names>" where
    + where `pr-name` is the urlencoded name of a promisor remote the server
    + advertised and the client accepts.
    + 
    ++The promisor remotes that the client accepted will be tried before the
    ++other configured promisor remotes when the client attempts to fetch
    ++missing objects.
    ++
    + Note that, everywhere in this document, the ';' and ',' characters
    + MUST be encoded if they appear in `pr-name` or `field-value`.
    + 
    +
      ## promisor-remote.c ##
     @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,
      	return remaining_nr;
    @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,
     +	struct promisor_remote *r = repo->promisor_remote_config->promisors;
     +
     +	for (; r; r = r->next) {
    -+		if (accepted_only && !r->accepted)
    -+			continue;
    -+		if (!accepted_only && r->accepted)
    ++		if (accepted_only != r->accepted)
     +			continue;
     +		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
     +			if (*remaining_nr == 1)
    @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,
      
      	for (i = 0; i < remaining_nr; i++) {
      		if (is_promisor_object(repo, &remaining_oids[i]))
    +
    + ## t/t5710-promisor-remote-capability.sh ##
    +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "init + fetch with promisor.advertise set to 'true'" '
    + 	check_missing_objects server 1 "$oid"
    + '
    + 
    ++test_expect_success "clone with two promisors but only one advertised" '
    ++	git -C server config promisor.advertise true &&
    ++	test_when_finished "rm -rf client unused_lop" &&
    ++
    ++	# Create a promisor that will be configured but not be used
    ++	git init --bare unused_lop &&
    ++
    ++	# Clone from server to create a client
    ++	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
    ++		-c remote.unused_lop.promisor=true \
    ++		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
    ++		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
    ++		-c remote.lop.promisor=true \
    ++		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
    ++		-c remote.lop.url="file://$(pwd)/lop" \
    ++		-c promisor.acceptfromserver=All \
    ++		--no-local --filter="blob:limit=5k" server client &&
    ++
    ++	# Check that "unused_lop" appears before "lop" in the config
    ++	printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
    ++	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	# Check that "lop" was tried
    ++	test_grep " fetch lop " trace &&
    ++	# Check that "unused_lop" was not contacted
    ++	# This means "lop", the accepted promisor, was tried first
    ++	test_grep ! " fetch unused_lop " trace &&
    ++
    ++	# Check that the largest object is still missing on the server
    ++	check_missing_objects server 1 "$oid"
    ++'
    ++
    ++test_expect_success "init + fetch two promisors but only one advertised" '
    ++	git -C server config promisor.advertise true &&
    ++	test_when_finished "rm -rf client unused_lop" &&
    ++
    ++	# Create a promisor that will be configured but not be used
    ++	git init --bare unused_lop &&
    ++
    ++	mkdir client &&
    ++	git -C client init &&
    ++	git -C client config remote.unused_lop.promisor true &&
    ++	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
    ++	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
    ++	git -C client config remote.lop.promisor true &&
    ++	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
    ++	git -C client config remote.lop.url "file://$(pwd)/lop" &&
    ++	git -C client config remote.server.url "file://$(pwd)/server" &&
    ++	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
    ++	git -C client config promisor.acceptfromserver All &&
    ++
    ++	# Check that "unused_lop" appears before "lop" in the config
    ++	printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect &&
    ++	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
    ++
    ++	# Check that "lop" was tried
    ++	test_grep " fetch lop " trace &&
    ++	# Check that "unused_lop" was not contacted
    ++	# This means "lop", the accepted promisor, was tried first
    ++	test_grep ! " fetch unused_lop " trace &&
    ++
    ++	# Check that the largest object is still missing on the server
    ++	check_missing_objects server 1 "$oid"
    ++'
    ++
    + test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
    + 	git -C server config promisor.advertise true &&
    + 	test_when_finished "rm -rf client" &&
 2:  853051db60 <  -:  ---------- urlmatch: change 'allow_globs' arg to bool
 3:  e17cc86c37 <  -:  ---------- urlmatch: add url_is_valid_pattern() helper
10:  ac7742f5ba !  2:  60ba726cda promisor-remote: pass config entry to all_fields_match() directly
    @@ Metadata
      ## Commit message ##
         promisor-remote: pass config entry to all_fields_match() directly
     
    -    The `in_list == 0` path of all_fields_match() re-looks up the
    -    remote in config_info by advertised->name, even though every
    +    The `in_list == 0` path of all_fields_match() looks up the remote in
    +    `config_info` by `advertised->name` repeatedly, even though every
         caller in should_accept_remote() has already performed this
    -    lookup and holds the result in 'p'.
    +    lookup and holds the result in `p`.
     
         To avoid this useless work, let's replace the `int in_list`
         parameter with a `struct promisor_info *config_entry` pointer:
    @@ Commit message
            avoiding the redundant string_list_lookup() call.
     
         This removes the hidden dependency on `advertised->name` inside
    -    all_fields_match(), which would be wrong in the following commits when
    -    auto-configured remotes will be implemented as the local config name
    -    may differ from the server's advertised name.
    +    all_fields_match(), which would be wrong if in the future
    +    auto-configured remotes are implemented, as the local config name may
    +    differ from the server's advertised name.
    +
    +    While at it, let's also add a comment before all_fields_match() and
    +    match_field_against_config() to help understand how things work and
    +    help avoid similar issues.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
      ## promisor-remote.c ##
    +@@ promisor-remote.c: enum accept_promisor {
    + 	ACCEPT_ALL
    + };
    + 
    ++/*
    ++ * Check if a specific field and its advertised value match the local
    ++ * configuration of a given promisor remote.
    ++ *
    ++ * Returns 1 if they match, 0 otherwise.
    ++ */
    + static int match_field_against_config(const char *field, const char *value,
    + 				      struct promisor_info *config_info)
    + {
     @@ promisor-remote.c: static int match_field_against_config(const char *field, const char *value,
      	return 0;
      }
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
     -		return all_fields_match(advertised, config_info, 0);
     +		return all_fields_match(advertised, config_info, p);
      
    - 	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
    - 		  "ignoring this remote"), remote_name, p->url, remote_url);
    + 	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
    + 		remote_name, p->url, remote_url);
 4:  c2f9f3ebef !  3:  d69391fd61 promisor-remote: clarify that a remote is ignored
    @@ Metadata
      ## Commit message ##
         promisor-remote: clarify that a remote is ignored
     
    -    In should_accept_remote() when a remote is ignored, we might tell users
    -    why it is ignored in a warning, but we don't tell them that the remote
    -    is actually ignored.
    +    In should_accept_remote() and parse_one_advertised_remote(), when a
    +    remote is ignored, we tell users why it is ignored in a warning, but we
    +    don't tell them that the remote is actually ignored.
     
         Let's clarify that, so users have a better idea of what's actually
         happening.
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	}
      
      	if (!strcmp(p->url, remote_url))
    - 		return all_fields_match(advertised, config_info, 0);
    + 		return all_fields_match(advertised, config_info, p);
      
     -	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
     -		remote_name, p->url, remote_url);
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
      	return 0;
      }
    +@@ promisor-remote.c: static struct promisor_info *parse_one_advertised_remote(const char *remote_info
    + 	string_list_clear(&elem_list, 0);
    + 
    + 	if (!info->name || !info->url) {
    +-		warning(_("server advertised a promisor remote without a name or URL: %s"),
    +-			remote_info);
    ++		warning(_("server advertised a promisor remote without a name or URL: '%s', "
    ++			  "ignoring this remote"), remote_info);
    + 		promisor_info_free(info);
    + 		return NULL;
    + 	}
 -:  ---------- >  4:  f4815d68ea promisor-remote: reject empty name or URL in advertised remote
11:  ebc6f2dbeb !  5:  48605c4612 promisor-remote: refactor should_accept_remote() control flow
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor should_accept_remote() control flow
     
    -    In following commits, we are going to add URL-based acceptance logic
    -    into should_accept_remote().
    +    A previous commit made sure we now reject empty URLs early at parse
    +    time. This makes the existing warning() in case a remote URL is NULL
    +    or empty very unlikely to be useful.
     
    -    To prepare for the upcoming changes, let's restructure the control flow
    -    in should_accept_remote().
    +    In future work, we also plan to add URL-based acceptance logic into
    +    should_accept_remote().
    +
    +    To adapt to previous changes and prepare for upcoming changes, let's
    +    restructure the control flow in should_accept_remote().
     
         Concretely, let's:
     
    -     - Move the empty-URL check to the very top of the function, so that
    -       every acceptance mode, instead of only ACCEPT_KNOWN_URL, rejects
    -       remotes with a missing or blank URL early.
    +     - Replace the warning() in case of an empty URL with a BUG(), as a
    +       previous commit made sure empty URLs are rejected early at parse
    +       time.
    +
    +     - Move that modified empty-URL check to the very top of the function,
    +       so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is
    +       covered.
     
          - Invert the URL comparison: instead of returning on match and
            warning on mismatch, return early on mismatch and let the match
            case fall through. This opens a single exit path at the bottom of
    -       the function for following commits to extend.
    -
    -    Anyway the server shouldn't send any empty URL in the first place, so
    -    this shouldn't change any behavior in practice.
    +       the function for future commits to extend.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	const char *remote_name = advertised->name;
      	const char *remote_url = advertised->url;
      
    -+	if (!remote_url || !*remote_url) {
    -+		warning(_("no or empty URL advertised for remote '%s', "
    -+			  "ignoring this remote"), remote_name);
    -+		return 0;
    -+	}
    ++	if (!remote_url || !*remote_url)
    ++		BUG("no or empty URL advertised for remote '%s'; "
    ++		    "this remote should have been rejected earlier",
    ++		    remote_name);
     +
      	if (accept == ACCEPT_ALL)
      		return all_fields_match(advertised, config_info, NULL);
 5:  bdd62c20a4 !  6:  632e3bfe25 promisor-remote: refactor has_control_char()
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor has_control_char()
     
    -    In a following commit we are going to check if some strings contain
    -    control character, so let's refactor the logic to do that in a new
    +    In a future commit we are going to check if some strings contain
    +    control characters, so let's refactor the logic to do that in a new
         has_control_char() helper function.
     
    +    It cleans up the code a bit anyway.
    +
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
      ## promisor-remote.c ##
 6:  962db2ee6c !  7:  12dc262514 promisor-remote: refactor accept_from_server()
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor accept_from_server()
     
    -    In a following commit, we are going to add more logic to
    +    In future commits, we are going to add more logic to
         filter_promisor_remote() which is already doing a lot of things.
     
         Let's alleviate that by moving the logic that checks and validates the
 7:  b4d125bd36 !  8:  9d47ed27e8 promisor-remote: keep accepted promisor_info structs alive
    @@ Commit message
         protocol reply, apply advertised filters, and mark remotes as
         accepted, rather than iterating three separate structures.
     
    -    This refactoring also prepares for a following commit that will add a
    +    This refactoring also prepares for a future commit that will add a
         'local_name' member to 'struct promisor_info'. Since struct instances
         stay alive, downstream code will be able to simply read both names
         from them rather than needing yet another parallel strvec.
 8:  3252e216f1 =  9:  b6c020696d promisor-remote: remove the 'accepted' strvec
 9:  60e17ccb99 <  -:  ---------- promisor-remote: add 'local_name' to 'struct promisor_info'
12:  467ee9eff3 ! 10:  d47aa7a31a t5710: use proper file:// URIs for absolute paths
    @@ Commit message
         (`file://D:/a/repo`). Standard URI parsers misinterpret this format,
         treating `D:` as the host rather than part of the absolute path.
     
    +    This is to be expected because RFC 8089 says that the `//` prefix with
    +    an empty local host must be followed by an absolute path starting with
    +    a slash.
    +
         While this hasn't broken the existing tests (because the old
    -    `promisor.acceptFromServer` logic relies entirely on strict `strcmp`
    +    `promisor.acceptFromServer` logic relies entirely on strict `strcmp()`
         without normalizing the URLs), it will break future commits that pass
    -    these URLs through `url_normalize()`.
    +    these URLs through `url_normalize()` or similar functions.
     
         To future-proof the tests and ensure cross-platform URI compliance,
         let's introduce a $PWD_URL helper that explicitly guarantees a leading
13:  637db2b4d8 <  -:  ---------- promisor-remote: introduce promisor.acceptFromServerUrl
14:  c03c5261fb <  -:  ---------- promisor-remote: trust known remotes matching acceptFromServerUrl
15:  0d8acf10f8 <  -:  ---------- promisor-remote: auto-configure unknown remotes
16:  a27565223f <  -:  ---------- doc: promisor: improve acceptFromServer entry

Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Recently, I sent a 16 patch long series that makes it possible for
> promisor remotes advertised through the "promisor-remote" protocol
> capability to be auto-configured on the client side via a URL
> allowlist configured using a new `promisor.acceptFromServerUrl`
> configuration variable:
>
> https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/
>
> I got the suggestion to split the 16 patches series into two smaller
> series, starting with a preparatory series. So here is this
> preparatory series. It's a mix of mostly small fixes, refactorings and
> cleanups.
>
> High level description of the patches
> =====================================
>
>  - Patches 1-4/10 are fixes:
>
>    - Patch 1/10 is the most significant. The others are relatively
>      small.
>      
>    - Patch 4/10 is the only new in this series (while all the others
>      were in the previous series). It prepares for Patch 5/10.
>
>  - Patches 5-10/10 are refactorings and cleanups:
>
>    - Patches 5-7/10 are relatively small independents cleanups or
>      refactorings.
>
>    - Patches 8-9/10 are related refactorings simplifying the data
>      structures used in filter_promisor_remote() and
>      promisor_remote_reply().
>
>    - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths
>      in the test script.
>
> CI tests
> ========
>
> They all pass, see:
>
> https://github.com/chriscool/git/actions/runs/23848484597
>
> Range-diff
> ==========
>
> Sorry, no range-diff as I don't think it would be quite useful because
> the number and order of commits has changed a lot.

When comparing against 16-patch series from the previous round, 6 of
the old patches will have no corresponding patch in this round,
which is expected.  But for the remaining 10 commits, the command
seems to do a decent job matching up the corresponding patches from
the previous round.  It is especially pleasing to see that the first
one from each round are matched up and showing that its tests are
moderately extended.

 1:  292252ce36 !  1:  d904d5f94d promisor-remote: try accepted remotes before others in get_direct()
    @@ Commit message
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    + ## Documentation/gitprotocol-v2.adoc ##
    +@@ Documentation/gitprotocol-v2.adoc: advertised, it can reply with "promisor-remote=<pr-names>" where
    + where `pr-name` is the urlencoded name of a promisor remote the server
    + advertised and the client accepts.
    + 
    ++The promisor remotes that the client accepted will be tried before the
    ++other configured promisor remotes when the client attempts to fetch
    ++missing objects.
    ++
    + Note that, everywhere in this document, the ';' and ',' characters
    + MUST be encoded if they appear in `pr-name` or `field-value`.
    + 
    +
      ## promisor-remote.c ##
     @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,
      	return remaining_nr;
    @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,
     +	struct promisor_remote *r = repo->promisor_remote_config->promisors;
     +
     +	for (; r; r = r->next) {
    -+		if (accepted_only && !r->accepted)
    -+			continue;
    -+		if (!accepted_only && r->accepted)
    ++		if (accepted_only != r->accepted)
     +			continue;
     +		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
     +			if (*remaining_nr == 1)
    @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,
      
      	for (i = 0; i < remaining_nr; i++) {
      		if (is_promisor_object(repo, &remaining_oids[i]))
    +
    + ## t/t5710-promisor-remote-capability.sh ##
    +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "init + fetch with promisor.advertise set to 'true'" '
    + 	check_missing_objects server 1 "$oid"
    + '
    + 
    ++test_expect_success "clone with two promisors but only one advertised" '
    ++	git -C server config promisor.advertise true &&
    ++	test_when_finished "rm -rf client unused_lop" &&
    ++
    ++	# Create a promisor that will be configured but not be used
    ++	git init --bare unused_lop &&
    ++
    ++	# Clone from server to create a client
    ++	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
    ++		-c remote.unused_lop.promisor=true \
    ++		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
    ++		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
    ++		-c remote.lop.promisor=true \
    ++		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
    ++		-c remote.lop.url="file://$(pwd)/lop" \
    ++		-c promisor.acceptfromserver=All \
    ++		--no-local --filter="blob:limit=5k" server client &&
    ++
    ++	# Check that "unused_lop" appears before "lop" in the config
    ++	printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
    ++	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	# Check that "lop" was tried
    ++	test_grep " fetch lop " trace &&
    ++	# Check that "unused_lop" was not contacted
    ++	# This means "lop", the accepted promisor, was tried first
    ++	test_grep ! " fetch unused_lop " trace &&
    ++
    ++	# Check that the largest object is still missing on the server
    ++	check_missing_objects server 1 "$oid"
    ++'
    ++
    ++test_expect_success "init + fetch two promisors but only one advertised" '
    ++	git -C server config promisor.advertise true &&
    ++	test_when_finished "rm -rf client unused_lop" &&
    ++
    ++	# Create a promisor that will be configured but not be used
    ++	git init --bare unused_lop &&
    ++
    ++	mkdir client &&
    ++	git -C client init &&
    ++	git -C client config remote.unused_lop.promisor true &&
    ++	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
    ++	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
    ++	git -C client config remote.lop.promisor true &&
    ++	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
    ++	git -C client config remote.lop.url "file://$(pwd)/lop" &&
    ++	git -C client config remote.server.url "file://$(pwd)/server" &&
    ++	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
    ++	git -C client config promisor.acceptfromserver All &&
    ++
    ++	# Check that "unused_lop" appears before "lop" in the config
    ++	printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect &&
    ++	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
    ++
    ++	# Check that "lop" was tried
    ++	test_grep " fetch lop " trace &&
    ++	# Check that "unused_lop" was not contacted
    ++	# This means "lop", the accepted promisor, was tried first
    ++	test_grep ! " fetch unused_lop " trace &&
    ++
    ++	# Check that the largest object is still missing on the server
    ++	check_missing_objects server 1 "$oid"
    ++'
    ++
    + test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
    + 	git -C server config promisor.advertise true &&
    + 	test_when_finished "rm -rf client" &&
 2:  8cea12d525 <  -:  ---------- urlmatch: change 'allow_globs' arg to bool
 3:  30e3618f1b <  -:  ---------- urlmatch: add url_is_valid_pattern() helper
10:  0ccf10712d !  2:  54174b6db5 promisor-remote: pass config entry to all_fields_match() directly
    @@ Metadata
      ## Commit message ##
         promisor-remote: pass config entry to all_fields_match() directly
     
    -    The `in_list == 0` path of all_fields_match() re-looks up the
    -    remote in config_info by advertised->name, even though every
    +    The `in_list == 0` path of all_fields_match() looks up the remote in
    +    `config_info` by `advertised->name` repeatedly, even though every
         caller in should_accept_remote() has already performed this
    -    lookup and holds the result in 'p'.
    +    lookup and holds the result in `p`.
     
         To avoid this useless work, let's replace the `int in_list`
         parameter with a `struct promisor_info *config_entry` pointer:
    @@ Commit message
            avoiding the redundant string_list_lookup() call.
     
         This removes the hidden dependency on `advertised->name` inside
    -    all_fields_match(), which would be wrong in the following commits when
    -    auto-configured remotes will be implemented as the local config name
    -    may differ from the server's advertised name.
    +    all_fields_match(), which would be wrong if in the future
    +    auto-configured remotes are implemented, as the local config name may
    +    differ from the server's advertised name.
    +
    +    While at it, let's also add a comment before all_fields_match() and
    +    match_field_against_config() to help understand how things work and
    +    help avoid similar issues.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## promisor-remote.c ##
    +@@ promisor-remote.c: enum accept_promisor {
    + 	ACCEPT_ALL
    + };
    + 
    ++/*
    ++ * Check if a specific field and its advertised value match the local
    ++ * configuration of a given promisor remote.
    ++ *
    ++ * Returns 1 if they match, 0 otherwise.
    ++ */
    + static int match_field_against_config(const char *field, const char *value,
    + 				      struct promisor_info *config_info)
    + {
     @@ promisor-remote.c: static int match_field_against_config(const char *field, const char *value,
      	return 0;
      }
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
     -		return all_fields_match(advertised, config_info, 0);
     +		return all_fields_match(advertised, config_info, p);
      
    - 	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
    - 		  "ignoring this remote"), remote_name, p->url, remote_url);
    + 	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
    + 		remote_name, p->url, remote_url);
 4:  b602c1d5ad !  3:  56642a3c6a promisor-remote: clarify that a remote is ignored
    @@ Metadata
      ## Commit message ##
         promisor-remote: clarify that a remote is ignored
     
    -    In should_accept_remote() when a remote is ignored, we might tell users
    -    why it is ignored in a warning, but we don't tell them that the remote
    -    is actually ignored.
    +    In should_accept_remote() and parse_one_advertised_remote(), when a
    +    remote is ignored, we tell users why it is ignored in a warning, but we
    +    don't tell them that the remote is actually ignored.
     
         Let's clarify that, so users have a better idea of what's actually
         happening.
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	}
      
      	if (!strcmp(p->url, remote_url))
    - 		return all_fields_match(advertised, config_info, 0);
    + 		return all_fields_match(advertised, config_info, p);
      
     -	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
     -		remote_name, p->url, remote_url);
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
      	return 0;
      }
    +@@ promisor-remote.c: static struct promisor_info *parse_one_advertised_remote(const char *remote_info
    + 	string_list_clear(&elem_list, 0);
    + 
    + 	if (!info->name || !info->url) {
    +-		warning(_("server advertised a promisor remote without a name or URL: %s"),
    +-			remote_info);
    ++		warning(_("server advertised a promisor remote without a name or URL: '%s', "
    ++			  "ignoring this remote"), remote_info);
    + 		promisor_info_free(info);
    + 		return NULL;
    + 	}
 -:  ---------- >  4:  859ba9d41c promisor-remote: reject empty name or URL in advertised remote
11:  c2b234a4ce !  5:  724f9bbc22 promisor-remote: refactor should_accept_remote() control flow
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor should_accept_remote() control flow
     
    -    In following commits, we are going to add URL-based acceptance logic
    -    into should_accept_remote().
    +    A previous commit made sure we now reject empty URLs early at parse
    +    time. This makes the existing warning() in case a remote URL is NULL
    +    or empty very unlikely to be useful.
     
    -    To prepare for the upcoming changes, let's restructure the control flow
    -    in should_accept_remote().
    +    In future work, we also plan to add URL-based acceptance logic into
    +    should_accept_remote().
    +
    +    To adapt to previous changes and prepare for upcoming changes, let's
    +    restructure the control flow in should_accept_remote().
     
         Concretely, let's:
     
    -     - Move the empty-URL check to the very top of the function, so that
    -       every acceptance mode, instead of only ACCEPT_KNOWN_URL, rejects
    -       remotes with a missing or blank URL early.
    +     - Replace the warning() in case of an empty URL with a BUG(), as a
    +       previous commit made sure empty URLs are rejected early at parse
    +       time.
    +
    +     - Move that modified empty-URL check to the very top of the function,
    +       so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is
    +       covered.
     
          - Invert the URL comparison: instead of returning on match and
            warning on mismatch, return early on mismatch and let the match
            case fall through. This opens a single exit path at the bottom of
    -       the function for following commits to extend.
    -
    -    Anyway the server shouldn't send any empty URL in the first place, so
    -    this shouldn't change any behavior in practice.
    +       the function for future commits to extend.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	const char *remote_name = advertised->name;
      	const char *remote_url = advertised->url;
      
    -+	if (!remote_url || !*remote_url) {
    -+		warning(_("no or empty URL advertised for remote '%s', "
    -+			  "ignoring this remote"), remote_name);
    -+		return 0;
    -+	}
    ++	if (!remote_url || !*remote_url)
    ++		BUG("no or empty URL advertised for remote '%s'; "
    ++		    "this remote should have been rejected earlier",
    ++		    remote_name);
     +
      	if (accept == ACCEPT_ALL)
      		return all_fields_match(advertised, config_info, NULL);
 5:  c51bed2fe6 !  6:  80cfbccba9 promisor-remote: refactor has_control_char()
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor has_control_char()
     
    -    In a following commit we are going to check if some strings contain
    -    control character, so let's refactor the logic to do that in a new
    +    In a future commit we are going to check if some strings contain
    +    control characters, so let's refactor the logic to do that in a new
         has_control_char() helper function.
     
    +    It cleans up the code a bit anyway.
    +
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
 6:  b063f4596a !  7:  3f2d6f8a50 promisor-remote: refactor accept_from_server()
    @@ Metadata
      ## Commit message ##
         promisor-remote: refactor accept_from_server()
     
    -    In a following commit, we are going to add more logic to
    +    In future commits, we are going to add more logic to
         filter_promisor_remote() which is already doing a lot of things.
     
         Let's alleviate that by moving the logic that checks and validates the
 7:  1e378b33c0 !  8:  dff4d339c8 promisor-remote: keep accepted promisor_info structs alive
    @@ Commit message
         protocol reply, apply advertised filters, and mark remotes as
         accepted, rather than iterating three separate structures.
     
    -    This refactoring also prepares for a following commit that will add a
    +    This refactoring also prepares for a future commit that will add a
         'local_name' member to 'struct promisor_info'. Since struct instances
         stay alive, downstream code will be able to simply read both names
         from them rather than needing yet another parallel strvec.
 8:  2304313704 =  9:  1fb08184c4 promisor-remote: remove the 'accepted' strvec
 9:  86d6a9e385 <  -:  ---------- promisor-remote: add 'local_name' to 'struct promisor_info'
12:  5c4461affc ! 10:  7cb0268075 t5710: use proper file:// URIs for absolute paths
    @@ Commit message
         (`file://D:/a/repo`). Standard URI parsers misinterpret this format,
         treating `D:` as the host rather than part of the absolute path.
     
    +    This is to be expected because RFC 8089 says that the `//` prefix with
    +    an empty local host must be followed by an absolute path starting with
    +    a slash.
    +
         While this hasn't broken the existing tests (because the old
    -    `promisor.acceptFromServer` logic relies entirely on strict `strcmp`
    +    `promisor.acceptFromServer` logic relies entirely on strict `strcmp()`
         without normalizing the URLs), it will break future commits that pass
    -    these URLs through `url_normalize()`.
    +    these URLs through `url_normalize()` or similar functions.
     
         To future-proof the tests and ensure cross-platform URI compliance,
         let's introduce a $PWD_URL helper that explicitly guarantees a leading
13:  294155978a <  -:  ---------- promisor-remote: introduce promisor.acceptFromServerUrl
14:  f17a3ed35b <  -:  ---------- promisor-remote: trust known remotes matching acceptFromServerUrl
15:  db503a285d <  -:  ---------- promisor-remote: auto-configure unknown remotes
16:  d92889efa9 <  -:  ---------- doc: promisor: improve acceptFromServer entry




> Christian Couder (10):
>   promisor-remote: try accepted remotes before others in get_direct()
>   promisor-remote: pass config entry to all_fields_match() directly
>   promisor-remote: clarify that a remote is ignored
>   promisor-remote: reject empty name or URL in advertised remote
>   promisor-remote: refactor should_accept_remote() control flow
>   promisor-remote: refactor has_control_char()
>   promisor-remote: refactor accept_from_server()
>   promisor-remote: keep accepted promisor_info structs alive
>   promisor-remote: remove the 'accepted' strvec
>   t5710: use proper file:// URIs for absolute paths
>
>  Documentation/gitprotocol-v2.adoc     |   4 +
>  promisor-remote.c                     | 207 +++++++++++++++-----------
>  t/t5710-promisor-remote-capability.sh | 122 ++++++++++++---
>  3 files changed, 223 insertions(+), 110 deletions(-)

Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[Christian Couder]

On Thu, Apr 2, 2026 at 8:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > Range-diff
> > ==========
> >
> > Sorry, no range-diff as I don't think it would be quite useful because
> > the number and order of commits has changed a lot.
>
> When comparing against 16-patch series from the previous round, 6 of
> the old patches will have no corresponding patch in this round,
> which is expected.  But for the remaining 10 commits, the command
> seems to do a decent job matching up the corresponding patches from
> the previous round.

Yeah, the range-diff is actually better than I expected.

>  It is especially pleasing to see that the first
> one from each round are matched up and showing that its tests are
> moderately extended.

Yeah, I followed Patrick's previous suggestion about adding tests.
Maybe I should have mentioned it somewhere in the cover letter.

[PATCH v2 00/10] Prepare for advertised remotes auto-configure via URL allowlist

[Christian Couder]

Recently, I sent a 16 patch long series that makes it possible for
promisor remotes advertised through the "promisor-remote" protocol
capability to be auto-configured on the client side via a URL
allowlist configured using a new `promisor.acceptFromServerUrl`
configuration variable:

https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/

I got the suggestion to split the 16 patches series into two smaller
series, starting with a preparatory series. So here is this
preparatory series. It's a mix of mostly small fixes, refactorings and
cleanups.

High level description of the patches
=====================================

 - Patches 1-4/10 are fixes:

   - Patch 1/10 is the most significant. The others are relatively
     small.
     
   - Patch 4/10 is the only new in this series (while all the others
     were in the previous series). It prepares for Patch 5/10.

 - Patches 5-10/10 are refactorings and cleanups:

   - Patches 5-7/10 are relatively small independents cleanups or
     refactorings.

   - Patches 8-9/10 are related refactorings simplifying the data
     structures used in filter_promisor_remote() and
     promisor_remote_reply().

   - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths
     in the test script.

Changes since v1
================

Thanks to Patrick and Junio for reviewing the previous versions of
this series.

Only patch 10/10 ("t5710: use proper file:// URIs for absolute paths")
changed since v1:

 - PWD_URL and ENCODED_PWD_URL have been renamed TRASH_DIRECTORY_URL
   and ENCODED_TRASH_DIRECTORY_URL respectively.

 - Instead of percent-encoding only space characters, we now encode
   '%', ' ', '=', ',' and ';'.

 - The test script is skipped altogether if there are other characters
   than those we encode or expect in the current directory path.

 - The tests introduced by commit 1/10 now also benefit from the
   TRASH_DIRECTORY_URL and ENCODED_TRASH_DIRECTORY_URL variables.

 - Commit message and code comments are improved a bit.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/24076952684

Range-diff since v1
===================

 1:  906c5f6fbb =  1:  6687a0fe3f promisor-remote: try accepted remotes before others in get_direct()
 2:  830eb10097 =  2:  23869a4509 promisor-remote: pass config entry to all_fields_match() directly
 3:  b18cdf2fcb =  3:  16d5e0b7ef promisor-remote: clarify that a remote is ignored
 4:  b2125ec770 =  4:  f55f6e8c14 promisor-remote: reject empty name or URL in advertised remote
 5:  e69abb1256 =  5:  52cd36ab64 promisor-remote: refactor should_accept_remote() control flow
 6:  772ea53e9e =  6:  286ae3b4e3 promisor-remote: refactor has_control_char()
 7:  79cf945c24 =  7:  4cb239cd24 promisor-remote: refactor accept_from_server()
 8:  b953e2491e =  8:  7bfe26d5d4 promisor-remote: keep accepted promisor_info structs alive
 9:  3fce719fc0 =  9:  9ffa80444f promisor-remote: remove the 'accepted' strvec
10:  e54af9e176 ! 10:  b77b449c76 t5710: use proper file:// URIs for absolute paths
    @@ Commit message
         these URLs through `url_normalize()` or similar functions.
     
         To future-proof the tests and ensure cross-platform URI compliance,
    -    let's introduce a $PWD_URL helper that explicitly guarantees a leading
    -    slash for the path component, ensuring valid 3-slash `file:///` URIs on
    -    all operating systems.
    +    let's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly
    +    guarantees a leading slash for the path component, ensuring valid
    +    3-slash `file:///` URIs on all operating systems.
     
    -    While at it, let's also introduce $ENCODED_PWD_URL to handle spaces in
    -    directory paths (which is needed for URL glob pattern matching).
    +    While at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to
    +    handle some common special characters in directory paths.
     
    -    Then let's replace all instances of `file://$(pwd)` with $PWD_URL across
    -    the test script, and let's simplify the `ENCODED_URL` constructions in
    -    the `sendFields` and `checkFields` tests to use $ENCODED_PWD_URL
    -    directly.
    +    To be extra safe, let's skip all the tests if there are uncommon
    +    special characters in the directory path.
    +
    +    Then let's replace all instances of `file://$(pwd)` with
    +    $TRASH_DIRECTORY_URL across the test script, and let's simplify the
    +    `sendFields` and `checkFields` tests to use
    +    $ENCODED_TRASH_DIRECTORY_URL directly.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ t/t5710-promisor-remote-capability.sh: copy_to_lop () {
      	cp "$path" "$path2"
      }
      
    -+# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn
    ++# On Windows, `pwd` returns a path like 'D:/foo/bar'. Prepend '/' to turn
     +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.
     +# On Unix, the path already starts with '/', so this is a no-op.
     +pwd_path=$(pwd)
     +case "$pwd_path" in
     +[a-zA-Z]:*) pwd_path="/$pwd_path" ;;
     +esac
    -+PWD_URL="file://$pwd_path"
    -+# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.
    -+ENCODED_PWD_URL="file://$(echo "$pwd_path" | sed "s/ /%20/g")"
    ++
    ++# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),
    ++# and those percent-encoded below (% space = , ;)
    ++rest=$(printf "%s" "$pwd_path" | tr -d 'a-zA-Z0-9_.~/:% =,;-')
    ++if test -n "$rest"
    ++then
    ++	skip_all="PWD contains unsupported special characters"
    ++	test_done
    ++fi
    ++
    ++TRASH_DIRECTORY_URL="file://$pwd_path"
    ++
    ++encoded_path=$(printf "%s" "$pwd_path" |
    ++	       sed -e 's/%/%25/g' -e 's/ /%20/g' -e 's/=/%3D/g' \
    ++		   -e 's/;/%3B/g' -e 's/,/%2C/g')
    ++
    ++ENCODED_TRASH_DIRECTORY_URL="file://$encoded_path"
     +
      test_expect_success "setup for testing promisor remote advertisement" '
      	# Create another bare repo called "lop" (for Large Object Promisor)
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "setup for testing pr
      
      	# Configure lop as promisor remote for server
     -	git -C server remote add lop "file://$(pwd)/lop" &&
    -+	git -C server remote add lop "$PWD_URL/lop" &&
    ++	git -C server remote add lop "$TRASH_DIRECTORY_URL/lop" &&
      	git -C server config remote.lop.promisor true &&
      
      	git -C lop config uploadpack.allowFilter true &&
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=All \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=All \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=None \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "init + fetch with pr
      	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
     -	git -C client config remote.lop.url "file://$(pwd)/lop" &&
     -	git -C client config remote.server.url "file://$(pwd)/server" &&
    -+	git -C client config remote.lop.url "$PWD_URL/lop" &&
    -+	git -C client config remote.server.url "$PWD_URL/server" &&
    ++	git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" &&
    ++	git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" &&
      	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
      	git -C client config promisor.acceptfromserver All &&
      	GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
    +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with two promisors but only one advertised" '
    + 	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
    + 		-c remote.unused_lop.promisor=true \
    + 		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
    +-		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
    ++		-c remote.unused_lop.url="$TRASH_DIRECTORY_URL/unused_lop" \
    + 		-c remote.lop.promisor=true \
    + 		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
    +-		-c remote.lop.url="file://$(pwd)/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
    + 		-c promisor.acceptfromserver=All \
    + 		--no-local --filter="blob:limit=5k" server client &&
    + 
    +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "init + fetch two promisors but only one advertised" '
    + 	git -C client init &&
    + 	git -C client config remote.unused_lop.promisor true &&
    + 	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
    +-	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
    ++	git -C client config remote.unused_lop.url "$TRASH_DIRECTORY_URL/unused_lop" &&
    + 	git -C client config remote.lop.promisor true &&
    + 	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
    +-	git -C client config remote.lop.url "file://$(pwd)/lop" &&
    +-	git -C client config remote.server.url "file://$(pwd)/server" &&
    ++	git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" &&
    ++	git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" &&
    + 	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
    + 	git -C client config promisor.acceptfromserver All &&
    + 
     @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
      	# Clone from server to create a client
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=KnownName \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownNam
      	GIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \
      		-c remote.serverTwo.fetch="+refs/heads/*:refs/remotes/lop/*" \
     -		-c remote.serverTwo.url="file://$(pwd)/lop" \
    -+		-c remote.serverTwo.url="$PWD_URL/lop" \
    ++		-c remote.serverTwo.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=KnownName \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=KnownUrl \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	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)/serverTwo" \
    -+		-c remote.lop.url="$PWD_URL/serverTwo" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \
      		-c promisor.acceptfromserver=KnownUrl \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	test_when_finished "rm -rf client" &&
      
     -	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
    -+	test_when_finished "git -C server config set remote.lop.url \"$PWD_URL/lop\"" &&
    ++	test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" &&
      	git -C server config unset remote.lop.url &&
      
      	# Clone from server to create a client
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	test_must_fail env 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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=KnownUrl \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	test_when_finished "rm -rf client" &&
      
     -	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
    -+	test_when_finished "git -C server config set remote.lop.url \"$PWD_URL/lop\"" &&
    ++	test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" &&
      	git -C server config set remote.lop.url "" &&
      
      	# Clone from server to create a client
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	test_must_fail env 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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=KnownUrl \
      		--no-local --filter="blob:limit=5k" server client &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      		-c remote.lop.promisor=true \
      		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
     -		-c remote.lop.url="file://$(pwd)/lop" \
    -+		-c remote.lop.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=All \
      		--no-local --filter="blob:limit=5k" server client &&
      
      	# Check that fields are properly transmitted
     -	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
     -	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
    -+	PR1="name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none" &&
    ++	PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" &&
      	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
      	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
      	test_grep "clone> promisor-remote=lop;otherLop" trace &&
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      		-c remote.lop.promisor=true \
      		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
     -		-c remote.lop.url="file://$(pwd)/lop" \
    -+		-c remote.lop.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c remote.lop.partialCloneFilter="blob:none" \
      		-c promisor.acceptfromserver=All \
      		-c promisor.checkFields=partialcloneFilter \
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	# Check that fields are properly transmitted
     -	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
     -	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
    -+	PR1="name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none" &&
    ++	PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" &&
      	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
      	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
      	test_grep "clone> promisor-remote=lop" trace &&
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      		-c remote.lop.promisor=true \
      		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
     -		-c remote.lop.url="file://$(pwd)/lop" \
    -+		-c remote.lop.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c remote.lop.token="fooYYY" \
      		-c remote.lop.partialCloneFilter="blob:none" \
      		-c promisor.acceptfromserver=All \
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone and fetch with
      	GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
      		-c remote.lop.promisor=true \
     -		-c remote.lop.url="file://$(pwd)/lop" \
    -+		-c remote.lop.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=All \
      		--no-local --filter=auto server client 2>err &&
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
      	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.url="$PWD_URL/lop" \
    ++		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
      		-c promisor.acceptfromserver=All \
      		--no-local --filter="blob:limit=5k" server client &&

Christian Couder (10):
  promisor-remote: try accepted remotes before others in get_direct()
  promisor-remote: pass config entry to all_fields_match() directly
  promisor-remote: clarify that a remote is ignored
  promisor-remote: reject empty name or URL in advertised remote
  promisor-remote: refactor should_accept_remote() control flow
  promisor-remote: refactor has_control_char()
  promisor-remote: refactor accept_from_server()
  promisor-remote: keep accepted promisor_info structs alive
  promisor-remote: remove the 'accepted' strvec
  t5710: use proper file:// URIs for absolute paths

 Documentation/gitprotocol-v2.adoc     |   4 +
 promisor-remote.c                     | 207 +++++++++++++++-----------
 t/t5710-promisor-remote-capability.sh | 138 ++++++++++++++---
 3 files changed, 238 insertions(+), 111 deletions(-)

-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 01/10] promisor-remote: try accepted remotes before others in get_direct()

[Christian Couder]

When a server advertises promisor remotes and the client accepts some
of them, those remotes carry the server's intent: 'fetch missing
objects preferably from here', and the client agrees with that for the
remotes it accepts.

However promisor_remote_get_direct() actually iterates over all
promisor remotes in list order, which is the order they appear in the
config files (except perhaps for the one appearing in the
`extensions.partialClone` config variable which is tried last).

This means an existing, but not accepted, promisor remote, could be
tried before the accepted ones, which does not reflect the intent of
the agreement between client and server.

If the client doesn't care about what the server suggests, it should
accept nothing and rely on its remotes as they are already configured.

To better reflect the agreement between client and server, let's make
promisor_remote_get_direct() try the accepted promisor remotes before
the non-accepted ones.

Concretely, let's extract a try_promisor_remotes() helper and call it
twice from promisor_remote_get_direct():

- first with an `accepted_only=true` argument to try only the accepted
  remotes,
- then with `accepted_only=false` to fall back to any remaining remote.

Ensuring that accepted remotes are preferred will be even more
important if in the future a mechanism is developed to allow the
client to auto-configure remotes that the server advertises. This will
in particular avoid fetching from the server (which is already
configured as a promisor remote) before trying the auto-configured
remotes, as these new remotes would likely appear at the end of the
config file, and as the server might not appear in the
`extensions.partialClone` config variable.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/gitprotocol-v2.adoc     |  4 ++
 promisor-remote.c                     | 44 ++++++++++++-----
 t/t5710-promisor-remote-capability.sh | 69 +++++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 13 deletions(-)

diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc
index f985cb4c47..4fcb1a7bda 100644
--- a/Documentation/gitprotocol-v2.adoc
+++ b/Documentation/gitprotocol-v2.adoc
@@ -848,6 +848,10 @@ advertised, it can reply with "promisor-remote=<pr-names>" where
 where `pr-name` is the urlencoded name of a promisor remote the server
 advertised and the client accepts.
 
+The promisor remotes that the client accepted will be tried before the
+other configured promisor remotes when the client attempts to fetch
+missing objects.
+
 Note that, everywhere in this document, the ';' and ',' characters
 MUST be encoded if they appear in `pr-name` or `field-value`.
 
diff --git a/promisor-remote.c b/promisor-remote.c
index 96fa215b06..7ce7d22f95 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
+static int try_promisor_remotes(struct repository *repo,
+				struct object_id **remaining_oids,
+				int *remaining_nr, int *to_free,
+				bool accepted_only)
+{
+	struct promisor_remote *r = repo->promisor_remote_config->promisors;
+
+	for (; r; r = r->next) {
+		if (accepted_only != r->accepted)
+			continue;
+		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
+			if (*remaining_nr == 1)
+				continue;
+			*remaining_nr = remove_fetched_oids(repo, remaining_oids,
+							    *remaining_nr, *to_free);
+			if (*remaining_nr) {
+				*to_free = 1;
+				continue;
+			}
+		}
+		return 1; /* all fetched */
+	}
+	return 0;
+}
+
 void promisor_remote_get_direct(struct repository *repo,
 				const struct object_id *oids,
 				int oid_nr)
 {
-	struct promisor_remote *r;
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
@@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo,
 
 	promisor_remote_init(repo);
 
-	for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
-		if (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) {
-			if (remaining_nr == 1)
-				continue;
-			remaining_nr = remove_fetched_oids(repo, &remaining_oids,
-							 remaining_nr, to_free);
-			if (remaining_nr) {
-				to_free = 1;
-				continue;
-			}
-		}
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
+				 &to_free, true))
+		goto all_fetched;
+	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
+				 &to_free, false))
 		goto all_fetched;
-	}
 
 	for (i = 0; i < remaining_nr; i++) {
 		if (is_promisor_object(repo, &remaining_oids[i]))
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 357822c01a..bf0eed9f10 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -166,6 +166,75 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	check_missing_objects server 1 "$oid"
 '
 
+test_expect_success "clone with two promisors but only one advertised" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client unused_lop" &&
+
+	# Create a promisor that will be configured but not be used
+	git init --bare unused_lop &&
+
+	# Clone from server to create a client
+	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
+		-c remote.unused_lop.promisor=true \
+		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
+		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
+		-c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="file://$(pwd)/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client &&
+
+	# Check that "unused_lop" appears before "lop" in the config
+	printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
+	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
+	test_cmp expect actual &&
+
+	# Check that "lop" was tried
+	test_grep " fetch lop " trace &&
+	# Check that "unused_lop" was not contacted
+	# This means "lop", the accepted promisor, was tried first
+	test_grep ! " fetch unused_lop " trace &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "init + fetch two promisors but only one advertised" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client unused_lop" &&
+
+	# Create a promisor that will be configured but not be used
+	git init --bare unused_lop &&
+
+	mkdir client &&
+	git -C client init &&
+	git -C client config remote.unused_lop.promisor true &&
+	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
+	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
+	git -C client config remote.lop.promisor true &&
+	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
+	git -C client config remote.lop.url "file://$(pwd)/lop" &&
+	git -C client config remote.server.url "file://$(pwd)/server" &&
+	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
+	git -C client config promisor.acceptfromserver All &&
+
+	# Check that "unused_lop" appears before "lop" in the config
+	printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect &&
+	git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
+	test_cmp expect actual &&
+
+	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
+
+	# Check that "lop" was tried
+	test_grep " fetch lop " trace &&
+	# Check that "unused_lop" was not contacted
+	# This means "lop", the accepted promisor, was tried first
+	test_grep ! " fetch unused_lop " trace &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 02/10] promisor-remote: pass config entry to all_fields_match() directly

[Christian Couder]

The `in_list == 0` path of all_fields_match() looks up the remote in
`config_info` by `advertised->name` repeatedly, even though every
caller in should_accept_remote() has already performed this
lookup and holds the result in `p`.

To avoid this useless work, let's replace the `int in_list`
parameter with a `struct promisor_info *config_entry` pointer:

 - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as
   the old `in_list == 1` path did.

 - When non-NULL: match against that single config entry directly,
   avoiding the redundant string_list_lookup() call.

This removes the hidden dependency on `advertised->name` inside
all_fields_match(), which would be wrong if in the future
auto-configured remotes are implemented, as the local config name may
differ from the server's advertised name.

While at it, let's also add a comment before all_fields_match() and
match_field_against_config() to help understand how things work and
help avoid similar issues.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 7ce7d22f95..6c935f855a 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -575,6 +575,12 @@ enum accept_promisor {
 	ACCEPT_ALL
 };
 
+/*
+ * Check if a specific field and its advertised value match the local
+ * configuration of a given promisor remote.
+ *
+ * Returns 1 if they match, 0 otherwise.
+ */
 static int match_field_against_config(const char *field, const char *value,
 				      struct promisor_info *config_info)
 {
@@ -586,9 +592,18 @@ static int match_field_against_config(const char *field, const char *value,
 	return 0;
 }
 
+/*
+ * Check that the advertised fields match the local configuration.
+ *
+ * When 'config_entry' is NULL (ACCEPT_ALL mode), every checked field
+ * must match at least one remote in 'config_info'.
+ *
+ * When 'config_entry' points to a specific remote's config, the
+ * checked fields are compared against that single remote only.
+ */
 static int all_fields_match(struct promisor_info *advertised,
 			    struct string_list *config_info,
-			    int in_list)
+			    struct promisor_info *config_entry)
 {
 	struct string_list *fields = fields_checked();
 	struct string_list_item *item_checked;
@@ -597,7 +612,6 @@ static int all_fields_match(struct promisor_info *advertised,
 		int match = 0;
 		const char *field = item_checked->string;
 		const char *value = NULL;
-		struct string_list_item *item;
 
 		if (!strcasecmp(field, promisor_field_filter))
 			value = advertised->filter;
@@ -607,7 +621,11 @@ static int all_fields_match(struct promisor_info *advertised,
 		if (!value)
 			return 0;
 
-		if (in_list) {
+		if (config_entry) {
+			match = match_field_against_config(field, value,
+							   config_entry);
+		} else {
+			struct string_list_item *item;
 			for_each_string_list_item(item, config_info) {
 				struct promisor_info *p = item->util;
 				if (match_field_against_config(field, value, p)) {
@@ -615,12 +633,6 @@ static int all_fields_match(struct promisor_info *advertised,
 					break;
 				}
 			}
-		} else {
-			item = string_list_lookup(config_info, advertised->name);
-			if (item) {
-				struct promisor_info *p = item->util;
-				match = match_field_against_config(field, value, p);
-			}
 		}
 
 		if (!match)
@@ -640,7 +652,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	const char *remote_url = advertised->url;
 
 	if (accept == ACCEPT_ALL)
-		return all_fields_match(advertised, config_info, 1);
+		return all_fields_match(advertised, config_info, NULL);
 
 	/* Get config info for that promisor remote */
 	item = string_list_lookup(config_info, remote_name);
@@ -652,7 +664,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	p = item->util;
 
 	if (accept == ACCEPT_KNOWN_NAME)
-		return all_fields_match(advertised, config_info, 0);
+		return all_fields_match(advertised, config_info, p);
 
 	if (accept != ACCEPT_KNOWN_URL)
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
@@ -663,7 +675,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	}
 
 	if (!strcmp(p->url, remote_url))
-		return all_fields_match(advertised, config_info, 0);
+		return all_fields_match(advertised, config_info, p);
 
 	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
 		remote_name, p->url, remote_url);
-- 
2.54.0.rc0.114.g05d466edb8

Re: [PATCH v2 02/10] promisor-remote: pass config entry to all_fields_match() directly

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> This removes the hidden dependency on `advertised->name` inside
> all_fields_match(), which would be wrong if in the future
> auto-configured remotes are implemented, as the local config name may
> differ from the server's advertised name.

Interesting.

The caller, should_accept_remote(), still uses remote_name variable
that is an alias to advertised->name to find the config_entry to
pass down the callchain, so this step does not change the fact that
we are still using their name and not overriding it with our local
name, but hopefully we will see such a change on the caller's side
to allow us do so.

> While at it, let's also add a comment before all_fields_match() and
> match_field_against_config() to help understand how things work and
> help avoid similar issues.

Very well done.

[PATCH v2 03/10] promisor-remote: clarify that a remote is ignored

[Christian Couder]

In should_accept_remote() and parse_one_advertised_remote(), when a
remote is ignored, we tell users why it is ignored in a warning, but we
don't tell them that the remote is actually ignored.

Let's clarify that, so users have a better idea of what's actually
happening.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 6c935f855a..8e062ec160 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
 
 	if (!remote_url || !*remote_url) {
-		warning(_("no or empty URL advertised for remote '%s'"), remote_name);
+		warning(_("no or empty URL advertised for remote '%s', "
+			  "ignoring this remote"), remote_name);
 		return 0;
 	}
 
 	if (!strcmp(p->url, remote_url))
 		return all_fields_match(advertised, config_info, p);
 
-	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
-		remote_name, p->url, remote_url);
+	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
+		  "ignoring this remote"), remote_name, p->url, remote_url);
 
 	return 0;
 }
@@ -722,8 +723,8 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
 	string_list_clear(&elem_list, 0);
 
 	if (!info->name || !info->url) {
-		warning(_("server advertised a promisor remote without a name or URL: %s"),
-			remote_info);
+		warning(_("server advertised a promisor remote without a name or URL: '%s', "
+			  "ignoring this remote"), remote_info);
 		promisor_info_free(info);
 		return NULL;
 	}
-- 
2.54.0.rc0.114.g05d466edb8

Re: [PATCH v2 03/10] promisor-remote: clarify that a remote is ignored

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> In should_accept_remote() and parse_one_advertised_remote(), when a
> remote is ignored, we tell users why it is ignored in a warning, but we
> don't tell them that the remote is actually ignored.
>
> Let's clarify that, so users have a better idea of what's actually
> happening.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  promisor-remote.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

I agree that it makes sense to add the final disposition to the
message.  It would be even better to rephrase so that it, the most
important part of the message, comes first.  E.g.,

>
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 6c935f855a..8e062ec160 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,
>  		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
>  
>  	if (!remote_url || !*remote_url) {
> -		warning(_("no or empty URL advertised for remote '%s'"), remote_name);
> +		warning(_("no or empty URL advertised for remote '%s', "
> +			  "ignoring this remote"), remote_name);

I would find it easier to understand if it is phrased this way.

		warning(_("ignoring remote '%s' that advertises no usable URL"),
			remote_name);

i.e., conclusion first, the reason for the conclusion next.

Thanks.

[PATCH v2 04/10] promisor-remote: reject empty name or URL in advertised remote

[Christian Couder]

In parse_one_advertised_remote(), we check for a NULL remote name and
remote URL, but not for empty ones. An empty URL seems possible as
url_percent_decode("") doesn't return NULL.

In promisor_config_info_list(), we ignore remotes with empty URLs, so a
Git server should not advertise remotes with empty URLs. It's possible
that a buggy or malicious server would do it though.

So let's tighten the check in parse_one_advertised_remote() to also
reject empty strings at parse time.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8e062ec160..8322349ae8 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
 
 	string_list_clear(&elem_list, 0);
 
-	if (!info->name || !info->url) {
+	if (!info->name || !*info->name || !info->url || !*info->url) {
 		warning(_("server advertised a promisor remote without a name or URL: '%s', "
 			  "ignoring this remote"), remote_info);
 		promisor_info_free(info);
-- 
2.54.0.rc0.114.g05d466edb8

Re: [PATCH v2 04/10] promisor-remote: reject empty name or URL in advertised remote

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> In parse_one_advertised_remote(), we check for a NULL remote name and
> remote URL, but not for empty ones. An empty URL seems possible as
> url_percent_decode("") doesn't return NULL.
>
> In promisor_config_info_list(), we ignore remotes with empty URLs, so a
> Git server should not advertise remotes with empty URLs. It's possible
> that a buggy or malicious server would do it though.
>
> So let's tighten the check in parse_one_advertised_remote() to also
> reject empty strings at parse time.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  promisor-remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Personally, I am not enthused to see "NULL or empty", primarily
because it is an entry into a slippery slope.

Sure, an empty string is implausible, but would a single letter URL
a lot more plausible?  Not at all.  How about three letters?  Would
it be now a bit more plausible than an empty string?  Drawing the
line there between "" and "x" does not sound sensible.

Don't we have a code that _uses_ these URL strings to protect it
against a malformed or otherwise unusable URL already?  Can't we
rely on that working correctly to omit this check?

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 8e062ec160..8322349ae8 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
>  
>  	string_list_clear(&elem_list, 0);
>  
> -	if (!info->name || !info->url) {
> +	if (!info->name || !*info->name || !info->url || !*info->url) {
>  		warning(_("server advertised a promisor remote without a name or URL: '%s', "
>  			  "ignoring this remote"), remote_info);
>  		promisor_info_free(info);

[PATCH v2 05/10] promisor-remote: refactor should_accept_remote() control flow

[Christian Couder]

A previous commit made sure we now reject empty URLs early at parse
time. This makes the existing warning() in case a remote URL is NULL
or empty very unlikely to be useful.

In future work, we also plan to add URL-based acceptance logic into
should_accept_remote().

To adapt to previous changes and prepare for upcoming changes, let's
restructure the control flow in should_accept_remote().

Concretely, let's:

 - Replace the warning() in case of an empty URL with a BUG(), as a
   previous commit made sure empty URLs are rejected early at parse
   time.

 - Move that modified empty-URL check to the very top of the function,
   so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is
   covered.

 - Invert the URL comparison: instead of returning on match and
   warning on mismatch, return early on mismatch and let the match
   case fall through. This opens a single exit path at the bottom of
   the function for future commits to extend.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8322349ae8..5860a3d3f3 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -651,6 +651,11 @@ static int should_accept_remote(enum accept_promisor accept,
 	const char *remote_name = advertised->name;
 	const char *remote_url = advertised->url;
 
+	if (!remote_url || !*remote_url)
+		BUG("no or empty URL advertised for remote '%s'; "
+		    "this remote should have been rejected earlier",
+		    remote_name);
+
 	if (accept == ACCEPT_ALL)
 		return all_fields_match(advertised, config_info, NULL);
 
@@ -669,19 +674,14 @@ static int should_accept_remote(enum accept_promisor accept,
 	if (accept != ACCEPT_KNOWN_URL)
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
 
-	if (!remote_url || !*remote_url) {
-		warning(_("no or empty URL advertised for remote '%s', "
-			  "ignoring this remote"), remote_name);
+	if (strcmp(p->url, remote_url)) {
+		warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
+			  "ignoring this remote"),
+			remote_name, p->url, remote_url);
 		return 0;
 	}
 
-	if (!strcmp(p->url, remote_url))
-		return all_fields_match(advertised, config_info, p);
-
-	warning(_("known remote named '%s' but with URL '%s' instead of '%s', "
-		  "ignoring this remote"), remote_name, p->url, remote_url);
-
-	return 0;
+	return all_fields_match(advertised, config_info, p);
 }
 
 static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 06/10] promisor-remote: refactor has_control_char()

[Christian Couder]

In a future commit we are going to check if some strings contain
control characters, so let's refactor the logic to do that in a new
has_control_char() helper function.

It cleans up the code a bit anyway.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 5860a3d3f3..d60518f19c 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -642,6 +642,14 @@ static int all_fields_match(struct promisor_info *advertised,
 	return 1;
 }
 
+static bool has_control_char(const char *s)
+{
+	for (const char *c = s; *c; c++)
+		if (iscntrl(*c))
+			return true;
+	return false;
+}
+
 static int should_accept_remote(enum accept_promisor accept,
 				struct promisor_info *advertised,
 				struct string_list *config_info)
@@ -772,18 +780,14 @@ static bool valid_filter(const char *filter, const char *remote_name)
 	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)) {
-			warning(_("invalid token '%s' for remote '%s' "
-				  "will not be stored"),
-				token, remote_name);
-			return false;
-		}
+	if (has_control_char(token)) {
+		warning(_("invalid token '%s' for remote '%s' "
+			  "will not be stored"),
+			token, remote_name);
+		return false;
+	}
 
 	return true;
 }
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 07/10] promisor-remote: refactor accept_from_server()

[Christian Couder]

In future commits, we are going to add more logic to
filter_promisor_remote() which is already doing a lot of things.

Let's alleviate that by moving the logic that checks and validates the
value of the `promisor.acceptFromServer` config variable into its own
accept_from_server() helper function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index d60518f19c..8d80ef6040 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -862,20 +862,12 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised,
 	return reload_config;
 }
 
-static void filter_promisor_remote(struct repository *repo,
-				   struct strvec *accepted,
-				   const char *info)
+static enum accept_promisor accept_from_server(struct repository *repo)
 {
 	const char *accept_str;
 	enum accept_promisor accept = ACCEPT_NONE;
-	struct string_list config_info = STRING_LIST_INIT_NODUP;
-	struct string_list remote_info = STRING_LIST_INIT_DUP;
-	struct store_info *store_info = NULL;
-	struct string_list_item *item;
-	bool reload_config = false;
-	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
 
-	if (!repo_config_get_string_tmp(the_repository, "promisor.acceptfromserver", &accept_str)) {
+	if (!repo_config_get_string_tmp(repo, "promisor.acceptfromserver", &accept_str)) {
 		if (!*accept_str || !strcasecmp("None", accept_str))
 			accept = ACCEPT_NONE;
 		else if (!strcasecmp("KnownUrl", accept_str))
@@ -889,6 +881,21 @@ static void filter_promisor_remote(struct repository *repo,
 				accept_str, "promisor.acceptfromserver");
 	}
 
+	return accept;
+}
+
+static void filter_promisor_remote(struct repository *repo,
+				   struct strvec *accepted,
+				   const char *info)
+{
+	struct string_list config_info = STRING_LIST_INIT_NODUP;
+	struct string_list remote_info = STRING_LIST_INIT_DUP;
+	struct store_info *store_info = NULL;
+	struct string_list_item *item;
+	bool reload_config = false;
+	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
+	enum accept_promisor accept = accept_from_server(repo);
+
 	if (accept == ACCEPT_NONE)
 		return;
 
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 08/10] promisor-remote: keep accepted promisor_info structs alive

[Christian Couder]

In filter_promisor_remote(), the instances of `struct promisor_info`
for accepted remotes are dismantled into separate parallel data
structures (the 'accepted' strvec for server names, and
'accepted_filters' for filter strings) and then immediately freed.

Instead, let's keep these instances on an 'accepted_remotes' list.

This way the post-loop phase can iterate a single list to build the
protocol reply, apply advertised filters, and mark remotes as
accepted, rather than iterating three separate structures.

This refactoring also prepares for a future commit that will add a
'local_name' member to 'struct promisor_info'. Since struct instances
stay alive, downstream code will be able to simply read both names
from them rather than needing yet another parallel strvec.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 8d80ef6040..74e65e9dd0 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -890,10 +890,10 @@ static void filter_promisor_remote(struct repository *repo,
 {
 	struct string_list config_info = STRING_LIST_INIT_NODUP;
 	struct string_list remote_info = STRING_LIST_INIT_DUP;
+	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 	struct store_info *store_info = NULL;
 	struct string_list_item *item;
 	bool reload_config = false;
-	struct string_list accepted_filters = STRING_LIST_INIT_DUP;
 	enum accept_promisor accept = accept_from_server(repo);
 
 	if (accept == ACCEPT_NONE)
@@ -922,17 +922,10 @@ static void filter_promisor_remote(struct repository *repo,
 			if (promisor_store_advertised_fields(advertised, store_info))
 				reload_config = true;
 
-			strvec_push(accepted, advertised->name);
-
-			/* Capture advertised filters for accepted remotes */
-			if (advertised->filter) {
-				struct string_list_item *i;
-				i = string_list_append(&accepted_filters, advertised->name);
-				i->util = xstrdup(advertised->filter);
-			}
+			string_list_append(&accepted_remotes, advertised->name)->util = advertised;
+		} else {
+			promisor_info_free(advertised);
 		}
-
-		promisor_info_free(advertised);
 	}
 
 	promisor_info_list_clear(&config_info);
@@ -942,24 +935,23 @@ static void filter_promisor_remote(struct repository *repo,
 	if (reload_config)
 		repo_promisor_remote_reinit(repo);
 
-	/* Apply accepted remote filters to the stable repo state */
-	for_each_string_list_item(item, &accepted_filters) {
-		struct promisor_remote *r = repo_promisor_remote_find(repo, item->string);
-		if (r) {
-			free(r->advertised_filter);
-			r->advertised_filter = item->util;
-			item->util = NULL;
-		}
-	}
+	/* Apply accepted remotes to the stable repo state */
+	for_each_string_list_item(item, &accepted_remotes) {
+		struct promisor_info *info = item->util;
+		struct promisor_remote *r = repo_promisor_remote_find(repo, info->name);
 
-	string_list_clear(&accepted_filters, 1);
+		strvec_push(accepted, info->name);
 
-	/* Mark the remotes as accepted in the repository state */
-	for (size_t i = 0; i < accepted->nr; i++) {
-		struct promisor_remote *r = repo_promisor_remote_find(repo, accepted->v[i]);
-		if (r)
+		if (r) {
 			r->accepted = 1;
+			if (info->filter) {
+				free(r->advertised_filter);
+				r->advertised_filter = xstrdup(info->filter);
+			}
+		}
 	}
+
+	promisor_info_list_clear(&accepted_remotes);
 }
 
 void promisor_remote_reply(const char *info, char **accepted_out)
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 09/10] promisor-remote: remove the 'accepted' strvec

[Christian Couder]

In a previous commit, filter_promisor_remote() was refactored to keep
accepted 'struct promisor_info' instances alive instead of dismantling
them into separate parallel data structures.

Let's go one step further and replace the 'struct strvec *accepted'
argument passed to filter_promisor_remote() with a
'struct string_list *accepted_remotes' argument.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 74e65e9dd0..38fa050542 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -885,12 +885,11 @@ static enum accept_promisor accept_from_server(struct repository *repo)
 }
 
 static void filter_promisor_remote(struct repository *repo,
-				   struct strvec *accepted,
+				   struct string_list *accepted_remotes,
 				   const char *info)
 {
 	struct string_list config_info = STRING_LIST_INIT_NODUP;
 	struct string_list remote_info = STRING_LIST_INIT_DUP;
-	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 	struct store_info *store_info = NULL;
 	struct string_list_item *item;
 	bool reload_config = false;
@@ -922,7 +921,7 @@ static void filter_promisor_remote(struct repository *repo,
 			if (promisor_store_advertised_fields(advertised, store_info))
 				reload_config = true;
 
-			string_list_append(&accepted_remotes, advertised->name)->util = advertised;
+			string_list_append(accepted_remotes, advertised->name)->util = advertised;
 		} else {
 			promisor_info_free(advertised);
 		}
@@ -936,12 +935,10 @@ static void filter_promisor_remote(struct repository *repo,
 		repo_promisor_remote_reinit(repo);
 
 	/* Apply accepted remotes to the stable repo state */
-	for_each_string_list_item(item, &accepted_remotes) {
+	for_each_string_list_item(item, accepted_remotes) {
 		struct promisor_info *info = item->util;
 		struct promisor_remote *r = repo_promisor_remote_find(repo, info->name);
 
-		strvec_push(accepted, info->name);
-
 		if (r) {
 			r->accepted = 1;
 			if (info->filter) {
@@ -950,23 +947,23 @@ static void filter_promisor_remote(struct repository *repo,
 			}
 		}
 	}
-
-	promisor_info_list_clear(&accepted_remotes);
 }
 
 void promisor_remote_reply(const char *info, char **accepted_out)
 {
-	struct strvec accepted = STRVEC_INIT;
+	struct string_list accepted_remotes = STRING_LIST_INIT_NODUP;
 
-	filter_promisor_remote(the_repository, &accepted, info);
+	filter_promisor_remote(the_repository, &accepted_remotes, info);
 
 	if (accepted_out) {
-		if (accepted.nr) {
+		if (accepted_remotes.nr) {
 			struct strbuf reply = STRBUF_INIT;
-			for (size_t i = 0; i < accepted.nr; i++) {
-				if (i)
+			struct string_list_item *item;
+
+			for_each_string_list_item(item, &accepted_remotes) {
+				if (reply.len)
 					strbuf_addch(&reply, ';');
-				strbuf_addstr_urlencode(&reply, accepted.v[i], allow_unsanitized);
+				strbuf_addstr_urlencode(&reply, item->string, allow_unsanitized);
 			}
 			*accepted_out = strbuf_detach(&reply, NULL);
 		} else {
@@ -974,7 +971,7 @@ void promisor_remote_reply(const char *info, char **accepted_out)
 		}
 	}
 
-	strvec_clear(&accepted);
+	promisor_info_list_clear(&accepted_remotes);
 }
 
 void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes)
-- 
2.54.0.rc0.114.g05d466edb8

[PATCH v2 10/10] t5710: use proper file:// URIs for absolute paths

[Christian Couder]

In t5710, we frequently construct local file URIs using `file://$(pwd)`.
On Unix-like systems, $(pwd) returns an absolute path starting with a
slash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an
empty host (`file:///tmp/repo`).

However, on Windows, $(pwd) returns a path starting with a drive
letter (e.g., `D:/a/repo`). This results in a 2-slash URI
(`file://D:/a/repo`). Standard URI parsers misinterpret this format,
treating `D:` as the host rather than part of the absolute path.

This is to be expected because RFC 8089 says that the `//` prefix with
an empty local host must be followed by an absolute path starting with
a slash.

While this hasn't broken the existing tests (because the old
`promisor.acceptFromServer` logic relies entirely on strict `strcmp()`
without normalizing the URLs), it will break future commits that pass
these URLs through `url_normalize()` or similar functions.

To future-proof the tests and ensure cross-platform URI compliance,
let's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly
guarantees a leading slash for the path component, ensuring valid
3-slash `file:///` URIs on all operating systems.

While at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to
handle some common special characters in directory paths.

To be extra safe, let's skip all the tests if there are uncommon
special characters in the directory path.

Then let's replace all instances of `file://$(pwd)` with
$TRASH_DIRECTORY_URL across the test script, and let's simplify the
`sendFields` and `checkFields` tests to use
$ENCODED_TRASH_DIRECTORY_URL directly.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t5710-promisor-remote-capability.sh | 79 +++++++++++++++++----------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index bf0eed9f10..b404ad9f0a 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -76,6 +76,31 @@ copy_to_lop () {
 	cp "$path" "$path2"
 }
 
+# On Windows, `pwd` returns a path like 'D:/foo/bar'. Prepend '/' to turn
+# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.
+# On Unix, the path already starts with '/', so this is a no-op.
+pwd_path=$(pwd)
+case "$pwd_path" in
+[a-zA-Z]:*) pwd_path="/$pwd_path" ;;
+esac
+
+# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),
+# and those percent-encoded below (% space = , ;)
+rest=$(printf "%s" "$pwd_path" | tr -d 'a-zA-Z0-9_.~/:% =,;-')
+if test -n "$rest"
+then
+	skip_all="PWD contains unsupported special characters"
+	test_done
+fi
+
+TRASH_DIRECTORY_URL="file://$pwd_path"
+
+encoded_path=$(printf "%s" "$pwd_path" |
+	       sed -e 's/%/%25/g' -e 's/ /%20/g' -e 's/=/%3D/g' \
+		   -e 's/;/%3B/g' -e 's/,/%2C/g')
+
+ENCODED_TRASH_DIRECTORY_URL="file://$encoded_path"
+
 test_expect_success "setup for testing promisor remote advertisement" '
 	# Create another bare repo called "lop" (for Large Object Promisor)
 	git init --bare lop &&
@@ -88,7 +113,7 @@ test_expect_success "setup for testing promisor remote advertisement" '
 	initialize_server 1 "$oid" &&
 
 	# Configure lop as promisor remote for server
-	git -C server remote add lop "file://$(pwd)/lop" &&
+	git -C server remote add lop "$TRASH_DIRECTORY_URL/lop" &&
 	git -C server config remote.lop.promisor true &&
 
 	git -C lop config uploadpack.allowFilter true &&
@@ -104,7 +129,7 @@ test_expect_success "clone with promisor.advertise set to 'true'" '
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -119,7 +144,7 @@ test_expect_success "clone with promisor.advertise set to 'false'" '
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -137,7 +162,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=None \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -156,8 +181,8 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C client init &&
 	git -C client config remote.lop.promisor true &&
 	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
-	git -C client config remote.lop.url "file://$(pwd)/lop" &&
-	git -C client config remote.server.url "file://$(pwd)/server" &&
+	git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" &&
+	git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" &&
 	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
 	git -C client config promisor.acceptfromserver All &&
 	GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
@@ -177,10 +202,10 @@ test_expect_success "clone with two promisors but only one advertised" '
 	GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
 		-c remote.unused_lop.promisor=true \
 		-c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
-		-c remote.unused_lop.url="file://$(pwd)/unused_lop" \
+		-c remote.unused_lop.url="$TRASH_DIRECTORY_URL/unused_lop" \
 		-c remote.lop.promisor=true \
 		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
-		-c remote.lop.url="file://$(pwd)/lop" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -210,11 +235,11 @@ test_expect_success "init + fetch two promisors but only one advertised" '
 	git -C client init &&
 	git -C client config remote.unused_lop.promisor true &&
 	git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
-	git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
+	git -C client config remote.unused_lop.url "$TRASH_DIRECTORY_URL/unused_lop" &&
 	git -C client config remote.lop.promisor true &&
 	git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
-	git -C client config remote.lop.url "file://$(pwd)/lop" &&
-	git -C client config remote.server.url "file://$(pwd)/server" &&
+	git -C client config remote.lop.url "$TRASH_DIRECTORY_URL/lop" &&
+	git -C client config remote.server.url "$TRASH_DIRECTORY_URL/server" &&
 	git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
 	git -C client config promisor.acceptfromserver All &&
 
@@ -242,7 +267,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=KnownName \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -257,7 +282,7 @@ test_expect_success "clone with 'KnownName' and different remote names" '
 	# Clone from server to create a client
 	GIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \
 		-c remote.serverTwo.fetch="+refs/heads/*:refs/remotes/lop/*" \
-		-c remote.serverTwo.url="file://$(pwd)/lop" \
+		-c remote.serverTwo.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=KnownName \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -294,7 +319,7 @@ test_expect_success "clone with promisor.acceptfromserver set to 'KnownUrl'" '
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -311,7 +336,7 @@ test_expect_success "clone with 'KnownUrl' and different remote urls" '
 	# Clone from server to create a client
 	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)/serverTwo" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/serverTwo" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -326,7 +351,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server"
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
 
-	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
+	test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" &&
 	git -C server config unset remote.lop.url &&
 
 	# Clone from server to create a client
@@ -335,7 +360,7 @@ test_expect_success "clone with 'KnownUrl' and url not configured on the server"
 	# missing, so the remote name will be used instead which will fail.
 	test_must_fail env 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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -347,7 +372,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
 
-	test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" &&
+	test_when_finished "git -C server config set remote.lop.url \"$TRASH_DIRECTORY_URL/lop\"" &&
 	git -C server config set remote.lop.url "" &&
 
 	# Clone from server to create a client
@@ -356,7 +381,7 @@ test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" '
 	# so the remote name will be used instead which will fail.
 	test_must_fail env 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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=KnownUrl \
 		--no-local --filter="blob:limit=5k" server client &&
 
@@ -380,13 +405,12 @@ test_expect_success "clone with promisor.sendFields" '
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
 	# Check that fields are properly transmitted
-	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
-	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+	PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" &&
 	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
 	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
 	test_grep "clone> promisor-remote=lop;otherLop" trace &&
@@ -411,15 +435,14 @@ test_expect_success "clone with promisor.checkFields" '
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c remote.lop.partialCloneFilter="blob:none" \
 		-c promisor.acceptfromserver=All \
 		-c promisor.checkFields=partialcloneFilter \
 		--no-local --filter="blob:limit=5k" server client &&
 
 	# Check that fields are properly transmitted
-	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
-	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+	PR1="name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none" &&
 	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
 	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
 	test_grep "clone> promisor-remote=lop" trace &&
@@ -449,7 +472,7 @@ test_expect_success "clone with promisor.storeFields=partialCloneFilter" '
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c remote.lop.token="fooYYY" \
 		-c remote.lop.partialCloneFilter="blob:none" \
 		-c promisor.acceptfromserver=All \
@@ -501,7 +524,7 @@ test_expect_success "clone and fetch with --filter=auto" '
 
 	GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
 		-c remote.lop.promisor=true \
-		-c remote.lop.url="file://$(pwd)/lop" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter=auto server client 2>err &&
 
@@ -558,7 +581,7 @@ test_expect_success "clone with promisor.advertise set to 'true' but don't delet
 	# Clone from server to create a client
 	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.url="$TRASH_DIRECTORY_URL/lop" \
 		-c promisor.acceptfromserver=All \
 		--no-local --filter="blob:limit=5k" server client &&
 
-- 
2.54.0.rc0.114.g05d466edb8