* Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines
2026-04-08 8:58 [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines Toon Claes
@ 2026-04-08 10:04 ` Patrick Steinhardt
2026-04-08 17:49 ` Justin Tobler
1 sibling, 0 replies; 5+ messages in thread
From: Patrick Steinhardt @ 2026-04-08 10:04 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Justin Tobler
On Wed, Apr 08, 2026 at 10:58:28AM +0200, Toon Claes wrote:
> This patch is a leftover from [1]. In that series I've submitted two
> patches. Because that series was submitted a long time ago, I'm
> submitting this as a new series.
>
> The first patch is in meantime superseded by [2], and thus is dropped
> from this series.
>
> The second patch fixes a misleading "fatal: expected 'packfile'" error
> that occurs when cloning over HTTP from a server with misconfigured
> bundle-URIs. It is modified to address Junio's concerns[3]:
>
> > I tend to agree. Instead of papering over a misconfiguration, it
> > would be better to let the users know, so they have a chance to
> > report and/or correct such a misconfiguration.
>
> To reiterate, in the previous series I changed the error() to a
> warning() and Justin and Junio both didn't like this. In this series I
> didn't remove the error(), but instead I'm ensuring the read buffer is
> flushed before get_remote_bundle_uri() exits. This leaves a clean state
> behind and clone can continue. (more details in the commit message).
>
> In reply to that other series, Justin also insisted to implement a
> server-side fix when bundles are misconfigured. I'm currently on the
> fence about this, because I don't have a good idea how to address this.
> I see a few options:
>
> - Emit a warning on server-side: Personally I don't think this is a
> good idea because this might just end up in the logs somewhere, and
> no one might ever read them and they would just make the logs
> explode.
>
> - Exit the upload-pack process: I like this even less. Bundle-URIs are
> considered to be optional by design. Breaking clone operations
> because of a misconfiguration of something optional is too drastic.
>
> - On the client-side, read the return value of
> `transport_get_remote_bundle_uri()` and exit the clone in case of
> error: This would make the user a lot more aware of the error, and
> that would encourage the user to inform the server admin to fix the
> issue. But that breaks their clone, and they cannot continue doing
> whatever they wanted to do. Their only option to continue is to
> disable the config transfer.bundleURI, but that's cumbersome.
>
> Because bundle-URIs are optional by design, I believe the changes in
> this series are sufficient. Also, the series [2] takes a similar
> approach: have the client gracefully continue in case of misconfigured
> bundles.
This makes sense. The only thing that I'm wondering about is whether we
now "paper over" such issues in all cases. I think it's totally fine for
us to just continue whenever bundle URIs are configured as optional, but
not in the case where the user explicitly asks us to use them.
For example, the user can also explicitly request bundle URIs by saying
`git clone --bundle-uri=...`, and if the configured bundle URI is
invalid due to whatever reason we should bail. We don't have any other
explicit toggle in git-clone(1) to the best of my knowledge, but if
there was I would claim that any such toggle should also cause us to
die.
I _think_ that's the case already with your patch series, but it would
be fine to make that destinction in the commit message and maybe add a
test that demonstrates that we die when configured explicitly.
> diff --git a/connect.c b/connect.c
> index a02583a102..e323455d3b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -536,10 +536,14 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> const char *line = reader->line;
> line_nr++;
>
> + /* Do not parse if an error was encountered */
> + if (err)
> + continue;
Let's document why we don't bail out of the loop immediately.
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 7a0943bd36..514cc881b6 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -1302,6 +1302,31 @@ test_expect_success 'bundles with newline in target path are rejected' '
> test_path_is_missing escape
> '
>
> +test_expect_success 'bundles advertised with missing URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/no-uri.git" target-no-uri
> +'
> +
> +test_expect_success 'bundles advertised with empty URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.uri "" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/empty-uri.git" target-empty-uri
> +'
Shouldn't these tests also verify that we see the expected error
messages?
Thanks!
Patrick
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines
2026-04-08 8:58 [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines Toon Claes
2026-04-08 10:04 ` Patrick Steinhardt
@ 2026-04-08 17:49 ` Justin Tobler
2026-04-10 6:31 ` Patrick Steinhardt
1 sibling, 1 reply; 5+ messages in thread
From: Justin Tobler @ 2026-04-08 17:49 UTC (permalink / raw)
To: Toon Claes; +Cc: git
On 26/04/08 10:58AM, Toon Claes wrote:
> On clone, when the client sends the `bundle-uri` command, the server
> might respond with invalid data. For example if it sends information
> about a bundle where the 'uri' is empty, it produces the following
> error:
>
> Cloning into 'foo'...
> error: bundle-uri: line has empty key or value
> error: error on bundle-uri response line 4: bundle.bundle-1.uri=
> error: could not retrieve server-advertised bundle-uri list
>
> This error is bubbled up to `transport_get_remote_bundle_uri()`, which
> is called by `cmd_clone()` in builtin/clone.c. Over here, the return
> value of is ignored, so clone continues.
Ok, my first thought here is whether we _should_ be ignoring the errors
here. Bundle-URIs are indeed an optional feature, so if for example the
client were unable to retrieve the bundles advertised by the server it
certainly would make sense to skip over them and continue with the
clone.
But, from my understanding the problem here is that the server is
configured in an invalid manner and sending this invalid bundle-uri
"key=value" pair to the client. This is all part of client-server
negotiation for the bundle-uri feature. Why would a server want to
knowingly advertise the invalid bundle-uri info to the client in the
first place? It seems to be that server is misbehaving by not following
the expected bundle-uri negotiation protocol and _should_ know better.
Despite this, it may also make sense to make the client more resilient
to a misbehaving server.
> Despite this, it still dies with this error:
>
> fatal: expected 'packfile'
>
> This happens because `get_remote_bundle_uri()` exited early, leaving
> some unprocessed packet data behind in the read buffer. This is
> misleading to the user, because it suggests a problem with the packfile
> exchange, when in reality it's caused by a misconfigured bundle-URI on
> the server-side.
This is certainly confusing, we should either immediately error out with
an error indicating the server is sending an invalid response, or
properly allow the clone to continue if we want to ignore it.
> Fix this by continuing to read packets when an error was encountered,
> but without processing the remaining lines. This drains the protocol
> stream so no stale data is left behind and the caller can use it if they
> like.
Ok.
> With this, clone now continues successfully if invalid bundle-URI data
> was sent by the server. This is intentional, because since the inception
> of `transport_get_remote_bundle_uri()` in 0cfde740f0 (clone: request the
> 'bundle-uri' command when available, 2022-12-22) the return value of
> that function is ignored in `cmd_clone()` so the clone can continue
> without bundles.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> This patch is a leftover from [1]. In that series I've submitted two
> patches. Because that series was submitted a long time ago, I'm
> submitting this as a new series.
>
> The first patch is in meantime superseded by [2], and thus is dropped
> from this series.
>
> The second patch fixes a misleading "fatal: expected 'packfile'" error
> that occurs when cloning over HTTP from a server with misconfigured
> bundle-URIs. It is modified to address Junio's concerns[3]:
>
> > I tend to agree. Instead of papering over a misconfiguration, it
> > would be better to let the users know, so they have a chance to
> > report and/or correct such a misconfiguration.
>
> To reiterate, in the previous series I changed the error() to a
> warning() and Justin and Junio both didn't like this. In this series I
> didn't remove the error(), but instead I'm ensuring the read buffer is
> flushed before get_remote_bundle_uri() exits. This leaves a clean state
> behind and clone can continue. (more details in the commit message).
>
> In reply to that other series, Justin also insisted to implement a
> server-side fix when bundles are misconfigured. I'm currently on the
> fence about this, because I don't have a good idea how to address this.
> I see a few options:
>
> - Emit a warning on server-side: Personally I don't think this is a
> good idea because this might just end up in the logs somewhere, and
> no one might ever read them and they would just make the logs
> explode.
>
> - Exit the upload-pack process: I like this even less. Bundle-URIs are
> considered to be optional by design. Breaking clone operations
> because of a misconfiguration of something optional is too drastic.
>
> - On the client-side, read the return value of
> `transport_get_remote_bundle_uri()` and exit the clone in case of
> error: This would make the user a lot more aware of the error, and
> that would encourage the user to inform the server admin to fix the
> issue. But that breaks their clone, and they cannot continue doing
> whatever they wanted to do. Their only option to continue is to
> disable the config transfer.bundleURI, but that's cumbersome.
From my understanding, the root of the problem is that the server is
sending a bundle-uri "key=value" where the value is empty. An empty
bundle-uri value is an invalid configuration and is causing the client
to misbehave. Wouldn't fixing the problem on the server-side be as
simple as not sending the invalid bundle-uri "key=value" to the client
in the first place?
Naively, I would assume the easiest way to fix the issue on the
server-side would be the following:
--- >8 ---
diff --git a/bundle-uri.c b/bundle-uri.c
index 3b2e347288..96d38bb80f 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -946,7 +946,7 @@ static int config_to_packet_line(const char *key, const char *value,
{
struct packet_reader *writer = data;
- if (starts_with(key, "bundle."))
+ if (starts_with(key, "bundle.") && value && *value)
packet_write_fmt(writer->fd, "%s=%s", key, value);
return 0;
---- >8 ---
A quick check using the tests provided in this patch seems to show them
passing with the above. If we want, we could also have the server print
a warning on its end regarding the missing value too.
> Because bundle-URIs are optional by design, I believe the changes in
> this series are sufficient. Also, the series [2] takes a similar
> approach: have the client gracefully continue in case of misconfigured
> bundles.
I'm still largely of the opinion that a server-side fix should be
implemented first. Unless we really don't care that a server may
advertise invalid bundle-uri info to a client, making the client ignore
the error doesn't address the root of the problem. I don't see a good
reason why we would want servers to keep doing this anyways.
To be clear, I'm not against also making the client more resilient since
a "fixed" client may still try to talk to an older server that still
misbehaves though.
> [1]: <20250912-b4-toon-bundle-uri-no-uri-v1-0-f4525a406df8@iotcl.com>
> [2]: <pull.2134.v2.git.git.1766160106521.gitgitgadget@gmail.com>
> [3]: <xmqqbjnfmvwo.fsf@gitster.g>
>
> Greets,
> Toon
> ---
> connect.c | 10 +++++++---
> t/t5558-clone-bundle-uri.sh | 25 +++++++++++++++++++++++++
> 2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index a02583a102..e323455d3b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -517,7 +517,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)
> int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> struct bundle_list *bundles, int stateless_rpc)
> {
> - int line_nr = 1;
> + int line_nr = 1, err = 0;
>
> /* Assert bundle-uri support */
> ensure_server_supports_v2("bundle-uri");
> @@ -536,10 +536,14 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> const char *line = reader->line;
> line_nr++;
>
> + /* Do not parse if an error was encountered */
> + if (err)
> + continue;
> +
> if (!bundle_uri_parse_line(bundles, line))
> continue;
>
> - return error(_("error on bundle-uri response line %d: %s"),
> + err = error(_("error on bundle-uri response line %d: %s"),
> line_nr, line);
Ok, with this change we continue reading input even when an error in
encountered and thus allows the clone to continue. It looks like we will
do this for any parsing error not just an empty bundle-uri value. This
may be fine though.
> }
>
> @@ -554,7 +558,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,
> check_stateless_delimiter(stateless_rpc, reader,
> _("expected response end packet after ref listing"));
>
> - return 0;
> + return err;
Earlier it was mentioned that callers of this function ultimately ignore
its errors. I wonder if we should tighten the error handling here and
have callers fail on any error. We could make it so any error caused by
an empty bundle-uri value is handled internally here, but still returns
0. This allow us handle the specific known server error, while providing
a more useful error message to clients otherwise. This may not really be
worth it though it we just want to always ignore any client side errors
due to bundle-uri.
> }
>
> struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 7a0943bd36..514cc881b6 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -1302,6 +1302,31 @@ test_expect_success 'bundles with newline in target path are rejected' '
> test_path_is_missing escape
> '
>
> +test_expect_success 'bundles advertised with missing URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/no-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/no-uri.git" target-no-uri
> +'
Ok, from my understanding a remote repository without any bundle URI
configured doesn't trigger the bug here. So this test would have also
passed before this fix. Makes sense to add still though.
> +
> +test_expect_success 'bundles advertised with empty URI' '
> + git clone --no-local --mirror clone-from \
> + "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config uploadpack.advertiseBundleURIs true &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.version 1 &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.mode all &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.uri "" &&
> + git -C "$HTTPD_DOCUMENT_ROOT_PATH/empty-uri.git" config bundle.bundle-1.creationToken 1 &&
> +
> + git -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/empty-uri.git" target-empty-uri
> +'
This test actually exercises the problematic scenario.
-Justin
^ permalink raw reply related [flat|nested] 5+ messages in thread