Re: [PATCH] bundle-uri: do not die on unresolvable relative URIs

[Patrick Steinhardt <ps@pks.im>]

On Wed, Oct 28, 2026 at 11:58:18AM +0000, Aniket Gupta wrote:
> relative_url() calls die() when the relative URL has more "../"
> components than the remote URL can absorb. bundle_list_update() calls
> it on URIs taken from a bundle list and a single bad entry takes down
> the whole command.

Which command?

> Add relative_url_gently() which returns NULL and records the reason in
> a strbuf instead of dying. This will keep relative_url() as a wrapper

s/will keep/keeps/. We're in present tense.

> that dies so the submodule callers are unchanged. Use the new function
> in bundle_list_update() and report the problem as an error like the
> other malformed lines in a bundle list.

These "bundle lists" are quite abstract here. It would help the reader
if the commit message briefly mentioned where such a list even comes
from.

Even more importantly though, do we now only print the error and then
die or do we continue? And if the latter, why is it fine to continue
even if we have failed to retrieve the bundle? And is it fine to
continue for both callers of this function?

> diff --git a/bundle-uri.c b/bundle-uri.c
> index 2bb2eb99e4..1f198ef9a9 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -210,9 +210,17 @@ static int bundle_list_update(const char *key, const char *value,
>  	strbuf_release(&id);

Okay. The only other allocated variable was released already, so nothing
that we'd have to do in that case.

>  	if (!strcmp(subkey, "uri")) {
> +		struct strbuf err = STRBUF_INIT;
> +
>  		if (bundle->uri)
>  			return -1;
> -		bundle->uri = relative_url(list->baseURI, value, NULL);
> +		bundle->uri = relative_url_gently(list->baseURI, value, NULL,
> +						  &err);
> +		if (!bundle->uri) {
> +			error("%s", err.buf);
> +			strbuf_release(&err);
> +			return -1;
> +		}
>  		return 0;
>  	}

Looks reasonable. There's already other branches here that may return
errors, so any caller that wouldn't know to hanlde such an error would
already be broken right now.

> diff --git a/remote.c b/remote.c
> index 49ee2ea318..7472c426aa 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -3073,8 +3077,16 @@ char *relative_url(const char *remote_url, const char *url,
>  	 */
>  	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
> +			int chopped;
> +
>  			url += 3;
> -			colonsep |= chop_last_dir(&remoteurl, is_relative);
> +			chopped = chop_last_dir(&remoteurl, is_relative, err);
> +			if (chopped < 0) {
> +				free(remoteurl);
> +				strbuf_release(&sb);
> +				return NULL;
> +			}
> +			colonsep |= chopped;

The error handling does get a bit unwieldy in this function by now. It
would have been great to maybe add a preparatory commit where we adapt
it to have a single unified exit path that releases memory. But I won't
insist on it.

> diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
> index 70ae94c4d0..0f0ac29565 100755
> --- a/t/t5750-bundle-uri-parse.sh
> +++ b/t/t5750-bundle-uri-parse.sh
> @@ -68,17 +68,16 @@ test_expect_success 'bundle_uri_parse_line(): relative URIs and parent paths' '
>  		uri = <uri>/bundle.bdl
>  	[bundle "two"]
>  		uri = bundle.bdl
> -	[bundle "three"]
> -		uri = <uri>/../bundle.bdl

Why do we have to drop these lines here? Interestingly, we don't seem to
use this file at all in this test.

>  	EOF
>  
> -	# TODO: We would prefer if parsing a bundle list would not cause
> -	# a die() and instead would give a warning and allow the rest of
> -	# a Git command to continue. This test_must_fail is necessary for
> -	# now until the interface for relative_url() allows for reporting
> -	# an error instead of die()ing.
> +	cat >err.expect <<-\EOF &&
> +	error: cannot strip one component off url '\''.'\''

This error message doesn't really look useful to me. In the current form
the user wouldn't be able to figure out which URL specifically we failed
to strip. It's a preexisting issue, true, but do we maybe want to fix it
while at it?

Thanks!

Patrick