[PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Toon Claes]

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.

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.

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.

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.

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.

[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);
 	}
 
@@ -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;
 }
 
 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
+'
+
+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
+'
+
 # Do not add tests here unless they use the HTTP server, as they will
 # not run unless the HTTP dependencies exist.
 

---
base-commit: 256554692df0685b45e60778b08802b720880c50
change-id: 20260408-toon-bundle-uri-no-uri-24f661a498aa

Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Patrick Steinhardt]

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

Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Justin Tobler]

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

Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Patrick Steinhardt]

On Wed, Apr 08, 2026 at 12:49:48PM -0500, Justin Tobler wrote:
> On 26/04/08 10:58AM, Toon Claes wrote:
> > 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.

I think that addressing the client-side is a good first step, as we need
to also be mindful that Git is not the only implementation used on the
server side. So even if we fixed Git itself to not report garbage bundle
URIs, other servers still very much might. So ensuring that clients can
handle these gracefully is a good thing to do.

That being said, I also think that we should fix the server side.
Whether that needs to be part of this patch series though is a different
question. Based on the proposed patch you posted it seems to be trivial
enough though, so maybe it's worth it to just add that in as a second
patch.

Patrick

Re: [PATCH] bundle-uri: drain remaining response on invalid bundle-uri lines

[Justin Tobler]

On 26/04/10 08:31AM, Patrick Steinhardt wrote:
> On Wed, Apr 08, 2026 at 12:49:48PM -0500, Justin Tobler wrote:
> > On 26/04/08 10:58AM, Toon Claes wrote:
> > > 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.
> 
> I think that addressing the client-side is a good first step, as we need
> to also be mindful that Git is not the only implementation used on the
> server side. So even if we fixed Git itself to not report garbage bundle
> URIs, other servers still very much might. So ensuring that clients can
> handle these gracefully is a good thing to do.

I think it is questionable for a Git server to be sending clients
malformed bundle-uri configuration. Do other Git implementations on the
server-side exhibit this same behavior? If so, or we reasonably think
they could and just want to be safe, then I agree that adjusting clients
first to ignore invalid bundle-uri configuration from the server is
reasonable.

Generally, I'm of the mindset that when a server is sending
malformed/garbage data that the client doesn't expect, the client should
should be more strict and error out. In this case though, since there
are known affected Git versions and bundle-uri is an optional feature to
begin with, it probably doesn't hurt to be more permissive.

> That being said, I also think that we should fix the server side.
> Whether that needs to be part of this patch series though is a different
> question. Based on the proposed patch you posted it seems to be trivial
> enough though, so maybe it's worth it to just add that in as a second
> patch.

Ya, my main concern was that a client-side fix would mask its root
cause. As long as it gets addressed though it's fine. I think it would
be worth adding to this series, but if not I'm happy to send a follow up
patch to fix it too.

-Justin