{"thread":{"id":"65413","subject":"[PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist","startedAt":"2026-04-02T07:06:37Z","lastAt":"2026-04-07T17:36:47Z","messageCount":35,"participants":["Christian Couder","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"540717","messageId":"20260402070613.85934-1-christian.couder@gmail.com","threadId":"65413","inReplyTo":null,"subject":"[PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:03Z","receivedAt":"2026-04-02T07:06:37Z","isPatch":true,"body":"Recently, I sent a 16 patch long series that makes it possible for\npromisor remotes advertised through the \"promisor-remote\" protocol\ncapability to be auto-configured on the client side via a URL\nallowlist configured using a new `promisor.acceptFromServerUrl`\nconfiguration variable:\n\nhttps://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/\n\nI got the suggestion to split the 16 patches series into two smaller\nseries, starting with a preparatory series. So here is this\npreparatory series. It's a mix of mostly small fixes, refactorings and\ncleanups.\n\nHigh level description of the patches\n=====================================\n\n - Patches 1-4/10 are fixes:\n\n - Patch 1/10 is the most significant. The others are relatively\n small.\n \n - Patch 4/10 is the only new in this series (while all the others\n were in the previous series). It prepares for Patch 5/10.\n\n - Patches 5-10/10 are refactorings and cleanups:\n\n - Patches 5-7/10 are relatively small independents cleanups or\n refactorings.\n\n - Patches 8-9/10 are related refactorings simplifying the data\n structures used in filter_promisor_remote() and\n promisor_remote_reply().\n\n - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths\n in the test script.\n\nCI tests\n========\n\nThey all pass, see:\n\nhttps://github.com/chriscool/git/actions/runs/23848484597\n\nRange-diff\n==========\n\nSorry, no range-diff as I don't think it would be quite useful because\nthe number and order of commits has changed a lot.\n\nChristian Couder (10):\n promisor-remote: try accepted remotes before others in get_direct()\n promisor-remote: pass config entry to all_fields_match() directly\n promisor-remote: clarify that a remote is ignored\n promisor-remote: reject empty name or URL in advertised remote\n promisor-remote: refactor should_accept_remote() control flow\n promisor-remote: refactor has_control_char()\n promisor-remote: refactor accept_from_server()\n promisor-remote: keep accepted promisor_info structs alive\n promisor-remote: remove the 'accepted' strvec\n t5710: use proper file:// URIs for absolute paths\n\n Documentation/gitprotocol-v2.adoc | 4 +\n promisor-remote.c | 207 +++++++++++++++-----------\n t/t5710-promisor-remote-capability.sh | 122 ++++++++++++---\n 3 files changed, 223 insertions(+), 110 deletions(-)\n\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540718","messageId":"20260402070613.85934-2-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:04Z","receivedAt":"2026-04-02T07:06:38Z","isPatch":true,"body":"When a server advertises promisor remotes and the client accepts some\nof them, those remotes carry the server's intent: 'fetch missing\nobjects preferably from here', and the client agrees with that for the\nremotes it accepts.\n\nHowever promisor_remote_get_direct() actually iterates over all\npromisor remotes in list order, which is the order they appear in the\nconfig files (except perhaps for the one appearing in the\n`extensions.partialClone` config variable which is tried last).\n\nThis means an existing, but not accepted, promisor remote, could be\ntried before the accepted ones, which does not reflect the intent of\nthe agreement between client and server.\n\nIf the client doesn't care about what the server suggests, it should\naccept nothing and rely on its remotes as they are already configured.\n\nTo better reflect the agreement between client and server, let's make\npromisor_remote_get_direct() try the accepted promisor remotes before\nthe non-accepted ones.\n\nConcretely, let's extract a try_promisor_remotes() helper and call it\ntwice from promisor_remote_get_direct():\n\n- first with an `accepted_only=true` argument to try only the accepted\n remotes,\n- then with `accepted_only=false` to fall back to any remaining remote.\n\nEnsuring that accepted remotes are preferred will be even more\nimportant if in the future a mechanism is developed to allow the\nclient to auto-configure remotes that the server advertises. This will\nin particular avoid fetching from the server (which is already\nconfigured as a promisor remote) before trying the auto-configured\nremotes, as these new remotes would likely appear at the end of the\nconfig file, and as the server might not appear in the\n`extensions.partialClone` config variable.\n\nSigned-off-by: Christian Couder \n---\n Documentation/gitprotocol-v2.adoc | 4 ++\n promisor-remote.c | 44 ++++++++++++-----\n t/t5710-promisor-remote-capability.sh | 69 +++++++++++++++++++++++++++\n 3 files changed, 104 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc\nindex f985cb4c47..4fcb1a7bda 100644\n--- a/Documentation/gitprotocol-v2.adoc\n+++ b/Documentation/gitprotocol-v2.adoc\n@@ -848,6 +848,10 @@ advertised, it can reply with \"promisor-remote=\" where\n where `pr-name` is the urlencoded name of a promisor remote the server\n advertised and the client accepts.\n \n+The promisor remotes that the client accepted will be tried before the\n+other configured promisor remotes when the client attempts to fetch\n+missing objects.\n+\n Note that, everywhere in this document, the ';' and ',' characters\n MUST be encoded if they appear in `pr-name` or `field-value`.\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 96fa215b06..7ce7d22f95 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n }\n \n+static int try_promisor_remotes(struct repository *repo,\n+\t\t\t\tstruct object_id **remaining_oids,\n+\t\t\t\tint *remaining_nr, int *to_free,\n+\t\t\t\tbool accepted_only)\n+{\n+\tstruct promisor_remote *r = repo->promisor_remote_config->promisors;\n+\n+\tfor (; r; r = r->next) {\n+\t\tif (accepted_only != r->accepted)\n+\t\t\tcontinue;\n+\t\tif (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {\n+\t\t\tif (*remaining_nr == 1)\n+\t\t\t\tcontinue;\n+\t\t\t*remaining_nr = remove_fetched_oids(repo, remaining_oids,\n+\t\t\t\t\t\t\t *remaining_nr, *to_free);\n+\t\t\tif (*remaining_nr) {\n+\t\t\t\t*to_free = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\t\treturn 1; /* all fetched */\n+\t}\n+\treturn 0;\n+}\n+\n void promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tconst struct object_id *oids,\n \t\t\t\tint oid_nr)\n {\n-\tstruct promisor_remote *r;\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n@@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo,\n \n \tpromisor_remote_init(repo);\n \n-\tfor (r = repo->promisor_remote_config->promisors; r; r = r->next) {\n-\t\tif (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) {\n-\t\t\tif (remaining_nr == 1)\n-\t\t\t\tcontinue;\n-\t\t\tremaining_nr = remove_fetched_oids(repo, &remaining_oids,\n-\t\t\t\t\t\t\t remaining_nr, to_free);\n-\t\t\tif (remaining_nr) {\n-\t\t\t\tto_free = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n+\t/* Try accepted remotes first (those the server told us to use) */\n+\tif (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,\n+\t\t\t\t &to_free, true))\n+\t\tgoto all_fetched;\n+\tif (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,\n+\t\t\t\t &to_free, false))\n \t\tgoto all_fetched;\n-\t}\n \n \tfor (i = 0; i < remaining_nr; i++) {\n \t\tif (is_promisor_object(repo, &remaining_oids[i]))\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex 357822c01a..bf0eed9f10 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -166,6 +166,75 @@ test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n \tcheck_missing_objects server 1 \"$oid\"\n '\n \n+test_expect_success \"clone with two promisors but only one advertised\" '\n+\tgit -C server config promisor.advertise true &&\n+\ttest_when_finished \"rm -rf client unused_lop\" &&\n+\n+\t# Create a promisor that will be configured but not be used\n+\tgit init --bare unused_lop &&\n+\n+\t# Clone from server to create a client\n+\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n+\t\t-c remote.unused_lop.promisor=true \\\n+\t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n+\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n+\t\t-c remote.lop.promisor=true \\\n+\t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n+\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c promisor.acceptfromserver=All \\\n+\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n+\n+\t# Check that \"unused_lop\" appears before \"lop\" in the config\n+\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" \"origin\" >expect &&\n+\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Check that \"lop\" was tried\n+\ttest_grep \" fetch lop \" trace &&\n+\t# Check that \"unused_lop\" was not contacted\n+\t# This means \"lop\", the accepted promisor, was tried first\n+\ttest_grep ! \" fetch unused_lop \" trace &&\n+\n+\t# Check that the largest object is still missing on the server\n+\tcheck_missing_objects server 1 \"$oid\"\n+'\n+\n+test_expect_success \"init + fetch two promisors but only one advertised\" '\n+\tgit -C server config promisor.advertise true &&\n+\ttest_when_finished \"rm -rf client unused_lop\" &&\n+\n+\t# Create a promisor that will be configured but not be used\n+\tgit init --bare unused_lop &&\n+\n+\tmkdir client &&\n+\tgit -C client init &&\n+\tgit -C client config remote.unused_lop.promisor true &&\n+\tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n+\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n+\tgit -C client config remote.lop.promisor true &&\n+\tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n+\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n+\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n+\tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n+\tgit -C client config promisor.acceptfromserver All &&\n+\n+\t# Check that \"unused_lop\" appears before \"lop\" in the config\n+\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" >expect &&\n+\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n+\n+\t# Check that \"lop\" was tried\n+\ttest_grep \" fetch lop \" trace &&\n+\t# Check that \"unused_lop\" was not contacted\n+\t# This means \"lop\", the accepted promisor, was tried first\n+\ttest_grep ! \" fetch unused_lop \" trace &&\n+\n+\t# Check that the largest object is still missing on the server\n+\tcheck_missing_objects server 1 \"$oid\"\n+'\n+\n test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540719","messageId":"20260402070613.85934-3-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 02/10] promisor-remote: pass config entry to all_fields_match() directly","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:05Z","receivedAt":"2026-04-02T07:06:39Z","isPatch":true,"body":"The `in_list == 0` path of all_fields_match() looks up the remote in\n`config_info` by `advertised->name` repeatedly, even though every\ncaller in should_accept_remote() has already performed this\nlookup and holds the result in `p`.\n\nTo avoid this useless work, let's replace the `int in_list`\nparameter with a `struct promisor_info *config_entry` pointer:\n\n - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as\n the old `in_list == 1` path did.\n\n - When non-NULL: match against that single config entry directly,\n avoiding the redundant string_list_lookup() call.\n\nThis removes the hidden dependency on `advertised->name` inside\nall_fields_match(), which would be wrong if in the future\nauto-configured remotes are implemented, as the local config name may\ndiffer from the server's advertised name.\n\nWhile at it, let's also add a comment before all_fields_match() and\nmatch_field_against_config() to help understand how things work and\nhelp avoid similar issues.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 36 ++++++++++++++++++++++++------------\n 1 file changed, 24 insertions(+), 12 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 7ce7d22f95..6c935f855a 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -575,6 +575,12 @@ enum accept_promisor {\n \tACCEPT_ALL\n };\n \n+/*\n+ * Check if a specific field and its advertised value match the local\n+ * configuration of a given promisor remote.\n+ *\n+ * Returns 1 if they match, 0 otherwise.\n+ */\n static int match_field_against_config(const char *field, const char *value,\n \t\t\t\t struct promisor_info *config_info)\n {\n@@ -586,9 +592,18 @@ static int match_field_against_config(const char *field, const char *value,\n \treturn 0;\n }\n \n+/*\n+ * Check that the advertised fields match the local configuration.\n+ *\n+ * When 'config_entry' is NULL (ACCEPT_ALL mode), every checked field\n+ * must match at least one remote in 'config_info'.\n+ *\n+ * When 'config_entry' points to a specific remote's config, the\n+ * checked fields are compared against that single remote only.\n+ */\n static int all_fields_match(struct promisor_info *advertised,\n \t\t\t struct string_list *config_info,\n-\t\t\t int in_list)\n+\t\t\t struct promisor_info *config_entry)\n {\n \tstruct string_list *fields = fields_checked();\n \tstruct string_list_item *item_checked;\n@@ -597,7 +612,6 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\tint match = 0;\n \t\tconst char *field = item_checked->string;\n \t\tconst char *value = NULL;\n-\t\tstruct string_list_item *item;\n \n \t\tif (!strcasecmp(field, promisor_field_filter))\n \t\t\tvalue = advertised->filter;\n@@ -607,7 +621,11 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\tif (!value)\n \t\t\treturn 0;\n \n-\t\tif (in_list) {\n+\t\tif (config_entry) {\n+\t\t\tmatch = match_field_against_config(field, value,\n+\t\t\t\t\t\t\t config_entry);\n+\t\t} else {\n+\t\t\tstruct string_list_item *item;\n \t\t\tfor_each_string_list_item(item, config_info) {\n \t\t\t\tstruct promisor_info *p = item->util;\n \t\t\t\tif (match_field_against_config(field, value, p)) {\n@@ -615,12 +633,6 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n-\t\t} else {\n-\t\t\titem = string_list_lookup(config_info, advertised->name);\n-\t\t\tif (item) {\n-\t\t\t\tstruct promisor_info *p = item->util;\n-\t\t\t\tmatch = match_field_against_config(field, value, p);\n-\t\t\t}\n \t\t}\n \n \t\tif (!match)\n@@ -640,7 +652,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_url = advertised->url;\n \n \tif (accept == ACCEPT_ALL)\n-\t\treturn all_fields_match(advertised, config_info, 1);\n+\t\treturn all_fields_match(advertised, config_info, NULL);\n \n \t/* Get config info for that promisor remote */\n \titem = string_list_lookup(config_info, remote_name);\n@@ -652,7 +664,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \tp = item->util;\n \n \tif (accept == ACCEPT_KNOWN_NAME)\n-\t\treturn all_fields_match(advertised, config_info, 0);\n+\t\treturn all_fields_match(advertised, config_info, p);\n \n \tif (accept != ACCEPT_KNOWN_URL)\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n@@ -663,7 +675,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n-\t\treturn all_fields_match(advertised, config_info, 0);\n+\t\treturn all_fields_match(advertised, config_info, p);\n \n \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n \t\tremote_name, p->url, remote_url);\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540720","messageId":"20260402070613.85934-4-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 03/10] promisor-remote: clarify that a remote is ignored","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:06Z","receivedAt":"2026-04-02T07:06:40Z","isPatch":true,"body":"In should_accept_remote() and parse_one_advertised_remote(), when a\nremote is ignored, we tell users why it is ignored in a warning, but we\ndon't tell them that the remote is actually ignored.\n\nLet's clarify that, so users have a better idea of what's actually\nhappening.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 6c935f855a..8e062ec160 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n \n \tif (!remote_url || !*remote_url) {\n-\t\twarning(_(\"no or empty URL advertised for remote '%s'\"), remote_name);\n+\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n+\t\t\t \"ignoring this remote\"), remote_name);\n \t\treturn 0;\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n \t\treturn all_fields_match(advertised, config_info, p);\n \n-\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n-\t\tremote_name, p->url, remote_url);\n+\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n+\t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n \n \treturn 0;\n }\n@@ -722,8 +723,8 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n \tstring_list_clear(&elem_list, 0);\n \n \tif (!info->name || !info->url) {\n-\t\twarning(_(\"server advertised a promisor remote without a name or URL: %s\"),\n-\t\t\tremote_info);\n+\t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n+\t\t\t \"ignoring this remote\"), remote_info);\n \t\tpromisor_info_free(info);\n \t\treturn NULL;\n \t}\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540721","messageId":"20260402070613.85934-5-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 04/10] promisor-remote: reject empty name or URL in advertised remote","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:07Z","receivedAt":"2026-04-02T07:06:41Z","isPatch":true,"body":"In parse_one_advertised_remote(), we check for a NULL remote name and\nremote URL, but not for empty ones. An empty URL seems possible as\nurl_percent_decode(\"\") doesn't return NULL.\n\nIn promisor_config_info_list(), we ignore remotes with empty URLs, so a\nGit server should not advertise remotes with empty URLs. It's possible\nthat a buggy or malicious server would do it though.\n\nSo let's tighten the check in parse_one_advertised_remote() to also\nreject empty strings at parse time.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8e062ec160..8322349ae8 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n \n \tstring_list_clear(&elem_list, 0);\n \n-\tif (!info->name || !info->url) {\n+\tif (!info->name || !*info->name || !info->url || !*info->url) {\n \t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n \t\t\t \"ignoring this remote\"), remote_info);\n \t\tpromisor_info_free(info);\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540722","messageId":"20260402070613.85934-6-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 05/10] promisor-remote: refactor should_accept_remote() control flow","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:08Z","receivedAt":"2026-04-02T07:06:43Z","isPatch":true,"body":"A previous commit made sure we now reject empty URLs early at parse\ntime. This makes the existing warning() in case a remote URL is NULL\nor empty very unlikely to be useful.\n\nIn future work, we also plan to add URL-based acceptance logic into\nshould_accept_remote().\n\nTo adapt to previous changes and prepare for upcoming changes, let's\nrestructure the control flow in should_accept_remote().\n\nConcretely, let's:\n\n - Replace the warning() in case of an empty URL with a BUG(), as a\n previous commit made sure empty URLs are rejected early at parse\n time.\n\n - Move that modified empty-URL check to the very top of the function,\n so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is\n covered.\n\n - Invert the URL comparison: instead of returning on match and\n warning on mismatch, return early on mismatch and let the match\n case fall through. This opens a single exit path at the bottom of\n the function for future commits to extend.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8322349ae8..5860a3d3f3 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -651,6 +651,11 @@ static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_name = advertised->name;\n \tconst char *remote_url = advertised->url;\n \n+\tif (!remote_url || !*remote_url)\n+\t\tBUG(\"no or empty URL advertised for remote '%s'; \"\n+\t\t \"this remote should have been rejected earlier\",\n+\t\t remote_name);\n+\n \tif (accept == ACCEPT_ALL)\n \t\treturn all_fields_match(advertised, config_info, NULL);\n \n@@ -669,19 +674,14 @@ static int should_accept_remote(enum accept_promisor accept,\n \tif (accept != ACCEPT_KNOWN_URL)\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n \n-\tif (!remote_url || !*remote_url) {\n-\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n-\t\t\t \"ignoring this remote\"), remote_name);\n+\tif (strcmp(p->url, remote_url)) {\n+\t\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n+\t\t\t \"ignoring this remote\"),\n+\t\t\tremote_name, p->url, remote_url);\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(p->url, remote_url))\n-\t\treturn all_fields_match(advertised, config_info, p);\n-\n-\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n-\t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n-\n-\treturn 0;\n+\treturn all_fields_match(advertised, config_info, p);\n }\n \n static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540723","messageId":"20260402070613.85934-7-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 06/10] promisor-remote: refactor has_control_char()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:09Z","receivedAt":"2026-04-02T07:06:44Z","isPatch":true,"body":"In a future commit we are going to check if some strings contain\ncontrol characters, so let's refactor the logic to do that in a new\nhas_control_char() helper function.\n\nIt cleans up the code a bit anyway.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 24 ++++++++++++++----------\n 1 file changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 5860a3d3f3..d60518f19c 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -642,6 +642,14 @@ static int all_fields_match(struct promisor_info *advertised,\n \treturn 1;\n }\n \n+static bool has_control_char(const char *s)\n+{\n+\tfor (const char *c = s; *c; c++)\n+\t\tif (iscntrl(*c))\n+\t\t\treturn true;\n+\treturn false;\n+}\n+\n static int should_accept_remote(enum accept_promisor accept,\n \t\t\t\tstruct promisor_info *advertised,\n \t\t\t\tstruct string_list *config_info)\n@@ -772,18 +780,14 @@ static bool valid_filter(const char *filter, const char *remote_name)\n \treturn !res;\n }\n \n-/* Check that a token doesn't contain any control character */\n static bool valid_token(const char *token, const char *remote_name)\n {\n-\tconst char *c = token;\n-\n-\tfor (; *c; c++)\n-\t\tif (iscntrl(*c)) {\n-\t\t\twarning(_(\"invalid token '%s' for remote '%s' \"\n-\t\t\t\t \"will not be stored\"),\n-\t\t\t\ttoken, remote_name);\n-\t\t\treturn false;\n-\t\t}\n+\tif (has_control_char(token)) {\n+\t\twarning(_(\"invalid token '%s' for remote '%s' \"\n+\t\t\t \"will not be stored\"),\n+\t\t\ttoken, remote_name);\n+\t\treturn false;\n+\t}\n \n \treturn true;\n }\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540724","messageId":"20260402070613.85934-8-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 07/10] promisor-remote: refactor accept_from_server()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:10Z","receivedAt":"2026-04-02T07:06:45Z","isPatch":true,"body":"In future commits, we are going to add more logic to\nfilter_promisor_remote() which is already doing a lot of things.\n\nLet's alleviate that by moving the logic that checks and validates the\nvalue of the `promisor.acceptFromServer` config variable into its own\naccept_from_server() helper function.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex d60518f19c..8d80ef6040 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -862,20 +862,12 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised,\n \treturn reload_config;\n }\n \n-static void filter_promisor_remote(struct repository *repo,\n-\t\t\t\t struct strvec *accepted,\n-\t\t\t\t const char *info)\n+static enum accept_promisor accept_from_server(struct repository *repo)\n {\n \tconst char *accept_str;\n \tenum accept_promisor accept = ACCEPT_NONE;\n-\tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n-\tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n-\tstruct store_info *store_info = NULL;\n-\tstruct string_list_item *item;\n-\tbool reload_config = false;\n-\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n \n-\tif (!repo_config_get_string_tmp(the_repository, \"promisor.acceptfromserver\", &accept_str)) {\n+\tif (!repo_config_get_string_tmp(repo, \"promisor.acceptfromserver\", &accept_str)) {\n \t\tif (!*accept_str || !strcasecmp(\"None\", accept_str))\n \t\t\taccept = ACCEPT_NONE;\n \t\telse if (!strcasecmp(\"KnownUrl\", accept_str))\n@@ -889,6 +881,21 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\t\taccept_str, \"promisor.acceptfromserver\");\n \t}\n \n+\treturn accept;\n+}\n+\n+static void filter_promisor_remote(struct repository *repo,\n+\t\t\t\t struct strvec *accepted,\n+\t\t\t\t const char *info)\n+{\n+\tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n+\tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n+\tstruct store_info *store_info = NULL;\n+\tstruct string_list_item *item;\n+\tbool reload_config = false;\n+\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n+\tenum accept_promisor accept = accept_from_server(repo);\n+\n \tif (accept == ACCEPT_NONE)\n \t\treturn;\n \n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540725","messageId":"20260402070613.85934-9-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 08/10] promisor-remote: keep accepted promisor_info structs alive","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:11Z","receivedAt":"2026-04-02T07:06:47Z","isPatch":true,"body":"In filter_promisor_remote(), the instances of `struct promisor_info`\nfor accepted remotes are dismantled into separate parallel data\nstructures (the 'accepted' strvec for server names, and\n'accepted_filters' for filter strings) and then immediately freed.\n\nInstead, let's keep these instances on an 'accepted_remotes' list.\n\nThis way the post-loop phase can iterate a single list to build the\nprotocol reply, apply advertised filters, and mark remotes as\naccepted, rather than iterating three separate structures.\n\nThis refactoring also prepares for a future commit that will add a\n'local_name' member to 'struct promisor_info'. Since struct instances\nstay alive, downstream code will be able to simply read both names\nfrom them rather than needing yet another parallel strvec.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 42 +++++++++++++++++-------------------------\n 1 file changed, 17 insertions(+), 25 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8d80ef6040..74e65e9dd0 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -890,10 +890,10 @@ static void filter_promisor_remote(struct repository *repo,\n {\n \tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n \tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n+\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \tstruct store_info *store_info = NULL;\n \tstruct string_list_item *item;\n \tbool reload_config = false;\n-\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n \tenum accept_promisor accept = accept_from_server(repo);\n \n \tif (accept == ACCEPT_NONE)\n@@ -922,17 +922,10 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\tif (promisor_store_advertised_fields(advertised, store_info))\n \t\t\t\treload_config = true;\n \n-\t\t\tstrvec_push(accepted, advertised->name);\n-\n-\t\t\t/* Capture advertised filters for accepted remotes */\n-\t\t\tif (advertised->filter) {\n-\t\t\t\tstruct string_list_item *i;\n-\t\t\t\ti = string_list_append(&accepted_filters, advertised->name);\n-\t\t\t\ti->util = xstrdup(advertised->filter);\n-\t\t\t}\n+\t\t\tstring_list_append(&accepted_remotes, advertised->name)->util = advertised;\n+\t\t} else {\n+\t\t\tpromisor_info_free(advertised);\n \t\t}\n-\n-\t\tpromisor_info_free(advertised);\n \t}\n \n \tpromisor_info_list_clear(&config_info);\n@@ -942,24 +935,23 @@ static void filter_promisor_remote(struct repository *repo,\n \tif (reload_config)\n \t\trepo_promisor_remote_reinit(repo);\n \n-\t/* Apply accepted remote filters to the stable repo state */\n-\tfor_each_string_list_item(item, &accepted_filters) {\n-\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, item->string);\n-\t\tif (r) {\n-\t\t\tfree(r->advertised_filter);\n-\t\t\tr->advertised_filter = item->util;\n-\t\t\titem->util = NULL;\n-\t\t}\n-\t}\n+\t/* Apply accepted remotes to the stable repo state */\n+\tfor_each_string_list_item(item, &accepted_remotes) {\n+\t\tstruct promisor_info *info = item->util;\n+\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, info->name);\n \n-\tstring_list_clear(&accepted_filters, 1);\n+\t\tstrvec_push(accepted, info->name);\n \n-\t/* Mark the remotes as accepted in the repository state */\n-\tfor (size_t i = 0; i < accepted->nr; i++) {\n-\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, accepted->v[i]);\n-\t\tif (r)\n+\t\tif (r) {\n \t\t\tr->accepted = 1;\n+\t\t\tif (info->filter) {\n+\t\t\t\tfree(r->advertised_filter);\n+\t\t\t\tr->advertised_filter = xstrdup(info->filter);\n+\t\t\t}\n+\t\t}\n \t}\n+\n+\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void promisor_remote_reply(const char *info, char **accepted_out)\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540726","messageId":"20260402070613.85934-10-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 09/10] promisor-remote: remove the 'accepted' strvec","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:12Z","receivedAt":"2026-04-02T07:06:48Z","isPatch":true,"body":"In a previous commit, filter_promisor_remote() was refactored to keep\naccepted 'struct promisor_info' instances alive instead of dismantling\nthem into separate parallel data structures.\n\nLet's go one step further and replace the 'struct strvec *accepted'\nargument passed to filter_promisor_remote() with a\n'struct string_list *accepted_remotes' argument.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 74e65e9dd0..38fa050542 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -885,12 +885,11 @@ static enum accept_promisor accept_from_server(struct repository *repo)\n }\n \n static void filter_promisor_remote(struct repository *repo,\n-\t\t\t\t struct strvec *accepted,\n+\t\t\t\t struct string_list *accepted_remotes,\n \t\t\t\t const char *info)\n {\n \tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n \tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n-\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \tstruct store_info *store_info = NULL;\n \tstruct string_list_item *item;\n \tbool reload_config = false;\n@@ -922,7 +921,7 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\tif (promisor_store_advertised_fields(advertised, store_info))\n \t\t\t\treload_config = true;\n \n-\t\t\tstring_list_append(&accepted_remotes, advertised->name)->util = advertised;\n+\t\t\tstring_list_append(accepted_remotes, advertised->name)->util = advertised;\n \t\t} else {\n \t\t\tpromisor_info_free(advertised);\n \t\t}\n@@ -936,12 +935,10 @@ static void filter_promisor_remote(struct repository *repo,\n \t\trepo_promisor_remote_reinit(repo);\n \n \t/* Apply accepted remotes to the stable repo state */\n-\tfor_each_string_list_item(item, &accepted_remotes) {\n+\tfor_each_string_list_item(item, accepted_remotes) {\n \t\tstruct promisor_info *info = item->util;\n \t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, info->name);\n \n-\t\tstrvec_push(accepted, info->name);\n-\n \t\tif (r) {\n \t\t\tr->accepted = 1;\n \t\t\tif (info->filter) {\n@@ -950,23 +947,23 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\t}\n \t\t}\n \t}\n-\n-\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void promisor_remote_reply(const char *info, char **accepted_out)\n {\n-\tstruct strvec accepted = STRVEC_INIT;\n+\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \n-\tfilter_promisor_remote(the_repository, &accepted, info);\n+\tfilter_promisor_remote(the_repository, &accepted_remotes, info);\n \n \tif (accepted_out) {\n-\t\tif (accepted.nr) {\n+\t\tif (accepted_remotes.nr) {\n \t\t\tstruct strbuf reply = STRBUF_INIT;\n-\t\t\tfor (size_t i = 0; i < accepted.nr; i++) {\n-\t\t\t\tif (i)\n+\t\t\tstruct string_list_item *item;\n+\n+\t\t\tfor_each_string_list_item(item, &accepted_remotes) {\n+\t\t\t\tif (reply.len)\n \t\t\t\t\tstrbuf_addch(&reply, ';');\n-\t\t\t\tstrbuf_addstr_urlencode(&reply, accepted.v[i], allow_unsanitized);\n+\t\t\t\tstrbuf_addstr_urlencode(&reply, item->string, allow_unsanitized);\n \t\t\t}\n \t\t\t*accepted_out = strbuf_detach(&reply, NULL);\n \t\t} else {\n@@ -974,7 +971,7 @@ void promisor_remote_reply(const char *info, char **accepted_out)\n \t\t}\n \t}\n \n-\tstrvec_clear(&accepted);\n+\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes)\n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540727","messageId":"20260402070613.85934-11-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH 10/10] t5710: use proper file:// URIs for absolute paths","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-02T07:06:13Z","receivedAt":"2026-04-02T07:06:51Z","isPatch":true,"body":"In t5710, we frequently construct local file URIs using `file://$(pwd)`.\nOn Unix-like systems, $(pwd) returns an absolute path starting with a\nslash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an\nempty host (`file:///tmp/repo`).\n\nHowever, on Windows, $(pwd) returns a path starting with a drive\nletter (e.g., `D:/a/repo`). This results in a 2-slash URI\n(`file://D:/a/repo`). Standard URI parsers misinterpret this format,\ntreating `D:` as the host rather than part of the absolute path.\n\nThis is to be expected because RFC 8089 says that the `//` prefix with\nan empty local host must be followed by an absolute path starting with\na slash.\n\nWhile this hasn't broken the existing tests (because the old\n`promisor.acceptFromServer` logic relies entirely on strict `strcmp()`\nwithout normalizing the URLs), it will break future commits that pass\nthese URLs through `url_normalize()` or similar functions.\n\nTo future-proof the tests and ensure cross-platform URI compliance,\nlet's introduce a $PWD_URL helper that explicitly guarantees a leading\nslash for the path component, ensuring valid 3-slash `file:///` URIs on\nall operating systems.\n\nWhile at it, let's also introduce $ENCODED_PWD_URL to handle spaces in\ndirectory paths (which is needed for URL glob pattern matching).\n\nThen let's replace all instances of `file://$(pwd)` with $PWD_URL across\nthe test script, and let's simplify the `ENCODED_URL` constructions in\nthe `sendFields` and `checkFields` tests to use $ENCODED_PWD_URL\ndirectly.\n\nSigned-off-by: Christian Couder \n---\n t/t5710-promisor-remote-capability.sh | 55 ++++++++++++++++-----------\n 1 file changed, 32 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex bf0eed9f10..3eca6601ca 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -76,6 +76,17 @@ copy_to_lop () {\n \tcp \"$path\" \"$path2\"\n }\n \n+# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn\n+# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.\n+# On Unix, the path already starts with '/', so this is a no-op.\n+pwd_path=$(pwd)\n+case \"$pwd_path\" in\n+[a-zA-Z]:*) pwd_path=\"/$pwd_path\" ;;\n+esac\n+PWD_URL=\"file://$pwd_path\"\n+# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.\n+ENCODED_PWD_URL=\"file://$(echo \"$pwd_path\" | sed \"s/ /%20/g\")\"\n+\n test_expect_success \"setup for testing promisor remote advertisement\" '\n \t# Create another bare repo called \"lop\" (for Large Object Promisor)\n \tgit init --bare lop &&\n@@ -88,7 +99,7 @@ test_expect_success \"setup for testing promisor remote advertisement\" '\n \tinitialize_server 1 \"$oid\" &&\n \n \t# Configure lop as promisor remote for server\n-\tgit -C server remote add lop \"file://$(pwd)/lop\" &&\n+\tgit -C server remote add lop \"$PWD_URL/lop\" &&\n \tgit -C server config remote.lop.promisor true &&\n \n \tgit -C lop config uploadpack.allowFilter true &&\n@@ -104,7 +115,7 @@ test_expect_success \"clone with promisor.advertise set to 'true'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -119,7 +130,7 @@ test_expect_success \"clone with promisor.advertise set to 'false'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -137,7 +148,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'None'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=None \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -156,8 +167,8 @@ test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n \tgit -C client init &&\n \tgit -C client config remote.lop.promisor true &&\n \tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n-\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n-\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n+\tgit -C client config remote.lop.url \"$PWD_URL/lop\" &&\n+\tgit -C client config remote.server.url \"$PWD_URL/server\" &&\n \tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n \tgit -C client config promisor.acceptfromserver All &&\n \tGIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n@@ -242,7 +253,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -257,7 +268,7 @@ test_expect_success \"clone with 'KnownName' and different remote names\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \\\n \t\t-c remote.serverTwo.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.serverTwo.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.serverTwo.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -294,7 +305,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'KnownUrl'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -311,7 +322,7 @@ test_expect_success \"clone with 'KnownUrl' and different remote urls\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/serverTwo\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/serverTwo\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -326,7 +337,7 @@ test_expect_success \"clone with 'KnownUrl' and url not configured on the server\"\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n \n-\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$PWD_URL/lop\\\"\" &&\n \tgit -C server config unset remote.lop.url &&\n \n \t# Clone from server to create a client\n@@ -335,7 +346,7 @@ test_expect_success \"clone with 'KnownUrl' and url not configured on the server\"\n \t# missing, so the remote name will be used instead which will fail.\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -347,7 +358,7 @@ test_expect_success \"clone with 'KnownUrl' and empty url, so not advertised\" '\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n \n-\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$PWD_URL/lop\\\"\" &&\n \tgit -C server config set remote.lop.url \"\" &&\n \n \t# Clone from server to create a client\n@@ -356,7 +367,7 @@ test_expect_success \"clone with 'KnownUrl' and empty url, so not advertised\" '\n \t# so the remote name will be used instead which will fail.\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -380,13 +391,12 @@ test_expect_success \"clone with promisor.sendFields\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n \t# Check that fields are properly transmitted\n-\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n-\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n+\tPR1=\"name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop;otherLop\" trace &&\n@@ -411,15 +421,14 @@ test_expect_success \"clone with promisor.checkFields\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t-c promisor.checkFields=partialcloneFilter \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n \t# Check that fields are properly transmitted\n-\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n-\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n+\tPR1=\"name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop\" trace &&\n@@ -449,7 +458,7 @@ test_expect_success \"clone with promisor.storeFields=partialCloneFilter\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c remote.lop.token=\"fooYYY\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n@@ -501,7 +510,7 @@ test_expect_success \"clone and fetch with --filter=auto\" '\n \n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=auto server client 2>err &&\n \n@@ -558,7 +567,7 @@ test_expect_success \"clone with promisor.advertise set to 'true' but don't delet\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n-- \n2.53.0.765.g57b94de1f0.dirty\n\n"},{"id":"540737","messageId":"ac4evWK9k69LIV91@pks.im","threadId":"65413","inReplyTo":"20260402070613.85934-2-christian.couder@gmail.com","subject":"Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-02T07:46:05Z","receivedAt":"2026-04-02T07:46:13Z","isPatch":true,"body":"On Thu, Apr 02, 2026 at 09:06:04AM +0200, Christian Couder wrote:\n> diff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc\n> index f985cb4c47..4fcb1a7bda 100644\n> --- a/Documentation/gitprotocol-v2.adoc\n> +++ b/Documentation/gitprotocol-v2.adoc\n> @@ -848,6 +848,10 @@ advertised, it can reply with \"promisor-remote=\" where\n> where `pr-name` is the urlencoded name of a promisor remote the server\n> advertised and the client accepts.\n> \n> +The promisor remotes that the client accepted will be tried before the\n> +other configured promisor remotes when the client attempts to fetch\n> +missing objects.\n> +\n> Note that, everywhere in this document, the ';' and ',' characters\n> MUST be encoded if they appear in `pr-name` or `field-value`.\n\nMakes sense, thanks for adding this blurb.\n\n> diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\n> index 357822c01a..bf0eed9f10 100755\n> --- a/t/t5710-promisor-remote-capability.sh\n> +++ b/t/t5710-promisor-remote-capability.sh\n> @@ -166,6 +166,75 @@ test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n> \tcheck_missing_objects server 1 \"$oid\"\n> '\n> \n> +test_expect_success \"clone with two promisors but only one advertised\" '\n> +\tgit -C server config promisor.advertise true &&\n> +\ttest_when_finished \"rm -rf client unused_lop\" &&\n> +\n> +\t# Create a promisor that will be configured but not be used\n> +\tgit init --bare unused_lop &&\n> +\n> +\t# Clone from server to create a client\n> +\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n> +\t\t-c remote.unused_lop.promisor=true \\\n> +\t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n> +\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n> +\t\t-c remote.lop.promisor=true \\\n> +\t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n> +\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n> +\t\t-c promisor.acceptfromserver=All \\\n> +\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n> +\n> +\t# Check that \"unused_lop\" appears before \"lop\" in the config\n> +\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" \"origin\" >expect &&\n> +\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Check that \"lop\" was tried\n> +\ttest_grep \" fetch lop \" trace &&\n> +\t# Check that \"unused_lop\" was not contacted\n> +\t# This means \"lop\", the accepted promisor, was tried first\n> +\ttest_grep ! \" fetch unused_lop \" trace &&\n> +\n> +\t# Check that the largest object is still missing on the server\n> +\tcheck_missing_objects server 1 \"$oid\"\n> +'\n> +\n> +test_expect_success \"init + fetch two promisors but only one advertised\" '\n> +\tgit -C server config promisor.advertise true &&\n> +\ttest_when_finished \"rm -rf client unused_lop\" &&\n> +\n> +\t# Create a promisor that will be configured but not be used\n> +\tgit init --bare unused_lop &&\n> +\n> +\tmkdir client &&\n> +\tgit -C client init &&\n\nTiniest nit, not worth rerolling over: this could just be `git init client`.\n\nPatrick\n"},{"id":"540738","messageId":"ac4ew5oQNU7VJYOa@pks.im","threadId":"65413","inReplyTo":"20260402070613.85934-5-christian.couder@gmail.com","subject":"Re: [PATCH 04/10] promisor-remote: reject empty name or URL in advertised remote","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-02T07:46:11Z","receivedAt":"2026-04-02T07:46:17Z","isPatch":true,"body":"On Thu, Apr 02, 2026 at 09:06:07AM +0200, Christian Couder wrote:\n> In parse_one_advertised_remote(), we check for a NULL remote name and\n> remote URL, but not for empty ones. An empty URL seems possible as\n> url_percent_decode(\"\") doesn't return NULL.\n> \n> In promisor_config_info_list(), we ignore remotes with empty URLs, so a\n> Git server should not advertise remotes with empty URLs. It's possible\n> that a buggy or malicious server would do it though.\n> \n> So let's tighten the check in parse_one_advertised_remote() to also\n> reject empty strings at parse time.\n\nMakes sense.\n\nPatrick\n"},{"id":"540739","messageId":"ac4fGpG9WLZqnH7f@pks.im","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-02T07:47:38Z","receivedAt":"2026-04-02T07:47:45Z","isPatch":true,"body":"On Thu, Apr 02, 2026 at 09:06:03AM +0200, Christian Couder wrote:\n> Recently, I sent a 16 patch long series that makes it possible for\n> promisor remotes advertised through the \"promisor-remote\" protocol\n> capability to be auto-configured on the client side via a URL\n> allowlist configured using a new `promisor.acceptFromServerUrl`\n> configuration variable:\n> \n> https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/\n> \n> I got the suggestion to split the 16 patches series into two smaller\n> series, starting with a preparatory series. So here is this\n> preparatory series. It's a mix of mostly small fixes, refactorings and\n> cleanups.\n> \n> High level description of the patches\n> =====================================\n> \n> - Patches 1-4/10 are fixes:\n> \n> - Patch 1/10 is the most significant. The others are relatively\n> small.\n> \n> - Patch 4/10 is the only new in this series (while all the others\n> were in the previous series). It prepares for Patch 5/10.\n> \n> - Patches 5-10/10 are refactorings and cleanups:\n> \n> - Patches 5-7/10 are relatively small independents cleanups or\n> refactorings.\n> \n> - Patches 8-9/10 are related refactorings simplifying the data\n> structures used in filter_promisor_remote() and\n> promisor_remote_reply().\n> \n> - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths\n> in the test script.\n\nThanks for splitting out these changes. In case anybody else cares,\nI've attached the range-diff below.\n\nIn any case, I'm happy with these changes. There's a single nit, but I\ndon't think it's worth rerolling over. Thanks!\n\nPatrick\n\n 1: aeae4b9971 ! 1: 30d0aa8a24 promisor-remote: try accepted remotes before others in get_direct()\n @@ Commit message\n \n Signed-off-by: Christian Couder \n \n + ## Documentation/gitprotocol-v2.adoc ##\n +@@ Documentation/gitprotocol-v2.adoc: advertised, it can reply with \"promisor-remote=\" where\n + where `pr-name` is the urlencoded name of a promisor remote the server\n + advertised and the client accepts.\n + \n ++The promisor remotes that the client accepted will be tried before the\n ++other configured promisor remotes when the client attempts to fetch\n ++missing objects.\n ++\n + Note that, everywhere in this document, the ';' and ',' characters\n + MUST be encoded if they appear in `pr-name` or `field-value`.\n + \n +\n ## promisor-remote.c ##\n @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,\n +\tstruct promisor_remote *r = repo->promisor_remote_config->promisors;\n +\n +\tfor (; r; r = r->next) {\n -+\t\tif (accepted_only && !r->accepted)\n -+\t\t\tcontinue;\n -+\t\tif (!accepted_only && r->accepted)\n ++\t\tif (accepted_only != r->accepted)\n +\t\t\tcontinue;\n +\t\tif (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {\n +\t\t\tif (*remaining_nr == 1)\n @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,\n \n \tfor (i = 0; i < remaining_nr; i++) {\n \t\tif (is_promisor_object(repo, &remaining_oids[i]))\n +\n + ## t/t5710-promisor-remote-capability.sh ##\n +@@ t/t5710-promisor-remote-capability.sh: test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n + \tcheck_missing_objects server 1 \"$oid\"\n + '\n + \n ++test_expect_success \"clone with two promisors but only one advertised\" '\n ++\tgit -C server config promisor.advertise true &&\n ++\ttest_when_finished \"rm -rf client unused_lop\" &&\n ++\n ++\t# Create a promisor that will be configured but not be used\n ++\tgit init --bare unused_lop &&\n ++\n ++\t# Clone from server to create a client\n ++\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n ++\t\t-c remote.unused_lop.promisor=true \\\n ++\t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n ++\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n ++\t\t-c remote.lop.promisor=true \\\n ++\t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n ++\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n ++\t\t-c promisor.acceptfromserver=All \\\n ++\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n ++\n ++\t# Check that \"unused_lop\" appears before \"lop\" in the config\n ++\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" \"origin\" >expect &&\n ++\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n ++\ttest_cmp expect actual &&\n ++\n ++\t# Check that \"lop\" was tried\n ++\ttest_grep \" fetch lop \" trace &&\n ++\t# Check that \"unused_lop\" was not contacted\n ++\t# This means \"lop\", the accepted promisor, was tried first\n ++\ttest_grep ! \" fetch unused_lop \" trace &&\n ++\n ++\t# Check that the largest object is still missing on the server\n ++\tcheck_missing_objects server 1 \"$oid\"\n ++'\n ++\n ++test_expect_success \"init + fetch two promisors but only one advertised\" '\n ++\tgit -C server config promisor.advertise true &&\n ++\ttest_when_finished \"rm -rf client unused_lop\" &&\n ++\n ++\t# Create a promisor that will be configured but not be used\n ++\tgit init --bare unused_lop &&\n ++\n ++\tmkdir client &&\n ++\tgit -C client init &&\n ++\tgit -C client config remote.unused_lop.promisor true &&\n ++\tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n ++\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n ++\tgit -C client config remote.lop.promisor true &&\n ++\tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n ++\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n ++\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n ++\tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n ++\tgit -C client config promisor.acceptfromserver All &&\n ++\n ++\t# Check that \"unused_lop\" appears before \"lop\" in the config\n ++\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" >expect &&\n ++\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n ++\ttest_cmp expect actual &&\n ++\n ++\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n ++\n ++\t# Check that \"lop\" was tried\n ++\ttest_grep \" fetch lop \" trace &&\n ++\t# Check that \"unused_lop\" was not contacted\n ++\t# This means \"lop\", the accepted promisor, was tried first\n ++\ttest_grep ! \" fetch unused_lop \" trace &&\n ++\n ++\t# Check that the largest object is still missing on the server\n ++\tcheck_missing_objects server 1 \"$oid\"\n ++'\n ++\n + test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n + \tgit -C server config promisor.advertise true &&\n + \ttest_when_finished \"rm -rf client\" &&\n 2: 853051db60 < -: ---------- urlmatch: change 'allow_globs' arg to bool\n 3: e17cc86c37 < -: ---------- urlmatch: add url_is_valid_pattern() helper\n10: ac7742f5ba ! 2: 60ba726cda promisor-remote: pass config entry to all_fields_match() directly\n @@ Metadata\n ## Commit message ##\n promisor-remote: pass config entry to all_fields_match() directly\n \n - The `in_list == 0` path of all_fields_match() re-looks up the\n - remote in config_info by advertised->name, even though every\n + The `in_list == 0` path of all_fields_match() looks up the remote in\n + `config_info` by `advertised->name` repeatedly, even though every\n caller in should_accept_remote() has already performed this\n - lookup and holds the result in 'p'.\n + lookup and holds the result in `p`.\n \n To avoid this useless work, let's replace the `int in_list`\n parameter with a `struct promisor_info *config_entry` pointer:\n @@ Commit message\n avoiding the redundant string_list_lookup() call.\n \n This removes the hidden dependency on `advertised->name` inside\n - all_fields_match(), which would be wrong in the following commits when\n - auto-configured remotes will be implemented as the local config name\n - may differ from the server's advertised name.\n + all_fields_match(), which would be wrong if in the future\n + auto-configured remotes are implemented, as the local config name may\n + differ from the server's advertised name.\n +\n + While at it, let's also add a comment before all_fields_match() and\n + match_field_against_config() to help understand how things work and\n + help avoid similar issues.\n \n Signed-off-by: Christian Couder \n \n ## promisor-remote.c ##\n +@@ promisor-remote.c: enum accept_promisor {\n + \tACCEPT_ALL\n + };\n + \n ++/*\n ++ * Check if a specific field and its advertised value match the local\n ++ * configuration of a given promisor remote.\n ++ *\n ++ * Returns 1 if they match, 0 otherwise.\n ++ */\n + static int match_field_against_config(const char *field, const char *value,\n + \t\t\t\t struct promisor_info *config_info)\n + {\n @@ promisor-remote.c: static int match_field_against_config(const char *field, const char *value,\n \treturn 0;\n }\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n -\t\treturn all_fields_match(advertised, config_info, 0);\n +\t\treturn all_fields_match(advertised, config_info, p);\n \n - \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n - \t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n + \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n + \t\tremote_name, p->url, remote_url);\n 4: c2f9f3ebef ! 3: d69391fd61 promisor-remote: clarify that a remote is ignored\n @@ Metadata\n ## Commit message ##\n promisor-remote: clarify that a remote is ignored\n \n - In should_accept_remote() when a remote is ignored, we might tell users\n - why it is ignored in a warning, but we don't tell them that the remote\n - is actually ignored.\n + In should_accept_remote() and parse_one_advertised_remote(), when a\n + remote is ignored, we tell users why it is ignored in a warning, but we\n + don't tell them that the remote is actually ignored.\n \n Let's clarify that, so users have a better idea of what's actually\n happening.\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n - \t\treturn all_fields_match(advertised, config_info, 0);\n + \t\treturn all_fields_match(advertised, config_info, p);\n \n -\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n -\t\tremote_name, p->url, remote_url);\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \n \treturn 0;\n }\n +@@ promisor-remote.c: static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n + \tstring_list_clear(&elem_list, 0);\n + \n + \tif (!info->name || !info->url) {\n +-\t\twarning(_(\"server advertised a promisor remote without a name or URL: %s\"),\n +-\t\t\tremote_info);\n ++\t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n ++\t\t\t \"ignoring this remote\"), remote_info);\n + \t\tpromisor_info_free(info);\n + \t\treturn NULL;\n + \t}\n -: ---------- > 4: f4815d68ea promisor-remote: reject empty name or URL in advertised remote\n11: ebc6f2dbeb ! 5: 48605c4612 promisor-remote: refactor should_accept_remote() control flow\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor should_accept_remote() control flow\n \n - In following commits, we are going to add URL-based acceptance logic\n - into should_accept_remote().\n + A previous commit made sure we now reject empty URLs early at parse\n + time. This makes the existing warning() in case a remote URL is NULL\n + or empty very unlikely to be useful.\n \n - To prepare for the upcoming changes, let's restructure the control flow\n - in should_accept_remote().\n + In future work, we also plan to add URL-based acceptance logic into\n + should_accept_remote().\n +\n + To adapt to previous changes and prepare for upcoming changes, let's\n + restructure the control flow in should_accept_remote().\n \n Concretely, let's:\n \n - - Move the empty-URL check to the very top of the function, so that\n - every acceptance mode, instead of only ACCEPT_KNOWN_URL, rejects\n - remotes with a missing or blank URL early.\n + - Replace the warning() in case of an empty URL with a BUG(), as a\n + previous commit made sure empty URLs are rejected early at parse\n + time.\n +\n + - Move that modified empty-URL check to the very top of the function,\n + so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is\n + covered.\n \n - Invert the URL comparison: instead of returning on match and\n warning on mismatch, return early on mismatch and let the match\n case fall through. This opens a single exit path at the bottom of\n - the function for following commits to extend.\n -\n - Anyway the server shouldn't send any empty URL in the first place, so\n - this shouldn't change any behavior in practice.\n + the function for future commits to extend.\n \n Signed-off-by: Christian Couder \n \n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_name = advertised->name;\n \tconst char *remote_url = advertised->url;\n \n -+\tif (!remote_url || !*remote_url) {\n -+\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n -+\t\t\t \"ignoring this remote\"), remote_name);\n -+\t\treturn 0;\n -+\t}\n ++\tif (!remote_url || !*remote_url)\n ++\t\tBUG(\"no or empty URL advertised for remote '%s'; \"\n ++\t\t \"this remote should have been rejected earlier\",\n ++\t\t remote_name);\n +\n \tif (accept == ACCEPT_ALL)\n \t\treturn all_fields_match(advertised, config_info, NULL);\n 5: bdd62c20a4 ! 6: 632e3bfe25 promisor-remote: refactor has_control_char()\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor has_control_char()\n \n - In a following commit we are going to check if some strings contain\n - control character, so let's refactor the logic to do that in a new\n + In a future commit we are going to check if some strings contain\n + control characters, so let's refactor the logic to do that in a new\n has_control_char() helper function.\n \n + It cleans up the code a bit anyway.\n +\n Signed-off-by: Christian Couder \n \n ## promisor-remote.c ##\n 6: 962db2ee6c ! 7: 12dc262514 promisor-remote: refactor accept_from_server()\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor accept_from_server()\n \n - In a following commit, we are going to add more logic to\n + In future commits, we are going to add more logic to\n filter_promisor_remote() which is already doing a lot of things.\n \n Let's alleviate that by moving the logic that checks and validates the\n 7: b4d125bd36 ! 8: 9d47ed27e8 promisor-remote: keep accepted promisor_info structs alive\n @@ Commit message\n protocol reply, apply advertised filters, and mark remotes as\n accepted, rather than iterating three separate structures.\n \n - This refactoring also prepares for a following commit that will add a\n + This refactoring also prepares for a future commit that will add a\n 'local_name' member to 'struct promisor_info'. Since struct instances\n stay alive, downstream code will be able to simply read both names\n from them rather than needing yet another parallel strvec.\n 8: 3252e216f1 = 9: b6c020696d promisor-remote: remove the 'accepted' strvec\n 9: 60e17ccb99 < -: ---------- promisor-remote: add 'local_name' to 'struct promisor_info'\n12: 467ee9eff3 ! 10: d47aa7a31a t5710: use proper file:// URIs for absolute paths\n @@ Commit message\n (`file://D:/a/repo`). Standard URI parsers misinterpret this format,\n treating `D:` as the host rather than part of the absolute path.\n \n + This is to be expected because RFC 8089 says that the `//` prefix with\n + an empty local host must be followed by an absolute path starting with\n + a slash.\n +\n While this hasn't broken the existing tests (because the old\n - `promisor.acceptFromServer` logic relies entirely on strict `strcmp`\n + `promisor.acceptFromServer` logic relies entirely on strict `strcmp()`\n without normalizing the URLs), it will break future commits that pass\n - these URLs through `url_normalize()`.\n + these URLs through `url_normalize()` or similar functions.\n \n To future-proof the tests and ensure cross-platform URI compliance,\n let's introduce a $PWD_URL helper that explicitly guarantees a leading\n13: 637db2b4d8 < -: ---------- promisor-remote: introduce promisor.acceptFromServerUrl\n14: c03c5261fb < -: ---------- promisor-remote: trust known remotes matching acceptFromServerUrl\n15: 0d8acf10f8 < -: ---------- promisor-remote: auto-configure unknown remotes\n16: a27565223f < -: ---------- doc: promisor: improve acceptFromServer entry\n"},{"id":"540741","messageId":"xmqqa4vlu1ij.fsf@gitster.g","threadId":"65413","inReplyTo":"20260402070613.85934-11-christian.couder@gmail.com","subject":"Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-02T09:58:44Z","receivedAt":"2026-04-02T09:58:48Z","isPatch":true,"body":"Christian Couder writes:\n\n> +# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn\n> +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.\n> +# On Unix, the path already starts with '/', so this is a no-op.\n> +pwd_path=$(pwd)\n> +case \"$pwd_path\" in\n> +[a-zA-Z]:*) pwd_path=\"/$pwd_path\" ;;\n> +esac\n> +PWD_URL=\"file://$pwd_path\"\n> +# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.\n> +ENCODED_PWD_URL=\"file://$(echo \"$pwd_path\" | sed \"s/ /%20/g\")\"\n\nTwo comments.\n\n - I was a bit surprised that these are not given as functions but\n as variables, as a caller that chdirs around in the trash\n directory would want a URL that points at its current working\n directory (the expectation is from \"pwd\" in the name PWD_URL).\n But a variable based interface \"Here is the URL that corresponds\n to the trash directory\" is OK and probably easier to use than\n \"give me the URL corresponding to my current working directory\",\n simply because it allows a caller to append some string to it to\n come up with a URL for any subdirectory on its own without\n actually going there. But in that case, the name PWD_URL would\n become misleading, as it is PWD as of the moment the variable\n gets defined, and the true meaning of the variable is not \"URL\n for the current directory\", but \"URL for the trash directory\" is\n more usable definition.\n\n - Is it sufficient to only special case SP? My repository may be\n $HOME/w/git.git, for example, and the trash repository may be\n \"$HOME/w/git.git/t/trash directory.t5710/\", so you need to cope\n with SP between \"trash\" and \"directory\" the test framework adds\n (to force you to be careful), but the test framework does not\n control what can be in the leading $HOME part.\n"},{"id":"540784","messageId":"xmqq5x69qkcg.fsf@gitster.g","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-02T18:37:51Z","receivedAt":"2026-04-02T18:37:54Z","isPatch":true,"body":"Christian Couder writes:\n\n> Recently, I sent a 16 patch long series that makes it possible for\n> promisor remotes advertised through the \"promisor-remote\" protocol\n> capability to be auto-configured on the client side via a URL\n> allowlist configured using a new `promisor.acceptFromServerUrl`\n> configuration variable:\n>\n> https://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/\n>\n> I got the suggestion to split the 16 patches series into two smaller\n> series, starting with a preparatory series. So here is this\n> preparatory series. It's a mix of mostly small fixes, refactorings and\n> cleanups.\n>\n> High level description of the patches\n> =====================================\n>\n> - Patches 1-4/10 are fixes:\n>\n> - Patch 1/10 is the most significant. The others are relatively\n> small.\n> \n> - Patch 4/10 is the only new in this series (while all the others\n> were in the previous series). It prepares for Patch 5/10.\n>\n> - Patches 5-10/10 are refactorings and cleanups:\n>\n> - Patches 5-7/10 are relatively small independents cleanups or\n> refactorings.\n>\n> - Patches 8-9/10 are related refactorings simplifying the data\n> structures used in filter_promisor_remote() and\n> promisor_remote_reply().\n>\n> - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths\n> in the test script.\n>\n> CI tests\n> ========\n>\n> They all pass, see:\n>\n> https://github.com/chriscool/git/actions/runs/23848484597\n>\n> Range-diff\n> ==========\n>\n> Sorry, no range-diff as I don't think it would be quite useful because\n> the number and order of commits has changed a lot.\n\nWhen comparing against 16-patch series from the previous round, 6 of\nthe old patches will have no corresponding patch in this round,\nwhich is expected. But for the remaining 10 commits, the command\nseems to do a decent job matching up the corresponding patches from\nthe previous round. It is especially pleasing to see that the first\none from each round are matched up and showing that its tests are\nmoderately extended.\n\n 1: 292252ce36 ! 1: d904d5f94d promisor-remote: try accepted remotes before others in get_direct()\n @@ Commit message\n Signed-off-by: Christian Couder \n Signed-off-by: Junio C Hamano \n \n + ## Documentation/gitprotocol-v2.adoc ##\n +@@ Documentation/gitprotocol-v2.adoc: advertised, it can reply with \"promisor-remote=\" where\n + where `pr-name` is the urlencoded name of a promisor remote the server\n + advertised and the client accepts.\n + \n ++The promisor remotes that the client accepted will be tried before the\n ++other configured promisor remotes when the client attempts to fetch\n ++missing objects.\n ++\n + Note that, everywhere in this document, the ';' and ',' characters\n + MUST be encoded if they appear in `pr-name` or `field-value`.\n + \n +\n ## promisor-remote.c ##\n @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n @@ promisor-remote.c: static int remove_fetched_oids(struct repository *repo,\n +\tstruct promisor_remote *r = repo->promisor_remote_config->promisors;\n +\n +\tfor (; r; r = r->next) {\n -+\t\tif (accepted_only && !r->accepted)\n -+\t\t\tcontinue;\n -+\t\tif (!accepted_only && r->accepted)\n ++\t\tif (accepted_only != r->accepted)\n +\t\t\tcontinue;\n +\t\tif (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {\n +\t\t\tif (*remaining_nr == 1)\n @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,\n \n \tfor (i = 0; i < remaining_nr; i++) {\n \t\tif (is_promisor_object(repo, &remaining_oids[i]))\n +\n + ## t/t5710-promisor-remote-capability.sh ##\n +@@ t/t5710-promisor-remote-capability.sh: test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n + \tcheck_missing_objects server 1 \"$oid\"\n + '\n + \n ++test_expect_success \"clone with two promisors but only one advertised\" '\n ++\tgit -C server config promisor.advertise true &&\n ++\ttest_when_finished \"rm -rf client unused_lop\" &&\n ++\n ++\t# Create a promisor that will be configured but not be used\n ++\tgit init --bare unused_lop &&\n ++\n ++\t# Clone from server to create a client\n ++\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n ++\t\t-c remote.unused_lop.promisor=true \\\n ++\t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n ++\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n ++\t\t-c remote.lop.promisor=true \\\n ++\t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n ++\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n ++\t\t-c promisor.acceptfromserver=All \\\n ++\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n ++\n ++\t# Check that \"unused_lop\" appears before \"lop\" in the config\n ++\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" \"origin\" >expect &&\n ++\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n ++\ttest_cmp expect actual &&\n ++\n ++\t# Check that \"lop\" was tried\n ++\ttest_grep \" fetch lop \" trace &&\n ++\t# Check that \"unused_lop\" was not contacted\n ++\t# This means \"lop\", the accepted promisor, was tried first\n ++\ttest_grep ! \" fetch unused_lop \" trace &&\n ++\n ++\t# Check that the largest object is still missing on the server\n ++\tcheck_missing_objects server 1 \"$oid\"\n ++'\n ++\n ++test_expect_success \"init + fetch two promisors but only one advertised\" '\n ++\tgit -C server config promisor.advertise true &&\n ++\ttest_when_finished \"rm -rf client unused_lop\" &&\n ++\n ++\t# Create a promisor that will be configured but not be used\n ++\tgit init --bare unused_lop &&\n ++\n ++\tmkdir client &&\n ++\tgit -C client init &&\n ++\tgit -C client config remote.unused_lop.promisor true &&\n ++\tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n ++\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n ++\tgit -C client config remote.lop.promisor true &&\n ++\tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n ++\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n ++\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n ++\tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n ++\tgit -C client config promisor.acceptfromserver All &&\n ++\n ++\t# Check that \"unused_lop\" appears before \"lop\" in the config\n ++\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" >expect &&\n ++\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n ++\ttest_cmp expect actual &&\n ++\n ++\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n ++\n ++\t# Check that \"lop\" was tried\n ++\ttest_grep \" fetch lop \" trace &&\n ++\t# Check that \"unused_lop\" was not contacted\n ++\t# This means \"lop\", the accepted promisor, was tried first\n ++\ttest_grep ! \" fetch unused_lop \" trace &&\n ++\n ++\t# Check that the largest object is still missing on the server\n ++\tcheck_missing_objects server 1 \"$oid\"\n ++'\n ++\n + test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n + \tgit -C server config promisor.advertise true &&\n + \ttest_when_finished \"rm -rf client\" &&\n 2: 8cea12d525 < -: ---------- urlmatch: change 'allow_globs' arg to bool\n 3: 30e3618f1b < -: ---------- urlmatch: add url_is_valid_pattern() helper\n10: 0ccf10712d ! 2: 54174b6db5 promisor-remote: pass config entry to all_fields_match() directly\n @@ Metadata\n ## Commit message ##\n promisor-remote: pass config entry to all_fields_match() directly\n \n - The `in_list == 0` path of all_fields_match() re-looks up the\n - remote in config_info by advertised->name, even though every\n + The `in_list == 0` path of all_fields_match() looks up the remote in\n + `config_info` by `advertised->name` repeatedly, even though every\n caller in should_accept_remote() has already performed this\n - lookup and holds the result in 'p'.\n + lookup and holds the result in `p`.\n \n To avoid this useless work, let's replace the `int in_list`\n parameter with a `struct promisor_info *config_entry` pointer:\n @@ Commit message\n avoiding the redundant string_list_lookup() call.\n \n This removes the hidden dependency on `advertised->name` inside\n - all_fields_match(), which would be wrong in the following commits when\n - auto-configured remotes will be implemented as the local config name\n - may differ from the server's advertised name.\n + all_fields_match(), which would be wrong if in the future\n + auto-configured remotes are implemented, as the local config name may\n + differ from the server's advertised name.\n +\n + While at it, let's also add a comment before all_fields_match() and\n + match_field_against_config() to help understand how things work and\n + help avoid similar issues.\n \n Signed-off-by: Christian Couder \n Signed-off-by: Junio C Hamano \n \n ## promisor-remote.c ##\n +@@ promisor-remote.c: enum accept_promisor {\n + \tACCEPT_ALL\n + };\n + \n ++/*\n ++ * Check if a specific field and its advertised value match the local\n ++ * configuration of a given promisor remote.\n ++ *\n ++ * Returns 1 if they match, 0 otherwise.\n ++ */\n + static int match_field_against_config(const char *field, const char *value,\n + \t\t\t\t struct promisor_info *config_info)\n + {\n @@ promisor-remote.c: static int match_field_against_config(const char *field, const char *value,\n \treturn 0;\n }\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n -\t\treturn all_fields_match(advertised, config_info, 0);\n +\t\treturn all_fields_match(advertised, config_info, p);\n \n - \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n - \t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n + \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n + \t\tremote_name, p->url, remote_url);\n 4: b602c1d5ad ! 3: 56642a3c6a promisor-remote: clarify that a remote is ignored\n @@ Metadata\n ## Commit message ##\n promisor-remote: clarify that a remote is ignored\n \n - In should_accept_remote() when a remote is ignored, we might tell users\n - why it is ignored in a warning, but we don't tell them that the remote\n - is actually ignored.\n + In should_accept_remote() and parse_one_advertised_remote(), when a\n + remote is ignored, we tell users why it is ignored in a warning, but we\n + don't tell them that the remote is actually ignored.\n \n Let's clarify that, so users have a better idea of what's actually\n happening.\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n - \t\treturn all_fields_match(advertised, config_info, 0);\n + \t\treturn all_fields_match(advertised, config_info, p);\n \n -\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n -\t\tremote_name, p->url, remote_url);\n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \n \treturn 0;\n }\n +@@ promisor-remote.c: static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n + \tstring_list_clear(&elem_list, 0);\n + \n + \tif (!info->name || !info->url) {\n +-\t\twarning(_(\"server advertised a promisor remote without a name or URL: %s\"),\n +-\t\t\tremote_info);\n ++\t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n ++\t\t\t \"ignoring this remote\"), remote_info);\n + \t\tpromisor_info_free(info);\n + \t\treturn NULL;\n + \t}\n -: ---------- > 4: 859ba9d41c promisor-remote: reject empty name or URL in advertised remote\n11: c2b234a4ce ! 5: 724f9bbc22 promisor-remote: refactor should_accept_remote() control flow\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor should_accept_remote() control flow\n \n - In following commits, we are going to add URL-based acceptance logic\n - into should_accept_remote().\n + A previous commit made sure we now reject empty URLs early at parse\n + time. This makes the existing warning() in case a remote URL is NULL\n + or empty very unlikely to be useful.\n \n - To prepare for the upcoming changes, let's restructure the control flow\n - in should_accept_remote().\n + In future work, we also plan to add URL-based acceptance logic into\n + should_accept_remote().\n +\n + To adapt to previous changes and prepare for upcoming changes, let's\n + restructure the control flow in should_accept_remote().\n \n Concretely, let's:\n \n - - Move the empty-URL check to the very top of the function, so that\n - every acceptance mode, instead of only ACCEPT_KNOWN_URL, rejects\n - remotes with a missing or blank URL early.\n + - Replace the warning() in case of an empty URL with a BUG(), as a\n + previous commit made sure empty URLs are rejected early at parse\n + time.\n +\n + - Move that modified empty-URL check to the very top of the function,\n + so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is\n + covered.\n \n - Invert the URL comparison: instead of returning on match and\n warning on mismatch, return early on mismatch and let the match\n case fall through. This opens a single exit path at the bottom of\n - the function for following commits to extend.\n -\n - Anyway the server shouldn't send any empty URL in the first place, so\n - this shouldn't change any behavior in practice.\n + the function for future commits to extend.\n \n Signed-off-by: Christian Couder \n Signed-off-by: Junio C Hamano \n @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_name = advertised->name;\n \tconst char *remote_url = advertised->url;\n \n -+\tif (!remote_url || !*remote_url) {\n -+\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n -+\t\t\t \"ignoring this remote\"), remote_name);\n -+\t\treturn 0;\n -+\t}\n ++\tif (!remote_url || !*remote_url)\n ++\t\tBUG(\"no or empty URL advertised for remote '%s'; \"\n ++\t\t \"this remote should have been rejected earlier\",\n ++\t\t remote_name);\n +\n \tif (accept == ACCEPT_ALL)\n \t\treturn all_fields_match(advertised, config_info, NULL);\n 5: c51bed2fe6 ! 6: 80cfbccba9 promisor-remote: refactor has_control_char()\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor has_control_char()\n \n - In a following commit we are going to check if some strings contain\n - control character, so let's refactor the logic to do that in a new\n + In a future commit we are going to check if some strings contain\n + control characters, so let's refactor the logic to do that in a new\n has_control_char() helper function.\n \n + It cleans up the code a bit anyway.\n +\n Signed-off-by: Christian Couder \n Signed-off-by: Junio C Hamano \n \n 6: b063f4596a ! 7: 3f2d6f8a50 promisor-remote: refactor accept_from_server()\n @@ Metadata\n ## Commit message ##\n promisor-remote: refactor accept_from_server()\n \n - In a following commit, we are going to add more logic to\n + In future commits, we are going to add more logic to\n filter_promisor_remote() which is already doing a lot of things.\n \n Let's alleviate that by moving the logic that checks and validates the\n 7: 1e378b33c0 ! 8: dff4d339c8 promisor-remote: keep accepted promisor_info structs alive\n @@ Commit message\n protocol reply, apply advertised filters, and mark remotes as\n accepted, rather than iterating three separate structures.\n \n - This refactoring also prepares for a following commit that will add a\n + This refactoring also prepares for a future commit that will add a\n 'local_name' member to 'struct promisor_info'. Since struct instances\n stay alive, downstream code will be able to simply read both names\n from them rather than needing yet another parallel strvec.\n 8: 2304313704 = 9: 1fb08184c4 promisor-remote: remove the 'accepted' strvec\n 9: 86d6a9e385 < -: ---------- promisor-remote: add 'local_name' to 'struct promisor_info'\n12: 5c4461affc ! 10: 7cb0268075 t5710: use proper file:// URIs for absolute paths\n @@ Commit message\n (`file://D:/a/repo`). Standard URI parsers misinterpret this format,\n treating `D:` as the host rather than part of the absolute path.\n \n + This is to be expected because RFC 8089 says that the `//` prefix with\n + an empty local host must be followed by an absolute path starting with\n + a slash.\n +\n While this hasn't broken the existing tests (because the old\n - `promisor.acceptFromServer` logic relies entirely on strict `strcmp`\n + `promisor.acceptFromServer` logic relies entirely on strict `strcmp()`\n without normalizing the URLs), it will break future commits that pass\n - these URLs through `url_normalize()`.\n + these URLs through `url_normalize()` or similar functions.\n \n To future-proof the tests and ensure cross-platform URI compliance,\n let's introduce a $PWD_URL helper that explicitly guarantees a leading\n13: 294155978a < -: ---------- promisor-remote: introduce promisor.acceptFromServerUrl\n14: f17a3ed35b < -: ---------- promisor-remote: trust known remotes matching acceptFromServerUrl\n15: db503a285d < -: ---------- promisor-remote: auto-configure unknown remotes\n16: d92889efa9 < -: ---------- doc: promisor: improve acceptFromServer entry\n\n\n\n\n> Christian Couder (10):\n> promisor-remote: try accepted remotes before others in get_direct()\n> promisor-remote: pass config entry to all_fields_match() directly\n> promisor-remote: clarify that a remote is ignored\n> promisor-remote: reject empty name or URL in advertised remote\n> promisor-remote: refactor should_accept_remote() control flow\n> promisor-remote: refactor has_control_char()\n> promisor-remote: refactor accept_from_server()\n> promisor-remote: keep accepted promisor_info structs alive\n> promisor-remote: remove the 'accepted' strvec\n> t5710: use proper file:// URIs for absolute paths\n>\n> Documentation/gitprotocol-v2.adoc | 4 +\n> promisor-remote.c | 207 +++++++++++++++-----------\n> t/t5710-promisor-remote-capability.sh | 122 ++++++++++++---\n> 3 files changed, 223 insertions(+), 110 deletions(-)\n"},{"id":"540842","messageId":"CAP8UFD3CMwTjC36Grhb6_6q0SBWtTwBX4_kM5sf+peTgd7P3dA@mail.gmail.com","threadId":"65413","inReplyTo":"xmqqa4vlu1ij.fsf@gitster.g","subject":"Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-03T08:16:15Z","receivedAt":"2026-04-03T08:16:28Z","isPatch":true,"body":"On Thu, Apr 2, 2026 at 11:58 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > +# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn\n> > +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.\n> > +# On Unix, the path already starts with '/', so this is a no-op.\n> > +pwd_path=$(pwd)\n> > +case \"$pwd_path\" in\n> > +[a-zA-Z]:*) pwd_path=\"/$pwd_path\" ;;\n> > +esac\n> > +PWD_URL=\"file://$pwd_path\"\n> > +# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.\n> > +ENCODED_PWD_URL=\"file://$(echo \"$pwd_path\" | sed \"s/ /%20/g\")\"\n>\n> Two comments.\n>\n> - I was a bit surprised that these are not given as functions but\n> as variables, as a caller that chdirs around in the trash\n> directory would want a URL that points at its current working\n> directory (the expectation is from \"pwd\" in the name PWD_URL).\n> But a variable based interface \"Here is the URL that corresponds\n> to the trash directory\" is OK and probably easier to use than\n> \"give me the URL corresponding to my current working directory\",\n> simply because it allows a caller to append some string to it to\n> come up with a URL for any subdirectory on its own without\n> actually going there. But in that case, the name PWD_URL would\n> become misleading, as it is PWD as of the moment the variable\n> gets defined, and the true meaning of the variable is not \"URL\n> for the current directory\", but \"URL for the trash directory\" is\n> more usable definition.\n\nRight. I will use something like the following then:\n\nTRASH_DIRECTORY_URL=\"file://$pwd_path\"\nENCODED_TRASH_DIRECTORY_URL=\"file://$encoded_path\"\n\n> - Is it sufficient to only special case SP? My repository may be\n> $HOME/w/git.git, for example, and the trash repository may be\n> \"$HOME/w/git.git/t/trash directory.t5710/\", so you need to cope\n> with SP between \"trash\" and \"directory\" the test framework adds\n> (to force you to be careful), but the test framework does not\n> control what can be in the leading $HOME part.\n\nI think it's quite unlikely for users to have strange characters in\n$HOME, but OK let's be cautious and encode a larger set than just\nspace characters. And to be extra safe, let's also skip all the tests\nif we detect a special character not in the larger set.\n\nSo something like:\n\n# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),\nand those percent-encoded below (% space = ; ,)\ncase \"$pwd_path\" in\n*[!a-zA-Z0-9_.~/:%\\ =,;-]*)\n skip_all=\"PWD contains unsupported special characters\"\n test_done\n ;;\nesac\n\nencoded_path=$(printf \"%s\" \"$pwd_path\" | sed -e 's/%/%25/g' -e 's/\n/%20/g' -e 's/=/%3D/g' -e 's/;/%3B/g' -e 's/,/%2C/g')\n"},{"id":"541051","messageId":"20260407115243.358642-1-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260402070613.85934-1-christian.couder@gmail.com","subject":"[PATCH v2 00/10] Prepare for advertised remotes auto-configure via URL allowlist","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:33Z","receivedAt":"2026-04-07T11:53:02Z","isPatch":true,"body":"Recently, I sent a 16 patch long series that makes it possible for\npromisor remotes advertised through the \"promisor-remote\" protocol\ncapability to be auto-configured on the client side via a URL\nallowlist configured using a new `promisor.acceptFromServerUrl`\nconfiguration variable:\n\nhttps://lore.kernel.org/git/20260323080520.887550-1-christian.couder@gmail.com/\n\nI got the suggestion to split the 16 patches series into two smaller\nseries, starting with a preparatory series. So here is this\npreparatory series. It's a mix of mostly small fixes, refactorings and\ncleanups.\n\nHigh level description of the patches\n=====================================\n\n - Patches 1-4/10 are fixes:\n\n - Patch 1/10 is the most significant. The others are relatively\n small.\n \n - Patch 4/10 is the only new in this series (while all the others\n were in the previous series). It prepares for Patch 5/10.\n\n - Patches 5-10/10 are refactorings and cleanups:\n\n - Patches 5-7/10 are relatively small independents cleanups or\n refactorings.\n\n - Patches 8-9/10 are related refactorings simplifying the data\n structures used in filter_promisor_remote() and\n promisor_remote_reply().\n\n - Patch 10/10 is cleaning up the 'file://' URIs with absolute paths\n in the test script.\n\nChanges since v1\n================\n\nThanks to Patrick and Junio for reviewing the previous versions of\nthis series.\n\nOnly patch 10/10 (\"t5710: use proper file:// URIs for absolute paths\")\nchanged since v1:\n\n - PWD_URL and ENCODED_PWD_URL have been renamed TRASH_DIRECTORY_URL\n and ENCODED_TRASH_DIRECTORY_URL respectively.\n\n - Instead of percent-encoding only space characters, we now encode\n '%', ' ', '=', ',' and ';'.\n\n - The test script is skipped altogether if there are other characters\n than those we encode or expect in the current directory path.\n\n - The tests introduced by commit 1/10 now also benefit from the\n TRASH_DIRECTORY_URL and ENCODED_TRASH_DIRECTORY_URL variables.\n\n - Commit message and code comments are improved a bit.\n\nCI tests\n========\n\nThey all pass, see:\n\nhttps://github.com/chriscool/git/actions/runs/24076952684\n\nRange-diff since v1\n===================\n\n 1: 906c5f6fbb = 1: 6687a0fe3f promisor-remote: try accepted remotes before others in get_direct()\n 2: 830eb10097 = 2: 23869a4509 promisor-remote: pass config entry to all_fields_match() directly\n 3: b18cdf2fcb = 3: 16d5e0b7ef promisor-remote: clarify that a remote is ignored\n 4: b2125ec770 = 4: f55f6e8c14 promisor-remote: reject empty name or URL in advertised remote\n 5: e69abb1256 = 5: 52cd36ab64 promisor-remote: refactor should_accept_remote() control flow\n 6: 772ea53e9e = 6: 286ae3b4e3 promisor-remote: refactor has_control_char()\n 7: 79cf945c24 = 7: 4cb239cd24 promisor-remote: refactor accept_from_server()\n 8: b953e2491e = 8: 7bfe26d5d4 promisor-remote: keep accepted promisor_info structs alive\n 9: 3fce719fc0 = 9: 9ffa80444f promisor-remote: remove the 'accepted' strvec\n10: e54af9e176 ! 10: b77b449c76 t5710: use proper file:// URIs for absolute paths\n @@ Commit message\n these URLs through `url_normalize()` or similar functions.\n \n To future-proof the tests and ensure cross-platform URI compliance,\n - let's introduce a $PWD_URL helper that explicitly guarantees a leading\n - slash for the path component, ensuring valid 3-slash `file:///` URIs on\n - all operating systems.\n + let's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly\n + guarantees a leading slash for the path component, ensuring valid\n + 3-slash `file:///` URIs on all operating systems.\n \n - While at it, let's also introduce $ENCODED_PWD_URL to handle spaces in\n - directory paths (which is needed for URL glob pattern matching).\n + While at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to\n + handle some common special characters in directory paths.\n \n - Then let's replace all instances of `file://$(pwd)` with $PWD_URL across\n - the test script, and let's simplify the `ENCODED_URL` constructions in\n - the `sendFields` and `checkFields` tests to use $ENCODED_PWD_URL\n - directly.\n + To be extra safe, let's skip all the tests if there are uncommon\n + special characters in the directory path.\n +\n + Then let's replace all instances of `file://$(pwd)` with\n + $TRASH_DIRECTORY_URL across the test script, and let's simplify the\n + `sendFields` and `checkFields` tests to use\n + $ENCODED_TRASH_DIRECTORY_URL directly.\n \n Signed-off-by: Christian Couder \n \n @@ t/t5710-promisor-remote-capability.sh: copy_to_lop () {\n \tcp \"$path\" \"$path2\"\n }\n \n -+# On Windows, 'pwd' returns a path like 'D:/foo/bar'. Prepend '/' to turn\n ++# On Windows, `pwd` returns a path like 'D:/foo/bar'. Prepend '/' to turn\n +# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.\n +# On Unix, the path already starts with '/', so this is a no-op.\n +pwd_path=$(pwd)\n +case \"$pwd_path\" in\n +[a-zA-Z]:*) pwd_path=\"/$pwd_path\" ;;\n +esac\n -+PWD_URL=\"file://$pwd_path\"\n -+# Same as PWD_URL but with spaces percent-encoded, for use in URL patterns.\n -+ENCODED_PWD_URL=\"file://$(echo \"$pwd_path\" | sed \"s/ /%20/g\")\"\n ++\n ++# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),\n ++# and those percent-encoded below (% space = , ;)\n ++rest=$(printf \"%s\" \"$pwd_path\" | tr -d 'a-zA-Z0-9_.~/:% =,;-')\n ++if test -n \"$rest\"\n ++then\n ++\tskip_all=\"PWD contains unsupported special characters\"\n ++\ttest_done\n ++fi\n ++\n ++TRASH_DIRECTORY_URL=\"file://$pwd_path\"\n ++\n ++encoded_path=$(printf \"%s\" \"$pwd_path\" |\n ++\t sed -e 's/%/%25/g' -e 's/ /%20/g' -e 's/=/%3D/g' \\\n ++\t\t -e 's/;/%3B/g' -e 's/,/%2C/g')\n ++\n ++ENCODED_TRASH_DIRECTORY_URL=\"file://$encoded_path\"\n +\n test_expect_success \"setup for testing promisor remote advertisement\" '\n \t# Create another bare repo called \"lop\" (for Large Object Promisor)\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"setup for testing pr\n \n \t# Configure lop as promisor remote for server\n -\tgit -C server remote add lop \"file://$(pwd)/lop\" &&\n -+\tgit -C server remote add lop \"$PWD_URL/lop\" &&\n ++\tgit -C server remote add lop \"$TRASH_DIRECTORY_URL/lop\" &&\n \tgit -C server config remote.lop.promisor true &&\n \n \tgit -C lop config uploadpack.allowFilter true &&\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=None \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"init + fetch with pr\n \tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n -\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n -\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n -+\tgit -C client config remote.lop.url \"$PWD_URL/lop\" &&\n -+\tgit -C client config remote.server.url \"$PWD_URL/server\" &&\n ++\tgit -C client config remote.lop.url \"$TRASH_DIRECTORY_URL/lop\" &&\n ++\tgit -C client config remote.server.url \"$TRASH_DIRECTORY_URL/server\" &&\n \tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n \tgit -C client config promisor.acceptfromserver All &&\n \tGIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n +@@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with two promisors but only one advertised\" '\n + \tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n + \t\t-c remote.unused_lop.promisor=true \\\n + \t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n +-\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n ++\t\t-c remote.unused_lop.url=\"$TRASH_DIRECTORY_URL/unused_lop\" \\\n + \t\t-c remote.lop.promisor=true \\\n + \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n +-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n + \t\t-c promisor.acceptfromserver=All \\\n + \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n + \n +@@ t/t5710-promisor-remote-capability.sh: test_expect_success \"init + fetch two promisors but only one advertised\" '\n + \tgit -C client init &&\n + \tgit -C client config remote.unused_lop.promisor true &&\n + \tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n +-\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n ++\tgit -C client config remote.unused_lop.url \"$TRASH_DIRECTORY_URL/unused_lop\" &&\n + \tgit -C client config remote.lop.promisor true &&\n + \tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n +-\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n +-\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n ++\tgit -C client config remote.lop.url \"$TRASH_DIRECTORY_URL/lop\" &&\n ++\tgit -C client config remote.server.url \"$TRASH_DIRECTORY_URL/server\" &&\n + \tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n + \tgit -C client config promisor.acceptfromserver All &&\n + \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownNam\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \\\n \t\t-c remote.serverTwo.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.serverTwo.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.serverTwo.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.serverTwo.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownUrl\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/serverTwo\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/serverTwo\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/serverTwo\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownUrl\n \ttest_when_finished \"rm -rf client\" &&\n \n -\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n -+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$PWD_URL/lop\\\"\" &&\n ++\ttest_when_finished \"git -C server config set remote.lop.url \\\"$TRASH_DIRECTORY_URL/lop\\\"\" &&\n \tgit -C server config unset remote.lop.url &&\n \n \t# Clone from server to create a client\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownUrl\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownUrl\n \ttest_when_finished \"rm -rf client\" &&\n \n -\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n -+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$PWD_URL/lop\\\"\" &&\n ++\ttest_when_finished \"git -C server config set remote.lop.url \\\"$TRASH_DIRECTORY_URL/lop\\\"\" &&\n \tgit -C server config set remote.lop.url \"\" &&\n \n \t# Clone from server to create a client\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with 'KnownUrl\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n \t# Check that fields are properly transmitted\n -\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n -\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n -+\tPR1=\"name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none\" &&\n ++\tPR1=\"name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop;otherLop\" trace &&\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t-c promisor.checkFields=partialcloneFilter \\\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \t# Check that fields are properly transmitted\n -\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n -\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n -+\tPR1=\"name=lop,url=$ENCODED_PWD_URL/lop,partialCloneFilter=blob:none\" &&\n ++\tPR1=\"name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop\" trace &&\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c remote.lop.token=\"fooYYY\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone and fetch with\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=auto server client 2>err &&\n \n @@ t/t5710-promisor-remote-capability.sh: test_expect_success \"clone with promisor.\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n -\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n -+\t\t-c remote.lop.url=\"$PWD_URL/lop\" \\\n ++\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n\nChristian Couder (10):\n promisor-remote: try accepted remotes before others in get_direct()\n promisor-remote: pass config entry to all_fields_match() directly\n promisor-remote: clarify that a remote is ignored\n promisor-remote: reject empty name or URL in advertised remote\n promisor-remote: refactor should_accept_remote() control flow\n promisor-remote: refactor has_control_char()\n promisor-remote: refactor accept_from_server()\n promisor-remote: keep accepted promisor_info structs alive\n promisor-remote: remove the 'accepted' strvec\n t5710: use proper file:// URIs for absolute paths\n\n Documentation/gitprotocol-v2.adoc | 4 +\n promisor-remote.c | 207 +++++++++++++++-----------\n t/t5710-promisor-remote-capability.sh | 138 ++++++++++++++---\n 3 files changed, 238 insertions(+), 111 deletions(-)\n\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541052","messageId":"20260407115243.358642-2-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 01/10] promisor-remote: try accepted remotes before others in get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:34Z","receivedAt":"2026-04-07T11:53:03Z","isPatch":true,"body":"When a server advertises promisor remotes and the client accepts some\nof them, those remotes carry the server's intent: 'fetch missing\nobjects preferably from here', and the client agrees with that for the\nremotes it accepts.\n\nHowever promisor_remote_get_direct() actually iterates over all\npromisor remotes in list order, which is the order they appear in the\nconfig files (except perhaps for the one appearing in the\n`extensions.partialClone` config variable which is tried last).\n\nThis means an existing, but not accepted, promisor remote, could be\ntried before the accepted ones, which does not reflect the intent of\nthe agreement between client and server.\n\nIf the client doesn't care about what the server suggests, it should\naccept nothing and rely on its remotes as they are already configured.\n\nTo better reflect the agreement between client and server, let's make\npromisor_remote_get_direct() try the accepted promisor remotes before\nthe non-accepted ones.\n\nConcretely, let's extract a try_promisor_remotes() helper and call it\ntwice from promisor_remote_get_direct():\n\n- first with an `accepted_only=true` argument to try only the accepted\n remotes,\n- then with `accepted_only=false` to fall back to any remaining remote.\n\nEnsuring that accepted remotes are preferred will be even more\nimportant if in the future a mechanism is developed to allow the\nclient to auto-configure remotes that the server advertises. This will\nin particular avoid fetching from the server (which is already\nconfigured as a promisor remote) before trying the auto-configured\nremotes, as these new remotes would likely appear at the end of the\nconfig file, and as the server might not appear in the\n`extensions.partialClone` config variable.\n\nSigned-off-by: Christian Couder \n---\n Documentation/gitprotocol-v2.adoc | 4 ++\n promisor-remote.c | 44 ++++++++++++-----\n t/t5710-promisor-remote-capability.sh | 69 +++++++++++++++++++++++++++\n 3 files changed, 104 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/gitprotocol-v2.adoc b/Documentation/gitprotocol-v2.adoc\nindex f985cb4c47..4fcb1a7bda 100644\n--- a/Documentation/gitprotocol-v2.adoc\n+++ b/Documentation/gitprotocol-v2.adoc\n@@ -848,6 +848,10 @@ advertised, it can reply with \"promisor-remote=\" where\n where `pr-name` is the urlencoded name of a promisor remote the server\n advertised and the client accepts.\n \n+The promisor remotes that the client accepted will be tried before the\n+other configured promisor remotes when the client attempts to fetch\n+missing objects.\n+\n Note that, everywhere in this document, the ';' and ',' characters\n MUST be encoded if they appear in `pr-name` or `field-value`.\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 96fa215b06..7ce7d22f95 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n }\n \n+static int try_promisor_remotes(struct repository *repo,\n+\t\t\t\tstruct object_id **remaining_oids,\n+\t\t\t\tint *remaining_nr, int *to_free,\n+\t\t\t\tbool accepted_only)\n+{\n+\tstruct promisor_remote *r = repo->promisor_remote_config->promisors;\n+\n+\tfor (; r; r = r->next) {\n+\t\tif (accepted_only != r->accepted)\n+\t\t\tcontinue;\n+\t\tif (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {\n+\t\t\tif (*remaining_nr == 1)\n+\t\t\t\tcontinue;\n+\t\t\t*remaining_nr = remove_fetched_oids(repo, remaining_oids,\n+\t\t\t\t\t\t\t *remaining_nr, *to_free);\n+\t\t\tif (*remaining_nr) {\n+\t\t\t\t*to_free = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\t\treturn 1; /* all fetched */\n+\t}\n+\treturn 0;\n+}\n+\n void promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tconst struct object_id *oids,\n \t\t\t\tint oid_nr)\n {\n-\tstruct promisor_remote *r;\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n@@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo,\n \n \tpromisor_remote_init(repo);\n \n-\tfor (r = repo->promisor_remote_config->promisors; r; r = r->next) {\n-\t\tif (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) {\n-\t\t\tif (remaining_nr == 1)\n-\t\t\t\tcontinue;\n-\t\t\tremaining_nr = remove_fetched_oids(repo, &remaining_oids,\n-\t\t\t\t\t\t\t remaining_nr, to_free);\n-\t\t\tif (remaining_nr) {\n-\t\t\t\tto_free = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n+\t/* Try accepted remotes first (those the server told us to use) */\n+\tif (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,\n+\t\t\t\t &to_free, true))\n+\t\tgoto all_fetched;\n+\tif (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,\n+\t\t\t\t &to_free, false))\n \t\tgoto all_fetched;\n-\t}\n \n \tfor (i = 0; i < remaining_nr; i++) {\n \t\tif (is_promisor_object(repo, &remaining_oids[i]))\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex 357822c01a..bf0eed9f10 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -166,6 +166,75 @@ test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n \tcheck_missing_objects server 1 \"$oid\"\n '\n \n+test_expect_success \"clone with two promisors but only one advertised\" '\n+\tgit -C server config promisor.advertise true &&\n+\ttest_when_finished \"rm -rf client unused_lop\" &&\n+\n+\t# Create a promisor that will be configured but not be used\n+\tgit init --bare unused_lop &&\n+\n+\t# Clone from server to create a client\n+\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n+\t\t-c remote.unused_lop.promisor=true \\\n+\t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n+\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n+\t\t-c remote.lop.promisor=true \\\n+\t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n+\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c promisor.acceptfromserver=All \\\n+\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n+\n+\t# Check that \"unused_lop\" appears before \"lop\" in the config\n+\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" \"origin\" >expect &&\n+\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Check that \"lop\" was tried\n+\ttest_grep \" fetch lop \" trace &&\n+\t# Check that \"unused_lop\" was not contacted\n+\t# This means \"lop\", the accepted promisor, was tried first\n+\ttest_grep ! \" fetch unused_lop \" trace &&\n+\n+\t# Check that the largest object is still missing on the server\n+\tcheck_missing_objects server 1 \"$oid\"\n+'\n+\n+test_expect_success \"init + fetch two promisors but only one advertised\" '\n+\tgit -C server config promisor.advertise true &&\n+\ttest_when_finished \"rm -rf client unused_lop\" &&\n+\n+\t# Create a promisor that will be configured but not be used\n+\tgit init --bare unused_lop &&\n+\n+\tmkdir client &&\n+\tgit -C client init &&\n+\tgit -C client config remote.unused_lop.promisor true &&\n+\tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n+\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n+\tgit -C client config remote.lop.promisor true &&\n+\tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n+\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n+\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n+\tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n+\tgit -C client config promisor.acceptfromserver All &&\n+\n+\t# Check that \"unused_lop\" appears before \"lop\" in the config\n+\tprintf \"remote.%s.promisor true\\n\" \"unused_lop\" \"lop\" >expect &&\n+\tgit -C client config get --all --show-names --regexp \"^remote\\..*\\.promisor$\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n+\n+\t# Check that \"lop\" was tried\n+\ttest_grep \" fetch lop \" trace &&\n+\t# Check that \"unused_lop\" was not contacted\n+\t# This means \"lop\", the accepted promisor, was tried first\n+\ttest_grep ! \" fetch unused_lop \" trace &&\n+\n+\t# Check that the largest object is still missing on the server\n+\tcheck_missing_objects server 1 \"$oid\"\n+'\n+\n test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541053","messageId":"20260407115243.358642-3-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 02/10] promisor-remote: pass config entry to all_fields_match() directly","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:35Z","receivedAt":"2026-04-07T11:53:05Z","isPatch":true,"body":"The `in_list == 0` path of all_fields_match() looks up the remote in\n`config_info` by `advertised->name` repeatedly, even though every\ncaller in should_accept_remote() has already performed this\nlookup and holds the result in `p`.\n\nTo avoid this useless work, let's replace the `int in_list`\nparameter with a `struct promisor_info *config_entry` pointer:\n\n - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as\n the old `in_list == 1` path did.\n\n - When non-NULL: match against that single config entry directly,\n avoiding the redundant string_list_lookup() call.\n\nThis removes the hidden dependency on `advertised->name` inside\nall_fields_match(), which would be wrong if in the future\nauto-configured remotes are implemented, as the local config name may\ndiffer from the server's advertised name.\n\nWhile at it, let's also add a comment before all_fields_match() and\nmatch_field_against_config() to help understand how things work and\nhelp avoid similar issues.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 36 ++++++++++++++++++++++++------------\n 1 file changed, 24 insertions(+), 12 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 7ce7d22f95..6c935f855a 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -575,6 +575,12 @@ enum accept_promisor {\n \tACCEPT_ALL\n };\n \n+/*\n+ * Check if a specific field and its advertised value match the local\n+ * configuration of a given promisor remote.\n+ *\n+ * Returns 1 if they match, 0 otherwise.\n+ */\n static int match_field_against_config(const char *field, const char *value,\n \t\t\t\t struct promisor_info *config_info)\n {\n@@ -586,9 +592,18 @@ static int match_field_against_config(const char *field, const char *value,\n \treturn 0;\n }\n \n+/*\n+ * Check that the advertised fields match the local configuration.\n+ *\n+ * When 'config_entry' is NULL (ACCEPT_ALL mode), every checked field\n+ * must match at least one remote in 'config_info'.\n+ *\n+ * When 'config_entry' points to a specific remote's config, the\n+ * checked fields are compared against that single remote only.\n+ */\n static int all_fields_match(struct promisor_info *advertised,\n \t\t\t struct string_list *config_info,\n-\t\t\t int in_list)\n+\t\t\t struct promisor_info *config_entry)\n {\n \tstruct string_list *fields = fields_checked();\n \tstruct string_list_item *item_checked;\n@@ -597,7 +612,6 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\tint match = 0;\n \t\tconst char *field = item_checked->string;\n \t\tconst char *value = NULL;\n-\t\tstruct string_list_item *item;\n \n \t\tif (!strcasecmp(field, promisor_field_filter))\n \t\t\tvalue = advertised->filter;\n@@ -607,7 +621,11 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\tif (!value)\n \t\t\treturn 0;\n \n-\t\tif (in_list) {\n+\t\tif (config_entry) {\n+\t\t\tmatch = match_field_against_config(field, value,\n+\t\t\t\t\t\t\t config_entry);\n+\t\t} else {\n+\t\t\tstruct string_list_item *item;\n \t\t\tfor_each_string_list_item(item, config_info) {\n \t\t\t\tstruct promisor_info *p = item->util;\n \t\t\t\tif (match_field_against_config(field, value, p)) {\n@@ -615,12 +633,6 @@ static int all_fields_match(struct promisor_info *advertised,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n-\t\t} else {\n-\t\t\titem = string_list_lookup(config_info, advertised->name);\n-\t\t\tif (item) {\n-\t\t\t\tstruct promisor_info *p = item->util;\n-\t\t\t\tmatch = match_field_against_config(field, value, p);\n-\t\t\t}\n \t\t}\n \n \t\tif (!match)\n@@ -640,7 +652,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_url = advertised->url;\n \n \tif (accept == ACCEPT_ALL)\n-\t\treturn all_fields_match(advertised, config_info, 1);\n+\t\treturn all_fields_match(advertised, config_info, NULL);\n \n \t/* Get config info for that promisor remote */\n \titem = string_list_lookup(config_info, remote_name);\n@@ -652,7 +664,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \tp = item->util;\n \n \tif (accept == ACCEPT_KNOWN_NAME)\n-\t\treturn all_fields_match(advertised, config_info, 0);\n+\t\treturn all_fields_match(advertised, config_info, p);\n \n \tif (accept != ACCEPT_KNOWN_URL)\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n@@ -663,7 +675,7 @@ static int should_accept_remote(enum accept_promisor accept,\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n-\t\treturn all_fields_match(advertised, config_info, 0);\n+\t\treturn all_fields_match(advertised, config_info, p);\n \n \twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n \t\tremote_name, p->url, remote_url);\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541054","messageId":"20260407115243.358642-4-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 03/10] promisor-remote: clarify that a remote is ignored","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:36Z","receivedAt":"2026-04-07T11:53:06Z","isPatch":true,"body":"In should_accept_remote() and parse_one_advertised_remote(), when a\nremote is ignored, we tell users why it is ignored in a warning, but we\ndon't tell them that the remote is actually ignored.\n\nLet's clarify that, so users have a better idea of what's actually\nhappening.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 6c935f855a..8e062ec160 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n \n \tif (!remote_url || !*remote_url) {\n-\t\twarning(_(\"no or empty URL advertised for remote '%s'\"), remote_name);\n+\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n+\t\t\t \"ignoring this remote\"), remote_name);\n \t\treturn 0;\n \t}\n \n \tif (!strcmp(p->url, remote_url))\n \t\treturn all_fields_match(advertised, config_info, p);\n \n-\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s'\"),\n-\t\tremote_name, p->url, remote_url);\n+\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n+\t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n \n \treturn 0;\n }\n@@ -722,8 +723,8 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n \tstring_list_clear(&elem_list, 0);\n \n \tif (!info->name || !info->url) {\n-\t\twarning(_(\"server advertised a promisor remote without a name or URL: %s\"),\n-\t\t\tremote_info);\n+\t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n+\t\t\t \"ignoring this remote\"), remote_info);\n \t\tpromisor_info_free(info);\n \t\treturn NULL;\n \t}\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541055","messageId":"20260407115243.358642-5-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 04/10] promisor-remote: reject empty name or URL in advertised remote","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:37Z","receivedAt":"2026-04-07T11:53:07Z","isPatch":true,"body":"In parse_one_advertised_remote(), we check for a NULL remote name and\nremote URL, but not for empty ones. An empty URL seems possible as\nurl_percent_decode(\"\") doesn't return NULL.\n\nIn promisor_config_info_list(), we ignore remotes with empty URLs, so a\nGit server should not advertise remotes with empty URLs. It's possible\nthat a buggy or malicious server would do it though.\n\nSo let's tighten the check in parse_one_advertised_remote() to also\nreject empty strings at parse time.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8e062ec160..8322349ae8 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n \n \tstring_list_clear(&elem_list, 0);\n \n-\tif (!info->name || !info->url) {\n+\tif (!info->name || !*info->name || !info->url || !*info->url) {\n \t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n \t\t\t \"ignoring this remote\"), remote_info);\n \t\tpromisor_info_free(info);\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541057","messageId":"20260407115243.358642-6-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 05/10] promisor-remote: refactor should_accept_remote() control flow","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:38Z","receivedAt":"2026-04-07T11:53:09Z","isPatch":true,"body":"A previous commit made sure we now reject empty URLs early at parse\ntime. This makes the existing warning() in case a remote URL is NULL\nor empty very unlikely to be useful.\n\nIn future work, we also plan to add URL-based acceptance logic into\nshould_accept_remote().\n\nTo adapt to previous changes and prepare for upcoming changes, let's\nrestructure the control flow in should_accept_remote().\n\nConcretely, let's:\n\n - Replace the warning() in case of an empty URL with a BUG(), as a\n previous commit made sure empty URLs are rejected early at parse\n time.\n\n - Move that modified empty-URL check to the very top of the function,\n so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is\n covered.\n\n - Invert the URL comparison: instead of returning on match and\n warning on mismatch, return early on mismatch and let the match\n case fall through. This opens a single exit path at the bottom of\n the function for future commits to extend.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8322349ae8..5860a3d3f3 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -651,6 +651,11 @@ static int should_accept_remote(enum accept_promisor accept,\n \tconst char *remote_name = advertised->name;\n \tconst char *remote_url = advertised->url;\n \n+\tif (!remote_url || !*remote_url)\n+\t\tBUG(\"no or empty URL advertised for remote '%s'; \"\n+\t\t \"this remote should have been rejected earlier\",\n+\t\t remote_name);\n+\n \tif (accept == ACCEPT_ALL)\n \t\treturn all_fields_match(advertised, config_info, NULL);\n \n@@ -669,19 +674,14 @@ static int should_accept_remote(enum accept_promisor accept,\n \tif (accept != ACCEPT_KNOWN_URL)\n \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n \n-\tif (!remote_url || !*remote_url) {\n-\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n-\t\t\t \"ignoring this remote\"), remote_name);\n+\tif (strcmp(p->url, remote_url)) {\n+\t\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n+\t\t\t \"ignoring this remote\"),\n+\t\t\tremote_name, p->url, remote_url);\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(p->url, remote_url))\n-\t\treturn all_fields_match(advertised, config_info, p);\n-\n-\twarning(_(\"known remote named '%s' but with URL '%s' instead of '%s', \"\n-\t\t \"ignoring this remote\"), remote_name, p->url, remote_url);\n-\n-\treturn 0;\n+\treturn all_fields_match(advertised, config_info, p);\n }\n \n static int skip_field_name_prefix(const char *elem, const char *field_name, const char **value)\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541056","messageId":"20260407115243.358642-7-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 06/10] promisor-remote: refactor has_control_char()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:39Z","receivedAt":"2026-04-07T11:53:10Z","isPatch":true,"body":"In a future commit we are going to check if some strings contain\ncontrol characters, so let's refactor the logic to do that in a new\nhas_control_char() helper function.\n\nIt cleans up the code a bit anyway.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 24 ++++++++++++++----------\n 1 file changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 5860a3d3f3..d60518f19c 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -642,6 +642,14 @@ static int all_fields_match(struct promisor_info *advertised,\n \treturn 1;\n }\n \n+static bool has_control_char(const char *s)\n+{\n+\tfor (const char *c = s; *c; c++)\n+\t\tif (iscntrl(*c))\n+\t\t\treturn true;\n+\treturn false;\n+}\n+\n static int should_accept_remote(enum accept_promisor accept,\n \t\t\t\tstruct promisor_info *advertised,\n \t\t\t\tstruct string_list *config_info)\n@@ -772,18 +780,14 @@ static bool valid_filter(const char *filter, const char *remote_name)\n \treturn !res;\n }\n \n-/* Check that a token doesn't contain any control character */\n static bool valid_token(const char *token, const char *remote_name)\n {\n-\tconst char *c = token;\n-\n-\tfor (; *c; c++)\n-\t\tif (iscntrl(*c)) {\n-\t\t\twarning(_(\"invalid token '%s' for remote '%s' \"\n-\t\t\t\t \"will not be stored\"),\n-\t\t\t\ttoken, remote_name);\n-\t\t\treturn false;\n-\t\t}\n+\tif (has_control_char(token)) {\n+\t\twarning(_(\"invalid token '%s' for remote '%s' \"\n+\t\t\t \"will not be stored\"),\n+\t\t\ttoken, remote_name);\n+\t\treturn false;\n+\t}\n \n \treturn true;\n }\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541058","messageId":"20260407115243.358642-8-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 07/10] promisor-remote: refactor accept_from_server()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:40Z","receivedAt":"2026-04-07T11:53:11Z","isPatch":true,"body":"In future commits, we are going to add more logic to\nfilter_promisor_remote() which is already doing a lot of things.\n\nLet's alleviate that by moving the logic that checks and validates the\nvalue of the `promisor.acceptFromServer` config variable into its own\naccept_from_server() helper function.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex d60518f19c..8d80ef6040 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -862,20 +862,12 @@ static bool promisor_store_advertised_fields(struct promisor_info *advertised,\n \treturn reload_config;\n }\n \n-static void filter_promisor_remote(struct repository *repo,\n-\t\t\t\t struct strvec *accepted,\n-\t\t\t\t const char *info)\n+static enum accept_promisor accept_from_server(struct repository *repo)\n {\n \tconst char *accept_str;\n \tenum accept_promisor accept = ACCEPT_NONE;\n-\tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n-\tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n-\tstruct store_info *store_info = NULL;\n-\tstruct string_list_item *item;\n-\tbool reload_config = false;\n-\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n \n-\tif (!repo_config_get_string_tmp(the_repository, \"promisor.acceptfromserver\", &accept_str)) {\n+\tif (!repo_config_get_string_tmp(repo, \"promisor.acceptfromserver\", &accept_str)) {\n \t\tif (!*accept_str || !strcasecmp(\"None\", accept_str))\n \t\t\taccept = ACCEPT_NONE;\n \t\telse if (!strcasecmp(\"KnownUrl\", accept_str))\n@@ -889,6 +881,21 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\t\taccept_str, \"promisor.acceptfromserver\");\n \t}\n \n+\treturn accept;\n+}\n+\n+static void filter_promisor_remote(struct repository *repo,\n+\t\t\t\t struct strvec *accepted,\n+\t\t\t\t const char *info)\n+{\n+\tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n+\tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n+\tstruct store_info *store_info = NULL;\n+\tstruct string_list_item *item;\n+\tbool reload_config = false;\n+\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n+\tenum accept_promisor accept = accept_from_server(repo);\n+\n \tif (accept == ACCEPT_NONE)\n \t\treturn;\n \n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541059","messageId":"20260407115243.358642-9-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 08/10] promisor-remote: keep accepted promisor_info structs alive","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:41Z","receivedAt":"2026-04-07T11:53:12Z","isPatch":true,"body":"In filter_promisor_remote(), the instances of `struct promisor_info`\nfor accepted remotes are dismantled into separate parallel data\nstructures (the 'accepted' strvec for server names, and\n'accepted_filters' for filter strings) and then immediately freed.\n\nInstead, let's keep these instances on an 'accepted_remotes' list.\n\nThis way the post-loop phase can iterate a single list to build the\nprotocol reply, apply advertised filters, and mark remotes as\naccepted, rather than iterating three separate structures.\n\nThis refactoring also prepares for a future commit that will add a\n'local_name' member to 'struct promisor_info'. Since struct instances\nstay alive, downstream code will be able to simply read both names\nfrom them rather than needing yet another parallel strvec.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 42 +++++++++++++++++-------------------------\n 1 file changed, 17 insertions(+), 25 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8d80ef6040..74e65e9dd0 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -890,10 +890,10 @@ static void filter_promisor_remote(struct repository *repo,\n {\n \tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n \tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n+\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \tstruct store_info *store_info = NULL;\n \tstruct string_list_item *item;\n \tbool reload_config = false;\n-\tstruct string_list accepted_filters = STRING_LIST_INIT_DUP;\n \tenum accept_promisor accept = accept_from_server(repo);\n \n \tif (accept == ACCEPT_NONE)\n@@ -922,17 +922,10 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\tif (promisor_store_advertised_fields(advertised, store_info))\n \t\t\t\treload_config = true;\n \n-\t\t\tstrvec_push(accepted, advertised->name);\n-\n-\t\t\t/* Capture advertised filters for accepted remotes */\n-\t\t\tif (advertised->filter) {\n-\t\t\t\tstruct string_list_item *i;\n-\t\t\t\ti = string_list_append(&accepted_filters, advertised->name);\n-\t\t\t\ti->util = xstrdup(advertised->filter);\n-\t\t\t}\n+\t\t\tstring_list_append(&accepted_remotes, advertised->name)->util = advertised;\n+\t\t} else {\n+\t\t\tpromisor_info_free(advertised);\n \t\t}\n-\n-\t\tpromisor_info_free(advertised);\n \t}\n \n \tpromisor_info_list_clear(&config_info);\n@@ -942,24 +935,23 @@ static void filter_promisor_remote(struct repository *repo,\n \tif (reload_config)\n \t\trepo_promisor_remote_reinit(repo);\n \n-\t/* Apply accepted remote filters to the stable repo state */\n-\tfor_each_string_list_item(item, &accepted_filters) {\n-\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, item->string);\n-\t\tif (r) {\n-\t\t\tfree(r->advertised_filter);\n-\t\t\tr->advertised_filter = item->util;\n-\t\t\titem->util = NULL;\n-\t\t}\n-\t}\n+\t/* Apply accepted remotes to the stable repo state */\n+\tfor_each_string_list_item(item, &accepted_remotes) {\n+\t\tstruct promisor_info *info = item->util;\n+\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, info->name);\n \n-\tstring_list_clear(&accepted_filters, 1);\n+\t\tstrvec_push(accepted, info->name);\n \n-\t/* Mark the remotes as accepted in the repository state */\n-\tfor (size_t i = 0; i < accepted->nr; i++) {\n-\t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, accepted->v[i]);\n-\t\tif (r)\n+\t\tif (r) {\n \t\t\tr->accepted = 1;\n+\t\t\tif (info->filter) {\n+\t\t\t\tfree(r->advertised_filter);\n+\t\t\t\tr->advertised_filter = xstrdup(info->filter);\n+\t\t\t}\n+\t\t}\n \t}\n+\n+\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void promisor_remote_reply(const char *info, char **accepted_out)\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541060","messageId":"20260407115243.358642-10-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 09/10] promisor-remote: remove the 'accepted' strvec","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:42Z","receivedAt":"2026-04-07T11:53:14Z","isPatch":true,"body":"In a previous commit, filter_promisor_remote() was refactored to keep\naccepted 'struct promisor_info' instances alive instead of dismantling\nthem into separate parallel data structures.\n\nLet's go one step further and replace the 'struct strvec *accepted'\nargument passed to filter_promisor_remote() with a\n'struct string_list *accepted_remotes' argument.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 74e65e9dd0..38fa050542 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -885,12 +885,11 @@ static enum accept_promisor accept_from_server(struct repository *repo)\n }\n \n static void filter_promisor_remote(struct repository *repo,\n-\t\t\t\t struct strvec *accepted,\n+\t\t\t\t struct string_list *accepted_remotes,\n \t\t\t\t const char *info)\n {\n \tstruct string_list config_info = STRING_LIST_INIT_NODUP;\n \tstruct string_list remote_info = STRING_LIST_INIT_DUP;\n-\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \tstruct store_info *store_info = NULL;\n \tstruct string_list_item *item;\n \tbool reload_config = false;\n@@ -922,7 +921,7 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\tif (promisor_store_advertised_fields(advertised, store_info))\n \t\t\t\treload_config = true;\n \n-\t\t\tstring_list_append(&accepted_remotes, advertised->name)->util = advertised;\n+\t\t\tstring_list_append(accepted_remotes, advertised->name)->util = advertised;\n \t\t} else {\n \t\t\tpromisor_info_free(advertised);\n \t\t}\n@@ -936,12 +935,10 @@ static void filter_promisor_remote(struct repository *repo,\n \t\trepo_promisor_remote_reinit(repo);\n \n \t/* Apply accepted remotes to the stable repo state */\n-\tfor_each_string_list_item(item, &accepted_remotes) {\n+\tfor_each_string_list_item(item, accepted_remotes) {\n \t\tstruct promisor_info *info = item->util;\n \t\tstruct promisor_remote *r = repo_promisor_remote_find(repo, info->name);\n \n-\t\tstrvec_push(accepted, info->name);\n-\n \t\tif (r) {\n \t\t\tr->accepted = 1;\n \t\t\tif (info->filter) {\n@@ -950,23 +947,23 @@ static void filter_promisor_remote(struct repository *repo,\n \t\t\t}\n \t\t}\n \t}\n-\n-\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void promisor_remote_reply(const char *info, char **accepted_out)\n {\n-\tstruct strvec accepted = STRVEC_INIT;\n+\tstruct string_list accepted_remotes = STRING_LIST_INIT_NODUP;\n \n-\tfilter_promisor_remote(the_repository, &accepted, info);\n+\tfilter_promisor_remote(the_repository, &accepted_remotes, info);\n \n \tif (accepted_out) {\n-\t\tif (accepted.nr) {\n+\t\tif (accepted_remotes.nr) {\n \t\t\tstruct strbuf reply = STRBUF_INIT;\n-\t\t\tfor (size_t i = 0; i < accepted.nr; i++) {\n-\t\t\t\tif (i)\n+\t\t\tstruct string_list_item *item;\n+\n+\t\t\tfor_each_string_list_item(item, &accepted_remotes) {\n+\t\t\t\tif (reply.len)\n \t\t\t\t\tstrbuf_addch(&reply, ';');\n-\t\t\t\tstrbuf_addstr_urlencode(&reply, accepted.v[i], allow_unsanitized);\n+\t\t\t\tstrbuf_addstr_urlencode(&reply, item->string, allow_unsanitized);\n \t\t\t}\n \t\t\t*accepted_out = strbuf_detach(&reply, NULL);\n \t\t} else {\n@@ -974,7 +971,7 @@ void promisor_remote_reply(const char *info, char **accepted_out)\n \t\t}\n \t}\n \n-\tstrvec_clear(&accepted);\n+\tpromisor_info_list_clear(&accepted_remotes);\n }\n \n void mark_promisor_remotes_as_accepted(struct repository *r, const char *remotes)\n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541061","messageId":"20260407115243.358642-11-christian.couder@gmail.com","threadId":"65413","inReplyTo":"20260407115243.358642-1-christian.couder@gmail.com","subject":"[PATCH v2 10/10] t5710: use proper file:// URIs for absolute paths","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:52:43Z","receivedAt":"2026-04-07T11:53:15Z","isPatch":true,"body":"In t5710, we frequently construct local file URIs using `file://$(pwd)`.\nOn Unix-like systems, $(pwd) returns an absolute path starting with a\nslash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an\nempty host (`file:///tmp/repo`).\n\nHowever, on Windows, $(pwd) returns a path starting with a drive\nletter (e.g., `D:/a/repo`). This results in a 2-slash URI\n(`file://D:/a/repo`). Standard URI parsers misinterpret this format,\ntreating `D:` as the host rather than part of the absolute path.\n\nThis is to be expected because RFC 8089 says that the `//` prefix with\nan empty local host must be followed by an absolute path starting with\na slash.\n\nWhile this hasn't broken the existing tests (because the old\n`promisor.acceptFromServer` logic relies entirely on strict `strcmp()`\nwithout normalizing the URLs), it will break future commits that pass\nthese URLs through `url_normalize()` or similar functions.\n\nTo future-proof the tests and ensure cross-platform URI compliance,\nlet's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly\nguarantees a leading slash for the path component, ensuring valid\n3-slash `file:///` URIs on all operating systems.\n\nWhile at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to\nhandle some common special characters in directory paths.\n\nTo be extra safe, let's skip all the tests if there are uncommon\nspecial characters in the directory path.\n\nThen let's replace all instances of `file://$(pwd)` with\n$TRASH_DIRECTORY_URL across the test script, and let's simplify the\n`sendFields` and `checkFields` tests to use\n$ENCODED_TRASH_DIRECTORY_URL directly.\n\nSigned-off-by: Christian Couder \n---\n t/t5710-promisor-remote-capability.sh | 79 +++++++++++++++++----------\n 1 file changed, 51 insertions(+), 28 deletions(-)\n\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex bf0eed9f10..b404ad9f0a 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -76,6 +76,31 @@ copy_to_lop () {\n \tcp \"$path\" \"$path2\"\n }\n \n+# On Windows, `pwd` returns a path like 'D:/foo/bar'. Prepend '/' to turn\n+# it into '/D:/foo/bar', which is what git expects in file:// URLs on Windows.\n+# On Unix, the path already starts with '/', so this is a no-op.\n+pwd_path=$(pwd)\n+case \"$pwd_path\" in\n+[a-zA-Z]:*) pwd_path=\"/$pwd_path\" ;;\n+esac\n+\n+# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),\n+# and those percent-encoded below (% space = , ;)\n+rest=$(printf \"%s\" \"$pwd_path\" | tr -d 'a-zA-Z0-9_.~/:% =,;-')\n+if test -n \"$rest\"\n+then\n+\tskip_all=\"PWD contains unsupported special characters\"\n+\ttest_done\n+fi\n+\n+TRASH_DIRECTORY_URL=\"file://$pwd_path\"\n+\n+encoded_path=$(printf \"%s\" \"$pwd_path\" |\n+\t sed -e 's/%/%25/g' -e 's/ /%20/g' -e 's/=/%3D/g' \\\n+\t\t -e 's/;/%3B/g' -e 's/,/%2C/g')\n+\n+ENCODED_TRASH_DIRECTORY_URL=\"file://$encoded_path\"\n+\n test_expect_success \"setup for testing promisor remote advertisement\" '\n \t# Create another bare repo called \"lop\" (for Large Object Promisor)\n \tgit init --bare lop &&\n@@ -88,7 +113,7 @@ test_expect_success \"setup for testing promisor remote advertisement\" '\n \tinitialize_server 1 \"$oid\" &&\n \n \t# Configure lop as promisor remote for server\n-\tgit -C server remote add lop \"file://$(pwd)/lop\" &&\n+\tgit -C server remote add lop \"$TRASH_DIRECTORY_URL/lop\" &&\n \tgit -C server config remote.lop.promisor true &&\n \n \tgit -C lop config uploadpack.allowFilter true &&\n@@ -104,7 +129,7 @@ test_expect_success \"clone with promisor.advertise set to 'true'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -119,7 +144,7 @@ test_expect_success \"clone with promisor.advertise set to 'false'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -137,7 +162,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'None'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=None \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -156,8 +181,8 @@ test_expect_success \"init + fetch with promisor.advertise set to 'true'\" '\n \tgit -C client init &&\n \tgit -C client config remote.lop.promisor true &&\n \tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n-\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n-\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n+\tgit -C client config remote.lop.url \"$TRASH_DIRECTORY_URL/lop\" &&\n+\tgit -C client config remote.server.url \"$TRASH_DIRECTORY_URL/server\" &&\n \tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n \tgit -C client config promisor.acceptfromserver All &&\n \tGIT_NO_LAZY_FETCH=0 git -C client fetch --filter=\"blob:limit=5k\" server &&\n@@ -177,10 +202,10 @@ test_expect_success \"clone with two promisors but only one advertised\" '\n \tGIT_TRACE=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.unused_lop.promisor=true \\\n \t\t-c remote.unused_lop.fetch=\"+refs/heads/*:refs/remotes/unused_lop/*\" \\\n-\t\t-c remote.unused_lop.url=\"file://$(pwd)/unused_lop\" \\\n+\t\t-c remote.unused_lop.url=\"$TRASH_DIRECTORY_URL/unused_lop\" \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -210,11 +235,11 @@ test_expect_success \"init + fetch two promisors but only one advertised\" '\n \tgit -C client init &&\n \tgit -C client config remote.unused_lop.promisor true &&\n \tgit -C client config remote.unused_lop.fetch \"+refs/heads/*:refs/remotes/unused_lop/*\" &&\n-\tgit -C client config remote.unused_lop.url \"file://$(pwd)/unused_lop\" &&\n+\tgit -C client config remote.unused_lop.url \"$TRASH_DIRECTORY_URL/unused_lop\" &&\n \tgit -C client config remote.lop.promisor true &&\n \tgit -C client config remote.lop.fetch \"+refs/heads/*:refs/remotes/lop/*\" &&\n-\tgit -C client config remote.lop.url \"file://$(pwd)/lop\" &&\n-\tgit -C client config remote.server.url \"file://$(pwd)/server\" &&\n+\tgit -C client config remote.lop.url \"$TRASH_DIRECTORY_URL/lop\" &&\n+\tgit -C client config remote.server.url \"$TRASH_DIRECTORY_URL/server\" &&\n \tgit -C client config remote.server.fetch \"+refs/heads/*:refs/remotes/server/*\" &&\n \tgit -C client config promisor.acceptfromserver All &&\n \n@@ -242,7 +267,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'KnownName'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -257,7 +282,7 @@ test_expect_success \"clone with 'KnownName' and different remote names\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.serverTwo.promisor=true \\\n \t\t-c remote.serverTwo.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.serverTwo.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.serverTwo.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownName \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -294,7 +319,7 @@ test_expect_success \"clone with promisor.acceptfromserver set to 'KnownUrl'\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -311,7 +336,7 @@ test_expect_success \"clone with 'KnownUrl' and different remote urls\" '\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/serverTwo\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/serverTwo\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -326,7 +351,7 @@ test_expect_success \"clone with 'KnownUrl' and url not configured on the server\"\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n \n-\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$TRASH_DIRECTORY_URL/lop\\\"\" &&\n \tgit -C server config unset remote.lop.url &&\n \n \t# Clone from server to create a client\n@@ -335,7 +360,7 @@ test_expect_success \"clone with 'KnownUrl' and url not configured on the server\"\n \t# missing, so the remote name will be used instead which will fail.\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -347,7 +372,7 @@ test_expect_success \"clone with 'KnownUrl' and empty url, so not advertised\" '\n \tgit -C server config promisor.advertise true &&\n \ttest_when_finished \"rm -rf client\" &&\n \n-\ttest_when_finished \"git -C server config set remote.lop.url \\\"file://$(pwd)/lop\\\"\" &&\n+\ttest_when_finished \"git -C server config set remote.lop.url \\\"$TRASH_DIRECTORY_URL/lop\\\"\" &&\n \tgit -C server config set remote.lop.url \"\" &&\n \n \t# Clone from server to create a client\n@@ -356,7 +381,7 @@ test_expect_success \"clone with 'KnownUrl' and empty url, so not advertised\" '\n \t# so the remote name will be used instead which will fail.\n \ttest_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=KnownUrl \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n@@ -380,13 +405,12 @@ test_expect_success \"clone with promisor.sendFields\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n \t# Check that fields are properly transmitted\n-\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n-\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n+\tPR1=\"name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop;otherLop\" trace &&\n@@ -411,15 +435,14 @@ test_expect_success \"clone with promisor.checkFields\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t-c promisor.checkFields=partialcloneFilter \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n \t# Check that fields are properly transmitted\n-\tENCODED_URL=$(echo \"file://$(pwd)/lop\" | sed -e \"s/ /%20/g\") &&\n-\tPR1=\"name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none\" &&\n+\tPR1=\"name=lop,url=$ENCODED_TRASH_DIRECTORY_URL/lop,partialCloneFilter=blob:none\" &&\n \tPR2=\"name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar\" &&\n \ttest_grep \"clone< promisor-remote=$PR1;$PR2\" trace &&\n \ttest_grep \"clone> promisor-remote=lop\" trace &&\n@@ -449,7 +472,7 @@ test_expect_success \"clone with promisor.storeFields=partialCloneFilter\" '\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c remote.lop.token=\"fooYYY\" \\\n \t\t-c remote.lop.partialCloneFilter=\"blob:none\" \\\n \t\t-c promisor.acceptfromserver=All \\\n@@ -501,7 +524,7 @@ test_expect_success \"clone and fetch with --filter=auto\" '\n \n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" GIT_NO_LAZY_FETCH=0 git clone \\\n \t\t-c remote.lop.promisor=true \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=auto server client 2>err &&\n \n@@ -558,7 +581,7 @@ test_expect_success \"clone with promisor.advertise set to 'true' but don't delet\n \t# Clone from server to create a client\n \tGIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \\\n \t\t-c remote.lop.fetch=\"+refs/heads/*:refs/remotes/lop/*\" \\\n-\t\t-c remote.lop.url=\"file://$(pwd)/lop\" \\\n+\t\t-c remote.lop.url=\"$TRASH_DIRECTORY_URL/lop\" \\\n \t\t-c promisor.acceptfromserver=All \\\n \t\t--no-local --filter=\"blob:limit=5k\" server client &&\n \n-- \n2.54.0.rc0.114.g05d466edb8\n\n"},{"id":"541062","messageId":"CAP8UFD22qtZk-WmkCnRN4Ws-pTe_9DYFeFC7xxj8e6hBk=yJ2A@mail.gmail.com","threadId":"65413","inReplyTo":"xmqq5x69qkcg.fsf@gitster.g","subject":"Re: [PATCH 00/10] Prepare for advertised remotes auto-configure via URL allowlist","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T11:57:33Z","receivedAt":"2026-04-07T11:57:45Z","isPatch":true,"body":"On Thu, Apr 2, 2026 at 8:37 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n> > Range-diff\n> > ==========\n> >\n> > Sorry, no range-diff as I don't think it would be quite useful because\n> > the number and order of commits has changed a lot.\n>\n> When comparing against 16-patch series from the previous round, 6 of\n> the old patches will have no corresponding patch in this round,\n> which is expected. But for the remaining 10 commits, the command\n> seems to do a decent job matching up the corresponding patches from\n> the previous round.\n\nYeah, the range-diff is actually better than I expected.\n\n> It is especially pleasing to see that the first\n> one from each round are matched up and showing that its tests are\n> moderately extended.\n\nYeah, I followed Patrick's previous suggestion about adding tests.\nMaybe I should have mentioned it somewhere in the cover letter.\n"},{"id":"541063","messageId":"CAP8UFD0pa9OOHwBtiK_+xE5jq6Qq3YPi=E_kNLpQt==soFXmGw@mail.gmail.com","threadId":"65413","inReplyTo":"CAP8UFD3CMwTjC36Grhb6_6q0SBWtTwBX4_kM5sf+peTgd7P3dA@mail.gmail.com","subject":"Re: [PATCH 10/10] t5710: use proper file:// URIs for absolute paths","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T12:02:01Z","receivedAt":"2026-04-07T12:02:15Z","isPatch":true,"body":"On Fri, Apr 3, 2026 at 10:16 AM Christian Couder\n wrote:\n\n> # Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),\n> and those percent-encoded below (% space = ; ,)\n> case \"$pwd_path\" in\n> *[!a-zA-Z0-9_.~/:%\\ =,;-]*)\n> skip_all=\"PWD contains unsupported special characters\"\n> test_done\n> ;;\n> esac\n\nIt turned out that dash and perhaps other shells don't support a space\ncharacter in a case bracket expressions, so I used the following\ninstead:\n\n# Allowed characters: alphanumeric, standard path/URI (_ . ~ / : -),\n# and those percent-encoded below (% space = , ;)\nrest=$(printf \"%s\" \"$pwd_path\" | tr -d 'a-zA-Z0-9_.~/:% =,;-')\nif test -n \"$rest\"\nthen\n skip_all=\"PWD contains unsupported special characters\"\n test_done\nfi\n"},{"id":"541064","messageId":"CAP8UFD1iT12ap7_A7Hq1KVPia_mPwqXN7W8Q0atMo0hz3qn8FA@mail.gmail.com","threadId":"65413","inReplyTo":"ac4evWK9k69LIV91@pks.im","subject":"Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-07T12:05:53Z","receivedAt":"2026-04-07T12:06:06Z","isPatch":true,"body":"On Thu, Apr 2, 2026 at 9:46 AM Patrick Steinhardt wrote:\n>\n> On Thu, Apr 02, 2026 at 09:06:04AM +0200, Christian Couder wrote:\n\n> > +test_expect_success \"init + fetch two promisors but only one advertised\" '\n> > + git -C server config promisor.advertise true &&\n> > + test_when_finished \"rm -rf client unused_lop\" &&\n> > +\n> > + # Create a promisor that will be configured but not be used\n> > + git init --bare unused_lop &&\n> > +\n> > + mkdir client &&\n> > + git -C client init &&\n>\n> Tiniest nit, not worth rerolling over: this could just be `git init client`.\n\nI copied this from another test, and I didn't think it was worth it to\nadd a preparatory patch just to fix this in the other test, so I left\nit like this.\n\nIt could be a microproject idea to clean things like this in all the\ntest scripts.\n\nThanks.\n"},{"id":"541069","messageId":"xmqq8qay7rm7.fsf@gitster.g","threadId":"65413","inReplyTo":"CAP8UFD1iT12ap7_A7Hq1KVPia_mPwqXN7W8Q0atMo0hz3qn8FA@mail.gmail.com","subject":"Re: [PATCH 01/10] promisor-remote: try accepted remotes before others in get_direct()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-07T14:49:20Z","receivedAt":"2026-04-07T14:49:23Z","isPatch":true,"body":"Christian Couder writes:\n\n> It could be a microproject idea to clean things like this in all the\n> test scripts.\n\nLet's not deliberately add extra technical debt. Preparatory\nclean-up is very good. Even without it, not adding known to be bad\ninvocation is better than \"following the pattern\".\n\nMicroproject materials are not free, as it still requires review and\napplication costs.\n\nThanks.\n"},{"id":"541079","messageId":"xmqqh5pm6761.fsf@gitster.g","threadId":"65413","inReplyTo":"20260407115243.358642-3-christian.couder@gmail.com","subject":"Re: [PATCH v2 02/10] promisor-remote: pass config entry to all_fields_match() directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-07T16:56:22Z","receivedAt":"2026-04-07T16:56:25Z","isPatch":true,"body":"Christian Couder writes:\n\n> This removes the hidden dependency on `advertised->name` inside\n> all_fields_match(), which would be wrong if in the future\n> auto-configured remotes are implemented, as the local config name may\n> differ from the server's advertised name.\n\nInteresting.\n\nThe caller, should_accept_remote(), still uses remote_name variable\nthat is an alias to advertised->name to find the config_entry to\npass down the callchain, so this step does not change the fact that\nwe are still using their name and not overriding it with our local\nname, but hopefully we will see such a change on the caller's side\nto allow us do so.\n\n> While at it, let's also add a comment before all_fields_match() and\n> match_field_against_config() to help understand how things work and\n> help avoid similar issues.\n\nVery well done.\n\n"},{"id":"541080","messageId":"xmqqbjfu65pf.fsf@gitster.g","threadId":"65413","inReplyTo":"20260407115243.358642-4-christian.couder@gmail.com","subject":"Re: [PATCH v2 03/10] promisor-remote: clarify that a remote is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-07T17:27:56Z","receivedAt":"2026-04-07T17:27:59Z","isPatch":true,"body":"Christian Couder writes:\n\n> In should_accept_remote() and parse_one_advertised_remote(), when a\n> remote is ignored, we tell users why it is ignored in a warning, but we\n> don't tell them that the remote is actually ignored.\n>\n> Let's clarify that, so users have a better idea of what's actually\n> happening.\n>\n> Signed-off-by: Christian Couder \n> ---\n> promisor-remote.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n\nI agree that it makes sense to add the final disposition to the\nmessage. It would be even better to rephrase so that it, the most\nimportant part of the message, comes first. E.g.,\n\n>\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 6c935f855a..8e062ec160 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,\n> \t\tBUG(\"Unhandled 'enum accept_promisor' value '%d'\", accept);\n> \n> \tif (!remote_url || !*remote_url) {\n> -\t\twarning(_(\"no or empty URL advertised for remote '%s'\"), remote_name);\n> +\t\twarning(_(\"no or empty URL advertised for remote '%s', \"\n> +\t\t\t \"ignoring this remote\"), remote_name);\n\nI would find it easier to understand if it is phrased this way.\n\n\t\twarning(_(\"ignoring remote '%s' that advertises no usable URL\"),\n\t\t\tremote_name);\n\ni.e., conclusion first, the reason for the conclusion next.\n\nThanks.\n"},{"id":"541081","messageId":"xmqq7bqi65aq.fsf@gitster.g","threadId":"65413","inReplyTo":"20260407115243.358642-5-christian.couder@gmail.com","subject":"Re: [PATCH v2 04/10] promisor-remote: reject empty name or URL in advertised remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-07T17:36:45Z","receivedAt":"2026-04-07T17:36:47Z","isPatch":true,"body":"Christian Couder writes:\n\n> In parse_one_advertised_remote(), we check for a NULL remote name and\n> remote URL, but not for empty ones. An empty URL seems possible as\n> url_percent_decode(\"\") doesn't return NULL.\n>\n> In promisor_config_info_list(), we ignore remotes with empty URLs, so a\n> Git server should not advertise remotes with empty URLs. It's possible\n> that a buggy or malicious server would do it though.\n>\n> So let's tighten the check in parse_one_advertised_remote() to also\n> reject empty strings at parse time.\n>\n> Signed-off-by: Christian Couder \n> ---\n> promisor-remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nPersonally, I am not enthused to see \"NULL or empty\", primarily\nbecause it is an entry into a slippery slope.\n\nSure, an empty string is implausible, but would a single letter URL\na lot more plausible? Not at all. How about three letters? Would\nit be now a bit more plausible than an empty string? Drawing the\nline there between \"\" and \"x\" does not sound sensible.\n\nDon't we have a code that _uses_ these URL strings to protect it\nagainst a malformed or otherwise unusable URL already? Can't we\nrely on that working correctly to omit this check?\n\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 8e062ec160..8322349ae8 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info\n> \n> \tstring_list_clear(&elem_list, 0);\n> \n> -\tif (!info->name || !info->url) {\n> +\tif (!info->name || !*info->name || !info->url || !*info->url) {\n> \t\twarning(_(\"server advertised a promisor remote without a name or URL: '%s', \"\n> \t\t\t \"ignoring this remote\"), remote_info);\n> \t\tpromisor_info_free(info);\n"}]}