{"thread":{"id":"65456","subject":"[PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","startedAt":"2026-04-08T08:58:44Z","lastAt":"2026-07-08T21:13:46Z","messageCount":10,"participants":["Toon Claes","Patrick Steinhardt","Justin Tobler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"541123","messageId":"20260408-toon-bundle-uri-no-uri-v1-1-d4a0e3937eba@iotcl.com","threadId":"65456","inReplyTo":null,"subject":"[PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-04-08T08:58:28Z","receivedAt":"2026-04-08T08:58:44Z","isPatch":true,"body":"On clone, when the client sends the `bundle-uri` command, the server\nmight respond with invalid data. For example if it sends information\nabout a bundle where the 'uri' is empty, it produces the following\nerror:\n\n Cloning into 'foo'...\n error: bundle-uri: line has empty key or value\n error: error on bundle-uri response line 4: bundle.bundle-1.uri=\n error: could not retrieve server-advertised bundle-uri list\n\nThis error is bubbled up to `transport_get_remote_bundle_uri()`, which\nis called by `cmd_clone()` in builtin/clone.c. Over here, the return\nvalue of is ignored, so clone continues.\n\nDespite this, it still dies with this error:\n\n fatal: expected 'packfile'\n\nThis happens because `get_remote_bundle_uri()` exited early, leaving\nsome unprocessed packet data behind in the read buffer. This is\nmisleading to the user, because it suggests a problem with the packfile\nexchange, when in reality it's caused by a misconfigured bundle-URI on\nthe server-side.\n\nFix this by continuing to read packets when an error was encountered,\nbut without processing the remaining lines. This drains the protocol\nstream so no stale data is left behind and the caller can use it if they\nlike.\n\nWith this, clone now continues successfully if invalid bundle-URI data\nwas sent by the server. This is intentional, because since the inception\nof `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the\n'bundle-uri' command when available, 2022-12-22) the return value of\nthat function is ignored in `cmd_clone()` so the clone can continue\nwithout bundles.\n\nSigned-off-by: Toon Claes \n---\nThis patch is a leftover from [1]. In that series I've submitted two\npatches. Because that series was submitted a long time ago, I'm\nsubmitting this as a new series.\n\nThe first patch is in meantime superseded by [2], and thus is dropped\nfrom this series.\n\nThe second patch fixes a misleading \"fatal: expected 'packfile'\" error\nthat occurs when cloning over HTTP from a server with misconfigured\nbundle-URIs. It is modified to address Junio's concerns[3]:\n\n> I tend to agree. Instead of papering over a misconfiguration, it\n> would be better to let the users know, so they have a chance to\n> report and/or correct such a misconfiguration.\n\nTo reiterate, in the previous series I changed the error() to a\nwarning() and Justin and Junio both didn't like this. In this series I\ndidn't remove the error(), but instead I'm ensuring the read buffer is\nflushed before get_remote_bundle_uri() exits. This leaves a clean state\nbehind and clone can continue. (more details in the commit message).\n\nIn reply to that other series, Justin also insisted to implement a\nserver-side fix when bundles are misconfigured. I'm currently on the\nfence about this, because I don't have a good idea how to address this.\nI see a few options:\n\n - Emit a warning on server-side: Personally I don't think this is a\n good idea because this might just end up in the logs somewhere, and\n no one might ever read them and they would just make the logs\n explode.\n\n - Exit the upload-pack process: I like this even less. Bundle-URIs are\n considered to be optional by design. Breaking clone operations\n because of a misconfiguration of something optional is too drastic.\n\n - On the client-side, read the return value of\n `transport_get_remote_bundle_uri()` and exit the clone in case of\n error: This would make the user a lot more aware of the error, and\n that would encourage the user to inform the server admin to fix the\n issue. But that breaks their clone, and they cannot continue doing\n whatever they wanted to do. Their only option to continue is to\n disable the config transfer.bundleURI, but that's cumbersome.\n\nBecause bundle-URIs are optional by design, I believe the changes in\nthis series are sufficient. Also, the series [2] takes a similar\napproach: have the client gracefully continue in case of misconfigured\nbundles.\n\n[1]: <20250912-b4-toon-bundle-uri-no-uri-v1-0-f4525a406df8@iotcl.com>\n[2]: \n[3]: \n\nGreets,\nToon\n---\n connect.c | 10 +++++++---\n t/t5558-clone-bundle-uri.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex a02583a102..e323455d3b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \t\t\t struct bundle_list *bundles, int stateless_rpc)\n {\n-\tint line_nr = 1;\n+\tint line_nr = 1, err = 0;\n \n \t/* Assert bundle-uri support */\n \tensure_server_supports_v2(\"bundle-uri\");\n@@ -536,10 +536,14 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \t\tconst char *line = reader->line;\n \t\tline_nr++;\n \n+\t\t/* Do not parse if an error was encountered */\n+\t\tif (err)\n+\t\t\tcontinue;\n+\n \t\tif (!bundle_uri_parse_line(bundles, line))\n \t\t\tcontinue;\n \n-\t\treturn error(_(\"error on bundle-uri response line %d: %s\"),\n+\t\terr = error(_(\"error on bundle-uri response line %d: %s\"),\n \t\t\t line_nr, line);\n \t}\n \n@@ -554,7 +558,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \tcheck_stateless_delimiter(stateless_rpc, reader,\n \t\t\t\t _(\"expected response end packet after ref listing\"));\n \n-\treturn 0;\n+\treturn err;\n }\n \n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex 7a0943bd36..514cc881b6 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -1302,6 +1302,31 @@ test_expect_success 'bundles with newline in target path are rejected' '\n \ttest_path_is_missing escape\n '\n \n+test_expect_success 'bundles advertised with missing URI' '\n+\tgit clone --no-local --mirror clone-from \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config uploadpack.advertiseBundleURIs true &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.version 1 &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.mode all &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n+\n+\tgit -c transfer.bundleURI=true clone \\\n+\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri\n+'\n+\n+test_expect_success 'bundles advertised with empty URI' '\n+\tgit clone --no-local --mirror clone-from \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config uploadpack.advertiseBundleURIs true &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.version 1 &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.mode all &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.uri \"\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n+\n+\tgit -c transfer.bundleURI=true clone \\\n+\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri\n+'\n+\n # Do not add tests here unless they use the HTTP server, as they will\n # not run unless the HTTP dependencies exist.\n \n\n---\nbase-commit: 256554692df0685b45e60778b08802b720880c50\nchange-id: 20260408-toon-bundle-uri-no-uri-24f661a498aa\n\n"},{"id":"541124","messageId":"adYoJxHqoJ7c6Hin@pks.im","threadId":"65456","inReplyTo":"20260408-toon-bundle-uri-no-uri-v1-1-d4a0e3937eba@iotcl.com","subject":"Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-08T10:04:23Z","receivedAt":"2026-04-08T10:04:30Z","isPatch":true,"body":"On Wed, Apr 08, 2026 at 10:58:28AM +0200, Toon Claes wrote:\n> This patch is a leftover from [1]. In that series I've submitted two\n> patches. Because that series was submitted a long time ago, I'm\n> submitting this as a new series.\n> \n> The first patch is in meantime superseded by [2], and thus is dropped\n> from this series.\n> \n> The second patch fixes a misleading \"fatal: expected 'packfile'\" error\n> that occurs when cloning over HTTP from a server with misconfigured\n> bundle-URIs. It is modified to address Junio's concerns[3]:\n> \n> > I tend to agree. Instead of papering over a misconfiguration, it\n> > would be better to let the users know, so they have a chance to\n> > report and/or correct such a misconfiguration.\n> \n> To reiterate, in the previous series I changed the error() to a\n> warning() and Justin and Junio both didn't like this. In this series I\n> didn't remove the error(), but instead I'm ensuring the read buffer is\n> flushed before get_remote_bundle_uri() exits. This leaves a clean state\n> behind and clone can continue. (more details in the commit message).\n> \n> In reply to that other series, Justin also insisted to implement a\n> server-side fix when bundles are misconfigured. I'm currently on the\n> fence about this, because I don't have a good idea how to address this.\n> I see a few options:\n> \n> - Emit a warning on server-side: Personally I don't think this is a\n> good idea because this might just end up in the logs somewhere, and\n> no one might ever read them and they would just make the logs\n> explode.\n> \n> - Exit the upload-pack process: I like this even less. Bundle-URIs are\n> considered to be optional by design. Breaking clone operations\n> because of a misconfiguration of something optional is too drastic.\n> \n> - On the client-side, read the return value of\n> `transport_get_remote_bundle_uri()` and exit the clone in case of\n> error: This would make the user a lot more aware of the error, and\n> that would encourage the user to inform the server admin to fix the\n> issue. But that breaks their clone, and they cannot continue doing\n> whatever they wanted to do. Their only option to continue is to\n> disable the config transfer.bundleURI, but that's cumbersome.\n> \n> Because bundle-URIs are optional by design, I believe the changes in\n> this series are sufficient. Also, the series [2] takes a similar\n> approach: have the client gracefully continue in case of misconfigured\n> bundles.\n\nThis makes sense. The only thing that I'm wondering about is whether we\nnow \"paper over\" such issues in all cases. I think it's totally fine for\nus to just continue whenever bundle URIs are configured as optional, but\nnot in the case where the user explicitly asks us to use them.\n\nFor example, the user can also explicitly request bundle URIs by saying\n`git clone --bundle-uri=...`, and if the configured bundle URI is\ninvalid due to whatever reason we should bail. We don't have any other\nexplicit toggle in git-clone(1) to the best of my knowledge, but if\nthere was I would claim that any such toggle should also cause us to\ndie.\n\nI _think_ that's the case already with your patch series, but it would\nbe fine to make that destinction in the commit message and maybe add a\ntest that demonstrates that we die when configured explicitly.\n\n> diff --git a/connect.c b/connect.c\n> index a02583a102..e323455d3b 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -536,10 +536,14 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \t\tconst char *line = reader->line;\n> \t\tline_nr++;\n> \n> +\t\t/* Do not parse if an error was encountered */\n> +\t\tif (err)\n> +\t\t\tcontinue;\n\nLet's document why we don't bail out of the loop immediately.\n\n> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\n> index 7a0943bd36..514cc881b6 100755\n> --- a/t/t5558-clone-bundle-uri.sh\n> +++ b/t/t5558-clone-bundle-uri.sh\n> @@ -1302,6 +1302,31 @@ test_expect_success 'bundles with newline in target path are rejected' '\n> \ttest_path_is_missing escape\n> '\n> \n> +test_expect_success 'bundles advertised with missing URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri\n> +'\n> +\n> +test_expect_success 'bundles advertised with empty URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.uri \"\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri\n> +'\n\nShouldn't these tests also verify that we see the expected error\nmessages?\n\nThanks!\n\nPatrick\n"},{"id":"541162","messageId":"adZ6yyGsoyjm7t0Q@denethor","threadId":"65456","inReplyTo":"20260408-toon-bundle-uri-no-uri-v1-1-d4a0e3937eba@iotcl.com","subject":"Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-04-08T17:49:48Z","receivedAt":"2026-04-08T17:49:53Z","isPatch":true,"body":"On 26/04/08 10:58AM, Toon Claes wrote:\n> On clone, when the client sends the `bundle-uri` command, the server\n> might respond with invalid data. For example if it sends information\n> about a bundle where the 'uri' is empty, it produces the following\n> error:\n> \n> Cloning into 'foo'...\n> error: bundle-uri: line has empty key or value\n> error: error on bundle-uri response line 4: bundle.bundle-1.uri=\n> error: could not retrieve server-advertised bundle-uri list\n> \n> This error is bubbled up to `transport_get_remote_bundle_uri()`, which\n> is called by `cmd_clone()` in builtin/clone.c. Over here, the return\n> value of is ignored, so clone continues.\n\nOk, my first thought here is whether we _should_ be ignoring the errors\nhere. Bundle-URIs are indeed an optional feature, so if for example the\nclient were unable to retrieve the bundles advertised by the server it\ncertainly would make sense to skip over them and continue with the\nclone.\n\nBut, from my understanding the problem here is that the server is\nconfigured in an invalid manner and sending this invalid bundle-uri\n\"key=value\" pair to the client. This is all part of client-server\nnegotiation for the bundle-uri feature. Why would a server want to\nknowingly advertise the invalid bundle-uri info to the client in the\nfirst place? It seems to be that server is misbehaving by not following\nthe expected bundle-uri negotiation protocol and _should_ know better.\n\nDespite this, it may also make sense to make the client more resilient\nto a misbehaving server.\n\n> Despite this, it still dies with this error:\n> \n> fatal: expected 'packfile'\n> \n> This happens because `get_remote_bundle_uri()` exited early, leaving\n> some unprocessed packet data behind in the read buffer. This is\n> misleading to the user, because it suggests a problem with the packfile\n> exchange, when in reality it's caused by a misconfigured bundle-URI on\n> the server-side.\n\nThis is certainly confusing, we should either immediately error out with\nan error indicating the server is sending an invalid response, or\nproperly allow the clone to continue if we want to ignore it.\n\n> Fix this by continuing to read packets when an error was encountered,\n> but without processing the remaining lines. This drains the protocol\n> stream so no stale data is left behind and the caller can use it if they\n> like.\n\nOk.\n\n> With this, clone now continues successfully if invalid bundle-URI data\n> was sent by the server. This is intentional, because since the inception\n> of `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the\n> 'bundle-uri' command when available, 2022-12-22) the return value of\n> that function is ignored in `cmd_clone()` so the clone can continue\n> without bundles.\n> \n> Signed-off-by: Toon Claes \n> ---\n> This patch is a leftover from [1]. In that series I've submitted two\n> patches. Because that series was submitted a long time ago, I'm\n> submitting this as a new series.\n> \n> The first patch is in meantime superseded by [2], and thus is dropped\n> from this series.\n> \n> The second patch fixes a misleading \"fatal: expected 'packfile'\" error\n> that occurs when cloning over HTTP from a server with misconfigured\n> bundle-URIs. It is modified to address Junio's concerns[3]:\n> \n> > I tend to agree. Instead of papering over a misconfiguration, it\n> > would be better to let the users know, so they have a chance to\n> > report and/or correct such a misconfiguration.\n> \n> To reiterate, in the previous series I changed the error() to a\n> warning() and Justin and Junio both didn't like this. In this series I\n> didn't remove the error(), but instead I'm ensuring the read buffer is\n> flushed before get_remote_bundle_uri() exits. This leaves a clean state\n> behind and clone can continue. (more details in the commit message).\n> \n> In reply to that other series, Justin also insisted to implement a\n> server-side fix when bundles are misconfigured. I'm currently on the\n> fence about this, because I don't have a good idea how to address this.\n> I see a few options:\n> \n> - Emit a warning on server-side: Personally I don't think this is a\n> good idea because this might just end up in the logs somewhere, and\n> no one might ever read them and they would just make the logs\n> explode.\n> \n> - Exit the upload-pack process: I like this even less. Bundle-URIs are\n> considered to be optional by design. Breaking clone operations\n> because of a misconfiguration of something optional is too drastic.\n> \n> - On the client-side, read the return value of\n> `transport_get_remote_bundle_uri()` and exit the clone in case of\n> error: This would make the user a lot more aware of the error, and\n> that would encourage the user to inform the server admin to fix the\n> issue. But that breaks their clone, and they cannot continue doing\n> whatever they wanted to do. Their only option to continue is to\n> disable the config transfer.bundleURI, but that's cumbersome.\n\nFrom my understanding, the root of the problem is that the server is\nsending a bundle-uri \"key=value\" where the value is empty. An empty\nbundle-uri value is an invalid configuration and is causing the client\nto misbehave. Wouldn't fixing the problem on the server-side be as\nsimple as not sending the invalid bundle-uri \"key=value\" to the client\nin the first place?\n\nNaively, I would assume the easiest way to fix the issue on the\nserver-side would be the following:\n\n--- >8 ---\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 3b2e347288..96d38bb80f 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -946,7 +946,7 @@ static int config_to_packet_line(const char *key, const char *value,\n {\n struct packet_reader *writer = data;\n \n- if (starts_with(key, \"bundle.\"))\n+ if (starts_with(key, \"bundle.\") && value && *value)\n packet_write_fmt(writer->fd, \"%s=%s\", key, value);\n \n return 0;\n---- >8 ---\n\nA quick check using the tests provided in this patch seems to show them\npassing with the above. If we want, we could also have the server print\na warning on its end regarding the missing value too.\n\n> Because bundle-URIs are optional by design, I believe the changes in\n> this series are sufficient. Also, the series [2] takes a similar\n> approach: have the client gracefully continue in case of misconfigured\n> bundles.\n\nI'm still largely of the opinion that a server-side fix should be\nimplemented first. Unless we really don't care that a server may\nadvertise invalid bundle-uri info to a client, making the client ignore\nthe error doesn't address the root of the problem. I don't see a good\nreason why we would want servers to keep doing this anyways.\n\nTo be clear, I'm not against also making the client more resilient since\na \"fixed\" client may still try to talk to an older server that still\nmisbehaves though.\n\n> [1]: <20250912-b4-toon-bundle-uri-no-uri-v1-0-f4525a406df8@iotcl.com>\n> [2]: \n> [3]: \n> \n> Greets,\n> Toon\n> ---\n> connect.c | 10 +++++++---\n> t/t5558-clone-bundle-uri.sh | 25 +++++++++++++++++++++++++\n> 2 files changed, 32 insertions(+), 3 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index a02583a102..e323455d3b 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n> int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \t\t\t struct bundle_list *bundles, int stateless_rpc)\n> {\n> -\tint line_nr = 1;\n> +\tint line_nr = 1, err = 0;\n> \n> \t/* Assert bundle-uri support */\n> \tensure_server_supports_v2(\"bundle-uri\");\n> @@ -536,10 +536,14 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \t\tconst char *line = reader->line;\n> \t\tline_nr++;\n> \n> +\t\t/* Do not parse if an error was encountered */\n> +\t\tif (err)\n> +\t\t\tcontinue;\n> +\n> \t\tif (!bundle_uri_parse_line(bundles, line))\n> \t\t\tcontinue;\n> \n> -\t\treturn error(_(\"error on bundle-uri response line %d: %s\"),\n> +\t\terr = error(_(\"error on bundle-uri response line %d: %s\"),\n> \t\t\t line_nr, line);\n\nOk, with this change we continue reading input even when an error in\nencountered and thus allows the clone to continue. It looks like we will\ndo this for any parsing error not just an empty bundle-uri value. This\nmay be fine though.\n\n> \t}\n> \n> @@ -554,7 +558,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \tcheck_stateless_delimiter(stateless_rpc, reader,\n> \t\t\t\t _(\"expected response end packet after ref listing\"));\n> \n> -\treturn 0;\n> +\treturn err;\n\nEarlier it was mentioned that callers of this function ultimately ignore\nits errors. I wonder if we should tighten the error handling here and\nhave callers fail on any error. We could make it so any error caused by\nan empty bundle-uri value is handled internally here, but still returns\n0. This allow us handle the specific known server error, while providing\na more useful error message to clients otherwise. This may not really be\nworth it though it we just want to always ignore any client side errors\ndue to bundle-uri.\n\n> }\n> \n> struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\n> index 7a0943bd36..514cc881b6 100755\n> --- a/t/t5558-clone-bundle-uri.sh\n> +++ b/t/t5558-clone-bundle-uri.sh\n> @@ -1302,6 +1302,31 @@ test_expect_success 'bundles with newline in target path are rejected' '\n> \ttest_path_is_missing escape\n> '\n> \n> +test_expect_success 'bundles advertised with missing URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri\n> +'\n\nOk, from my understanding a remote repository without any bundle URI\nconfigured doesn't trigger the bug here. So this test would have also\npassed before this fix. Makes sense to add still though.\n\n> +\n> +test_expect_success 'bundles advertised with empty URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.uri \"\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri\n> +'\n\nThis test actually exercises the problematic scenario.\n\n-Justin\n"},{"id":"541328","messageId":"adiZTBH_70nrpiHe@pks.im","threadId":"65456","inReplyTo":"adZ6yyGsoyjm7t0Q@denethor","subject":"Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-10T06:31:40Z","receivedAt":"2026-04-10T06:31:52Z","isPatch":true,"body":"On Wed, Apr 08, 2026 at 12:49:48PM -0500, Justin Tobler wrote:\n> On 26/04/08 10:58AM, Toon Claes wrote:\n> > Because bundle-URIs are optional by design, I believe the changes in\n> > this series are sufficient. Also, the series [2] takes a similar\n> > approach: have the client gracefully continue in case of misconfigured\n> > bundles.\n> \n> I'm still largely of the opinion that a server-side fix should be\n> implemented first. Unless we really don't care that a server may\n> advertise invalid bundle-uri info to a client, making the client ignore\n> the error doesn't address the root of the problem. I don't see a good\n> reason why we would want servers to keep doing this anyways.\n> \n> To be clear, I'm not against also making the client more resilient since\n> a \"fixed\" client may still try to talk to an older server that still\n> misbehaves though.\n\nI think that addressing the client-side is a good first step, as we need\nto also be mindful that Git is not the only implementation used on the\nserver side. So even if we fixed Git itself to not report garbage bundle\nURIs, other servers still very much might. So ensuring that clients can\nhandle these gracefully is a good thing to do.\n\nThat being said, I also think that we should fix the server side.\nWhether that needs to be part of this patch series though is a different\nquestion. Based on the proposed patch you posted it seems to be trivial\nenough though, so maybe it's worth it to just add that in as a second\npatch.\n\nPatrick\n"},{"id":"541378","messageId":"adkOJLfxs8TNGRjr@denethor","threadId":"65456","inReplyTo":"adiZTBH_70nrpiHe@pks.im","subject":"Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-04-10T15:38:02Z","receivedAt":"2026-04-10T15:38:07Z","isPatch":true,"body":"On 26/04/10 08:31AM, Patrick Steinhardt wrote:\n> On Wed, Apr 08, 2026 at 12:49:48PM -0500, Justin Tobler wrote:\n> > On 26/04/08 10:58AM, Toon Claes wrote:\n> > > Because bundle-URIs are optional by design, I believe the changes in\n> > > this series are sufficient. Also, the series [2] takes a similar\n> > > approach: have the client gracefully continue in case of misconfigured\n> > > bundles.\n> > \n> > I'm still largely of the opinion that a server-side fix should be\n> > implemented first. Unless we really don't care that a server may\n> > advertise invalid bundle-uri info to a client, making the client ignore\n> > the error doesn't address the root of the problem. I don't see a good\n> > reason why we would want servers to keep doing this anyways.\n> > \n> > To be clear, I'm not against also making the client more resilient since\n> > a \"fixed\" client may still try to talk to an older server that still\n> > misbehaves though.\n> \n> I think that addressing the client-side is a good first step, as we need\n> to also be mindful that Git is not the only implementation used on the\n> server side. So even if we fixed Git itself to not report garbage bundle\n> URIs, other servers still very much might. So ensuring that clients can\n> handle these gracefully is a good thing to do.\n\nI think it is questionable for a Git server to be sending clients\nmalformed bundle-uri configuration. Do other Git implementations on the\nserver-side exhibit this same behavior? If so, or we reasonably think\nthey could and just want to be safe, then I agree that adjusting clients\nfirst to ignore invalid bundle-uri configuration from the server is\nreasonable.\n\nGenerally, I'm of the mindset that when a server is sending\nmalformed/garbage data that the client doesn't expect, the client should\nshould be more strict and error out. In this case though, since there\nare known affected Git versions and bundle-uri is an optional feature to\nbegin with, it probably doesn't hurt to be more permissive.\n\n> That being said, I also think that we should fix the server side.\n> Whether that needs to be part of this patch series though is a different\n> question. Based on the proposed patch you posted it seems to be trivial\n> enough though, so maybe it's worth it to just add that in as a second\n> patch.\n\nYa, my main concern was that a client-side fix would mask its root\ncause. As long as it gets addressed though it's fine. I think it would\nbe worth adding to this series, but if not I'm happy to send a follow up\npatch to fix it too.\n\n-Justin\n"},{"id":"546263","messageId":"87y0g4xtsd.fsf@emacs.iotcl.com","threadId":"65456","inReplyTo":"adkOJLfxs8TNGRjr@denethor","subject":"Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-06-24T07:49:06Z","receivedAt":"2026-06-24T07:49:16Z","isPatch":true,"body":"Hi,\n\nMy apologies for digging up this old thread, but it was suddenly brought\nback to my attention. Anyhow:\n\nJustin Tobler writes:\n\n> I think it is questionable for a Git server to be sending clients\n> malformed bundle-uri configuration.\n\nI will not argue about that.\n\n> Do other Git implementations on the server-side exhibit this same\n> behavior? If so, or we reasonably think they could and just want to be\n> safe, then I agree that adjusting clients first to ignore invalid\n> bundle-uri configuration from the server is reasonable.\n>\n> Generally, I'm of the mindset that when a server is sending\n> malformed/garbage data that the client doesn't expect, the client should\n> should be more strict and error out. In this case though, since there\n> are known affected Git versions and bundle-uri is an optional feature to\n> begin with, it probably doesn't hurt to be more permissive.\n\nYeah, that's the point I was trying to make. The use of bundle-uri is\noptional, and clone can continue without it. The code was intentionally\nwritten to continue when something goes wrong with bundle-uris. But\nbecause of some underlaying issue I was trying to fix with this patch,\nthe process does not continue.\n\n> On 26/04/10 08:31AM, Patrick Steinhardt wrote:\n\n>> That being said, I also think that we should fix the server side.\n>> Whether that needs to be part of this patch series though is a different\n>> question. Based on the proposed patch you posted it seems to be trivial\n>> enough though, so maybe it's worth it to just add that in as a second\n>> patch.\n>\n> Ya, my main concern was that a client-side fix would mask its root\n> cause. As long as it gets addressed though it's fine. I think it would\n> be worth adding to this series, but if not I'm happy to send a follow up\n> patch to fix it too.\n\nI do not fully agree. My fix doesn't make the issue go away silently,\nthe user gets a warning message. I think this would cause (at least\nsome) users to complain to the owner of the server (especially because\nbundle-URI is an opt-in feature). But I realize now, this warning isn't\nchecked in the tests, adding that would have made that more clear.\n\nI do agree though a server-side fix would be advised. But I have no idea\nhow to best address this. In a previous mail you wrote:\n\n> Naively, I would assume the easiest way to fix the issue on the\n> server-side would be the following:\n> \n> --- >8 ---\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 3b2e347288..96d38bb80f 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -946,7 +946,7 @@ static int config_to_packet_line(const char *key, const char *value,\n> {\n> struct packet_reader *writer = data;\n> \n> - if (starts_with(key, \"bundle.\"))\n> + if (starts_with(key, \"bundle.\") && value && *value)\n> packet_write_fmt(writer->fd, \"%s=%s\", key, value);\n> \n> return 0;\n> ---- >8 ---\n>\n> A quick check using the tests provided in this patch seems to show them\n> passing with the above. If we want, we could also have the server print\n> a warning on its end regarding the missing value too.\n\nI don't like this fix, because it papers over the issue, silently. But\nthen again, what is the best way to inform the server admin there's\nsomething wrong? Adding one line to the log files is easily to be\nmissed.\n\n-- \nCheers,\nToon\n\n\n"},{"id":"547493","messageId":"20260708-toon-bundle-uri-no-uri-v2-0-09a03d8db556@iotcl.com","threadId":"65456","inReplyTo":"20260408-toon-bundle-uri-no-uri-v1-1-d4a0e3937eba@iotcl.com","subject":"[PATCH v2 0/2] Fix fatal error in git-clone(1) when reading empty bundle-URI","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-07-08T15:03:33Z","receivedAt":"2026-07-08T15:03:48Z","isPatch":true,"body":"This patch is a leftover from [1]. In that series I submitted two\npatches. Because that series was submitted a long time ago, I'm\nsubmitting this as a new series.\n\nThe first patch is in meantime superseded by [2], and thus is dropped\nfrom this series.\n\nThe second patch fixes a misleading \"fatal: expected 'packfile'\" error\nthat occurs when cloning over HTTP from a server with misconfigured\nbundle-URIs. It is modified to address Junio's concerns[3]:\n\n> I tend to agree. Instead of papering over a misconfiguration, it\n> would be better to let the users know, so they have a chance to\n> report and/or correct such a misconfiguration.\n\nTo reiterate, in the previous series I changed the error() to a\nwarning() and Justin and Junio both didn't like this. In this series I\ndidn't remove the error(), but instead I'm ensuring the read buffer is\nflushed before get_remote_bundle_uri() exits. This leaves a clean state\nbehind and clone can continue. (more details in the commit message).\n\nIn reply to that other series, Justin also insisted to implement a\nserver-side fix when bundles are misconfigured, and thus he provided the\nsecond patch. This patch fixes bundles with an empty `uri`, but not with\na missing `uri`, that would require a substantial change which is\noutside the scope of this series.\n\nBecause bundle-URIs are optional by design, I believe the changes in\nthis series are sufficient. Also, the series [2] takes a similar\napproach: have the client gracefully continue in case of misconfigured\nbundles.\n\n[1]: <20250912-b4-toon-bundle-uri-no-uri-v1-0-f4525a406df8@iotcl.com>\n[2]: \n[3]: \n\nGreets,\nToon\n\n---\nChanges in v2:\n- Add second patch provided by Justin that fixes empty bundle `uri` on\n the server-side.\n- Extend inline code comments about continuing the loop in\n get_remote_bundle_uri().\n- Extend tests to check error message presented to the user.\n- Link to v1: https://patch.msgid.link/20260408-toon-bundle-uri-no-uri-v1-1-d4a0e3937eba@iotcl.com\n\n---\nJustin Tobler (1):\n bundle-uri: stop sending invalid bundle configuration\n\nToon Claes (1):\n bundle-uri: drain remaining response on invalid bundle-uri lines\n\n bundle-uri.c | 8 ++++++--\n connect.c | 15 ++++++++++++---\n t/lib-bundle-uri-protocol.sh | 23 +++++++++++++++++++++++\n t/t5558-clone-bundle-uri.sh | 29 +++++++++++++++++++++++++++++\n 4 files changed, 70 insertions(+), 5 deletions(-)\n\nRange-diff versus v1:\n\n1: b2e52ca7fc ! 1: 22a9017826 bundle-uri: drain remaining response on invalid bundle-uri lines\n @@ Commit message\n \n This error is bubbled up to `transport_get_remote_bundle_uri()`, which\n is called by `cmd_clone()` in builtin/clone.c. Over here, the return\n - value of is ignored, so clone continues.\n + value is ignored, so clone continues.\n \n Despite this, it still dies with this error:\n \n @@ connect.c: int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \t\tconst char *line = reader->line;\n \t\tline_nr++;\n \n -+\t\t/* Do not parse if an error was encountered */\n ++\t\t/*\n ++\t\t * Do not parse if an error was encountered, but\n ++\t\t * continue draining the response so no stale data\n ++\t\t * is left in the reader for subsequent protocol\n ++\t\t * exchanges.\n ++\t\t */\n +\t\tif (err)\n +\t\t\tcontinue;\n +\n @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'bundles with newline in target\n +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n +\n +\tgit -c transfer.bundleURI=true clone \\\n -+\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri\n ++\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri 2>err &&\n ++\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n ++\ttest_grep ! \"expected packfile\" err\n +'\n +\n +test_expect_success 'bundles advertised with empty URI' '\n @@ t/t5558-clone-bundle-uri.sh: test_expect_success 'bundles with newline in target\n +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n +\n +\tgit -c transfer.bundleURI=true clone \\\n -+\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri\n ++\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri 2>err &&\n ++\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n ++\ttest_grep ! \"expected packfile\" err\n +'\n +\n # Do not add tests here unless they use the HTTP server, as they will\n-: ---------- > 2: 5d31c12afb bundle-uri: stop sending invalid bundle configuration\n\n\n---\nbase-commit: f85a7e662054a7b0d9070e432508831afa214b47\nchange-id: 20260408-toon-bundle-uri-no-uri-24f661a498aa\n\n"},{"id":"547494","messageId":"20260708-toon-bundle-uri-no-uri-v2-1-09a03d8db556@iotcl.com","threadId":"65456","inReplyTo":"20260708-toon-bundle-uri-no-uri-v2-0-09a03d8db556@iotcl.com","subject":"[PATCH v2 1/2] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-07-08T15:03:34Z","receivedAt":"2026-07-08T15:04:12Z","isPatch":true,"body":"On clone, when the client sends the `bundle-uri` command, the server\nmight respond with invalid data. For example if it sends information\nabout a bundle where the 'uri' is empty, it produces the following\nerror:\n\n Cloning into 'foo'...\n error: bundle-uri: line has empty key or value\n error: error on bundle-uri response line 4: bundle.bundle-1.uri=\n error: could not retrieve server-advertised bundle-uri list\n\nThis error is bubbled up to `transport_get_remote_bundle_uri()`, which\nis called by `cmd_clone()` in builtin/clone.c. Over here, the return\nvalue is ignored, so clone continues.\n\nDespite this, it still dies with this error:\n\n fatal: expected 'packfile'\n\nThis happens because `get_remote_bundle_uri()` exited early, leaving\nsome unprocessed packet data behind in the read buffer. This is\nmisleading to the user, because it suggests a problem with the packfile\nexchange, when in reality it's caused by a misconfigured bundle-URI on\nthe server-side.\n\nFix this by continuing to read packets when an error was encountered,\nbut without processing the remaining lines. This drains the protocol\nstream so no stale data is left behind and the caller can use it if they\nlike.\n\nWith this, clone now continues successfully if invalid bundle-URI data\nwas sent by the server. This is intentional, because since the inception\nof `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the\n'bundle-uri' command when available, 2022-12-22) the return value of\nthat function is ignored in `cmd_clone()` so the clone can continue\nwithout bundles.\n\nSigned-off-by: Toon Claes \n---\n connect.c | 15 ++++++++++++---\n t/t5558-clone-bundle-uri.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 47e39d2a73..1d74c1eda2 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \t\t\t struct bundle_list *bundles, int stateless_rpc)\n {\n-\tint line_nr = 1;\n+\tint line_nr = 1, err = 0;\n \n \t/* Assert bundle-uri support */\n \tensure_server_supports_v2(\"bundle-uri\");\n@@ -536,10 +536,19 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \t\tconst char *line = reader->line;\n \t\tline_nr++;\n \n+\t\t/*\n+\t\t * Do not parse if an error was encountered, but\n+\t\t * continue draining the response so no stale data\n+\t\t * is left in the reader for subsequent protocol\n+\t\t * exchanges.\n+\t\t */\n+\t\tif (err)\n+\t\t\tcontinue;\n+\n \t\tif (!bundle_uri_parse_line(bundles, line))\n \t\t\tcontinue;\n \n-\t\treturn error(_(\"error on bundle-uri response line %d: %s\"),\n+\t\terr = error(_(\"error on bundle-uri response line %d: %s\"),\n \t\t\t line_nr, line);\n \t}\n \n@@ -554,7 +563,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n \tcheck_stateless_delimiter(stateless_rpc, reader,\n \t\t\t\t _(\"expected response end packet after ref listing\"));\n \n-\treturn 0;\n+\treturn err;\n }\n \n struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex 7a0943bd36..7cc8627e17 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -1302,6 +1302,35 @@ test_expect_success 'bundles with newline in target path are rejected' '\n \ttest_path_is_missing escape\n '\n \n+test_expect_success 'bundles advertised with missing URI' '\n+\tgit clone --no-local --mirror clone-from \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config uploadpack.advertiseBundleURIs true &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.version 1 &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.mode all &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n+\n+\tgit -c transfer.bundleURI=true clone \\\n+\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri 2>err &&\n+\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n+\ttest_grep ! \"expected packfile\" err\n+'\n+\n+test_expect_success 'bundles advertised with empty URI' '\n+\tgit clone --no-local --mirror clone-from \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config uploadpack.advertiseBundleURIs true &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.version 1 &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.mode all &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.uri \"\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n+\n+\tgit -c transfer.bundleURI=true clone \\\n+\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri 2>err &&\n+\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n+\ttest_grep ! \"expected packfile\" err\n+'\n+\n # Do not add tests here unless they use the HTTP server, as they will\n # not run unless the HTTP dependencies exist.\n \n\n-- \n2.53.0.1323.g189a785ab5\n\n"},{"id":"547495","messageId":"20260708-toon-bundle-uri-no-uri-v2-2-09a03d8db556@iotcl.com","threadId":"65456","inReplyTo":"20260708-toon-bundle-uri-no-uri-v2-0-09a03d8db556@iotcl.com","subject":"[PATCH v2 2/2] bundle-uri: stop sending invalid bundle configuration","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-07-08T15:03:35Z","receivedAt":"2026-07-08T15:04:18Z","isPatch":true,"body":"From: Justin Tobler \n\nWhen bundle-URI info is requested by the client, the server responds\nwith all \"bundle.*\" config lines as key=value packet lines. On the\nclient-side, the received bundle config packet lines are always expected\nto contain both a key and a value otherwise the client errors out during\nparsing. The server performs no validation of the read bundle\nconfiguration though which results in any misconfiguration on the\nserver-side, such as bundle configuration with an empty value, being\nblindly sent to the client.\n\nTo avoid having the server transmit invalid configuration to clients,\nonly send bundle configuration that has non-empty values.\n\nThis change makes bundle-URI information sent by the server\nsyntactically correct, but semantically it still can be invalid. For\nexample the server may end up sending `bundle.bundle-1.creationToken`,\nbut be lacking a `bundle.bundle-1.uri` for that bundle. The `uri` is\nmandatory, thus the client cannot process this bundle and will error\nwith the message:\n\n error: bundle 'bundle-1' has no uri\n\nFixing this would require a more complex solution, because bundles need\nto be validated as a whole and not line-by-line. This is considered\noutside the scope of this change.\n\nCo-authored-by: Toon Claes \nSigned-off-by: Justin Tobler \nSigned-off-by: Toon Claes \n---\n bundle-uri.c | 8 ++++++--\n t/lib-bundle-uri-protocol.sh | 23 +++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 3b2e347288..f956d3db7b 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -946,8 +946,12 @@ static int config_to_packet_line(const char *key, const char *value,\n {\n \tstruct packet_reader *writer = data;\n \n-\tif (starts_with(key, \"bundle.\"))\n-\t\tpacket_write_fmt(writer->fd, \"%s=%s\", key, value);\n+\tif (starts_with(key, \"bundle.\")) {\n+\t\tif (value && *value)\n+\t\t\tpacket_write_fmt(writer->fd, \"%s=%s\", key, value);\n+\t\telse\n+\t\t\twarning(_(\"config '%s' has no value\"), key);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/lib-bundle-uri-protocol.sh b/t/lib-bundle-uri-protocol.sh\nindex de09b6b02e..e0e19715cd 100644\n--- a/t/lib-bundle-uri-protocol.sh\n+++ b/t/lib-bundle-uri-protocol.sh\n@@ -214,3 +214,26 @@ test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol\n \t\t>actual &&\n \ttest_cmp_config_output expect actual\n '\n+\n+test_expect_success \"test bundle-uri with $BUNDLE_URI_PROTOCOL:// using protocol v2 with empty value\" '\n+\ttest_config -C \"$BUNDLE_URI_PARENT\" \\\n+\t\tbundle.bundle1.uri \"$BUNDLE_URI_BUNDLE_URI_ESCAPED-1.bdl\" &&\n+\ttest_config -C \"$BUNDLE_URI_PARENT\" \\\n+\t\tbundle.bundle2.uri \"\" &&\n+\n+\t# The empty bundle.bundle2.uri value is invalid configuration and the\n+\t# server must not advertise it to the client.\n+\tcat >expect <<-EOF &&\n+\t[bundle]\n+\t\tversion = 1\n+\t\tmode = all\n+\t[bundle \"bundle1\"]\n+\t\turi = $BUNDLE_URI_BUNDLE_URI_ESCAPED-1.bdl\n+\tEOF\n+\n+\ttest-tool bundle-uri \\\n+\t\tls-remote \\\n+\t\t\"$BUNDLE_URI_REPO_URI\" \\\n+\t\t>actual &&\n+\ttest_cmp_config_output expect actual\n+'\n\n-- \n2.53.0.1323.g189a785ab5\n\n"},{"id":"547535","messageId":"xmqqtsq9qj5k.fsf@gitster.g","threadId":"65456","inReplyTo":"20260708-toon-bundle-uri-no-uri-v2-1-09a03d8db556@iotcl.com","subject":"Re: [PATCH v2 1/2] bundle-uri: drain remaining response on invalid bundle-uri lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T21:13:43Z","receivedAt":"2026-07-08T21:13:46Z","isPatch":true,"body":"Toon Claes writes:\n\n> With this, clone now continues successfully if invalid bundle-URI data\n> was sent by the server. This is intentional, because since the inception\n> of `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the\n> 'bundle-uri' command when available, 2022-12-22) the return value of\n> that function is ignored in `cmd_clone()` so the clone can continue\n> without bundles.\n\nI am on the fence.\n\nAlternatively, we could terminate the connection immediately, given\nthat we are clearly dealing with a broken server.\n\nIt is one thing to successfully parse the server's response (e.g.,\n'fetch the bundle from this address') but fail to follow its\ndirection because, for example, the resource is unreachable. Since\nbundles are optional, ignoring the failure and continuing makes\ncomplete sense.\n\nBut it feels different when we can't even parse what the server is\nsaying.\n\nWhile a malformed bundle-URI payload is benign enough to ignore\ntoday, future protocol extensions might introduce mandatory\ndata. Eventually, we will need a robust way to tell ignorable and\nfatal errors apart so we can react appropriately. That\nclassification can wait for a future topic, however.\n\nThe patch looks good and matches what you designed well.\n\nThanks.\n\n> Signed-off-by: Toon Claes \n> ---\n> connect.c | 15 ++++++++++++---\n> t/t5558-clone-bundle-uri.sh | 29 +++++++++++++++++++++++++++++\n> 2 files changed, 41 insertions(+), 3 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index 47e39d2a73..1d74c1eda2 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n> int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \t\t\t struct bundle_list *bundles, int stateless_rpc)\n> {\n> -\tint line_nr = 1;\n> +\tint line_nr = 1, err = 0;\n> \n> \t/* Assert bundle-uri support */\n> \tensure_server_supports_v2(\"bundle-uri\");\n> @@ -536,10 +536,19 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \t\tconst char *line = reader->line;\n> \t\tline_nr++;\n> \n> +\t\t/*\n> +\t\t * Do not parse if an error was encountered, but\n> +\t\t * continue draining the response so no stale data\n> +\t\t * is left in the reader for subsequent protocol\n> +\t\t * exchanges.\n> +\t\t */\n> +\t\tif (err)\n> +\t\t\tcontinue;\n> +\n> \t\tif (!bundle_uri_parse_line(bundles, line))\n> \t\t\tcontinue;\n> \n> -\t\treturn error(_(\"error on bundle-uri response line %d: %s\"),\n> +\t\terr = error(_(\"error on bundle-uri response line %d: %s\"),\n> \t\t\t line_nr, line);\n> \t}\n> \n> @@ -554,7 +563,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n> \tcheck_stateless_delimiter(stateless_rpc, reader,\n> \t\t\t\t _(\"expected response end packet after ref listing\"));\n> \n> -\treturn 0;\n> +\treturn err;\n> }\n> \n> struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\n> index 7a0943bd36..7cc8627e17 100755\n> --- a/t/t5558-clone-bundle-uri.sh\n> +++ b/t/t5558-clone-bundle-uri.sh\n> @@ -1302,6 +1302,35 @@ test_expect_success 'bundles with newline in target path are rejected' '\n> \ttest_path_is_missing escape\n> '\n> \n> +test_expect_success 'bundles advertised with missing URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/no-uri.git\" target-no-uri 2>err &&\n> +\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n> +\ttest_grep ! \"expected packfile\" err\n> +'\n> +\n> +test_expect_success 'bundles advertised with empty URI' '\n> +\tgit clone --no-local --mirror clone-from \\\n> +\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config uploadpack.advertiseBundleURIs true &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.version 1 &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.mode all &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.uri \"\" &&\n> +\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git\" config bundle.bundle-1.creationToken 1 &&\n> +\n> +\tgit -c transfer.bundleURI=true clone \\\n> +\t\t\"$HTTPD_URL/smart/empty-uri.git\" target-empty-uri 2>err &&\n> +\ttest_grep \"bundle ${SQ}bundle-1${SQ} has no uri\" err &&\n> +\ttest_grep ! \"expected packfile\" err\n> +'\n> +\n> # Do not add tests here unless they use the HTTP server, as they will\n> # not run unless the HTTP dependencies exist.\n"}]}