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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

On Thu, Apr 02, 2026 at 09:06:04AM +0200, Christian Couder wrote:

Makes sense, thanks for adding this blurb.

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

Patrick

On Thu, Apr 02, 2026 at 09:06:07AM +0200, Christian Couder wrote:

Makes sense.

Patrick

On Thu, Apr 02, 2026 at 09:06:03AM +0200, Christian Couder wrote:

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

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

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.

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

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

On Thu, Apr 2, 2026 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:

Right. I will use something like the following then:

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

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')

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

On Thu, Apr 2, 2026 at 8:37 PM Junio C Hamano <gitster@pobox.com> wrote:

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

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

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

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

On Thu, Apr 2, 2026 at 9:46 AM Patrick Steinhardt <ps@pks.im> wrote:

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.

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

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.

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

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.

Very well done.

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

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

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.

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

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?