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

[Patrick Steinhardt <ps@pks.im>]

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