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

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

[Aniket Gupta]

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.

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
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.

Update the test in t5750 accordingly and drop its TODO.

Signed-off-by: Aniket Gupta <aniketgupta3001@gmail.com>
---
 bundle-uri.c                | 10 +++++++++-
 remote.c                    | 39 +++++++++++++++++++++++++++++--------
 remote.h                    |  8 ++++++++
 t/t5750-bundle-uri-parse.sh | 15 +++++++-------
 4 files changed, 55 insertions(+), 17 deletions(-)

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);
 
 	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;
 	}
 
diff --git a/remote.c b/remote.c
index 49ee2ea318..7472c426aa 100644
--- a/remote.c
+++ b/remote.c
@@ -3005,9 +3005,11 @@ void remote_state_clear(struct remote_state *remote_state)
 }
 
 /*
- * Returns 1 if it was the last chop before ':'.
+ * Returns 1 if it was the last chop before ':', or -1 (and records the
+ * reason in `err`) if there is nothing left to chop.
  */
-static int chop_last_dir(char **remoteurl, int is_relative)
+static int chop_last_dir(char **remoteurl, int is_relative,
+			 struct strbuf *err)
 {
 	char *rfind = find_last_dir_sep(*remoteurl);
 	if (rfind) {
@@ -3021,17 +3023,19 @@ static int chop_last_dir(char **remoteurl, int is_relative)
 		return 1;
 	}
 
-	if (is_relative || !strcmp(".", *remoteurl))
-		die(_("cannot strip one component off url '%s'"),
-			*remoteurl);
+	if (is_relative || !strcmp(".", *remoteurl)) {
+		strbuf_addf(err, _("cannot strip one component off url '%s'"),
+			    *remoteurl);
+		return -1;
+	}
 
 	free(*remoteurl);
 	*remoteurl = xstrdup(".");
 	return 0;
 }
 
-char *relative_url(const char *remote_url, const char *url,
-		   const char *up_path)
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err)
 {
 	int is_relative = 0;
 	int colonsep = 0;
@@ -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;
 		} else if (starts_with_dot_slash_native(url))
 			url += 2;
 		else
@@ -3102,6 +3114,17 @@ char *relative_url(const char *remote_url, const char *url,
 	return strbuf_detach(&sb, NULL);
 }
 
+char *relative_url(const char *remote_url, const char *url,
+		   const char *up_path)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *out = relative_url_gently(remote_url, url, up_path, &err);
+
+	if (!out)
+		die("%s", err.buf);
+	return out;
+}
+
 int valid_remote_name(const char *name)
 {
 	int result;
diff --git a/remote.h b/remote.h
index cca02033b9..940474fa71 100644
--- a/remote.h
+++ b/remote.h
@@ -478,6 +478,14 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+/*
+ * Like relative_url(), but if `url` cannot be resolved against
+ * `remote_url`, return NULL and add the reason to `err` instead of
+ * dying. `err` must not be NULL.
+ */
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err);
+
 int valid_remote_name(const char *name);
 
 #endif
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
 	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 '\''.'\''
+	error: bad line: '\''bundle.three.uri=../../bundle.bdl'\''
+	EOF
+
 	test_must_fail test-tool bundle-uri parse-key-values in >actual 2>err &&
-	test_grep "fatal: cannot strip one component off url" err
+	test_cmp err.expect err &&
+	test_cmp_config_output expect actual
 '
 
 test_expect_success 'bundle_uri_parse_line() parsing edge cases: empty key or value' '

base-commit: 37b3475fb530c14109211faca33716a173db61ed
-- 
2.43.0

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

[Patrick Steinhardt]

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

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

[Aniket Gupta]

A bundle list, either advertised by a server or given to "git clone
--bundle-uri", may contain relative URIs. relative_url() resolves them
and calls die() when there are more "../" components than the base can
absorb, so one bad entry aborts the whole clone.

Add relative_url_gently() which returns NULL and records the reason in
a strbuf instead of dying. relative_url() keeps dying, so the submodule
callers are unchanged. bundle_list_update() reports the problem as an
error and leaves the bundle without a URI. The download code already
skips such a bundle, so the clone carries on with the others.

Name the URL in the error message, without any credentials, and update
the tests.

Signed-off-by: Aniket Gupta <aniketgupta3001@gmail.com>
---
Changes since v1:
 - the commit message says which lists are meant and why carrying on
   is fine
 - the error now names the URL that could not be resolved, with any
   credentials removed
 - add a test for that in t0060

 bundle-uri.c                | 10 +++++++++-
 remote.c                    | 37 +++++++++++++++++++++++++++++++------
 remote.h                    |  8 ++++++++
 t/t0060-path-utils.sh       |  7 +++++++
 t/t5750-bundle-uri-parse.sh | 15 +++++++--------
 5 files changed, 62 insertions(+), 15 deletions(-)

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);
 
 	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;
 	}
 
diff --git a/remote.c b/remote.c
index 49ee2ea318..36834b129c 100644
--- a/remote.c
+++ b/remote.c
@@ -3005,7 +3005,8 @@ void remote_state_clear(struct remote_state *remote_state)
 }
 
 /*
- * Returns 1 if it was the last chop before ':'.
+ * Returns 1 if it was the last chop before ':', or -1 if there is
+ * nothing left to chop.
  */
 static int chop_last_dir(char **remoteurl, int is_relative)
 {
@@ -3022,16 +3023,15 @@ static int chop_last_dir(char **remoteurl, int is_relative)
 	}
 
 	if (is_relative || !strcmp(".", *remoteurl))
-		die(_("cannot strip one component off url '%s'"),
-			*remoteurl);
+		return -1;
 
 	free(*remoteurl);
 	*remoteurl = xstrdup(".");
 	return 0;
 }
 
-char *relative_url(const char *remote_url, const char *url,
-		   const char *up_path)
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err)
 {
 	int is_relative = 0;
 	int colonsep = 0;
@@ -3073,8 +3073,22 @@ 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);
+			if (chopped < 0) {
+				char *anon = transport_anonymize_url(remote_url);
+
+				strbuf_addf(err,
+					    _("cannot strip one component off url '%s'"),
+					    anon);
+				free(anon);
+				free(remoteurl);
+				strbuf_release(&sb);
+				return NULL;
+			}
+			colonsep |= chopped;
 		} else if (starts_with_dot_slash_native(url))
 			url += 2;
 		else
@@ -3102,6 +3116,17 @@ char *relative_url(const char *remote_url, const char *url,
 	return strbuf_detach(&sb, NULL);
 }
 
+char *relative_url(const char *remote_url, const char *url,
+		   const char *up_path)
+{
+	struct strbuf err = STRBUF_INIT;
+	char *out = relative_url_gently(remote_url, url, up_path, &err);
+
+	if (!out)
+		die("%s", err.buf);
+	return out;
+}
+
 int valid_remote_name(const char *name)
 {
 	int result;
diff --git a/remote.h b/remote.h
index cca02033b9..940474fa71 100644
--- a/remote.h
+++ b/remote.h
@@ -478,6 +478,14 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+/*
+ * Like relative_url(), but if `url` cannot be resolved against
+ * `remote_url`, return NULL and add the reason to `err` instead of
+ * dying. `err` must not be NULL.
+ */
+char *relative_url_gently(const char *remote_url, const char *url,
+			  const char *up_path, struct strbuf *err);
+
 int valid_remote_name(const char *name);
 
 #endif
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 56faf5fe73..50cc37bc98 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -394,6 +394,13 @@ test_submodule_relative_url "../" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
 test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
 test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
 
+test_expect_success 'relative_url() does not show credentials when it fails' '
+	test_must_fail test-tool submodule resolve-relative-url "" \
+		"https://user:secret@host.example/a" ../../../../../../x 2>err &&
+	test_grep "cannot strip one component off url" err &&
+	test_grep ! secret err
+'
+
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c/" "../foo/sub/a/b/c"
 test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
index 70ae94c4d0..8abf8f31ae 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
 	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 '\''<uri>'\''
+	error: bad line: '\''bundle.three.uri=../../bundle.bdl'\''
+	EOF
+
 	test_must_fail test-tool bundle-uri parse-key-values in >actual 2>err &&
-	test_grep "fatal: cannot strip one component off url" err
+	test_cmp err.expect err &&
+	test_cmp_config_output expect actual
 '
 
 test_expect_success 'bundle_uri_parse_line() parsing edge cases: empty key or value' '
-- 
2.43.0

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

[Aniket Gupta]

On Tue, Nov 03, 2026 at 02:04:49PM +0100, Patrick Steinhardt wrote:
> 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?

git clone, with a bundle list from the server or from --bundle-uri.
Fetch uses the same code, but I only tried clone.

> > 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.

Fixed.

> 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.

Added.

> 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?

We continue. The bad entry ends up without a uri and the download code
already skips those. I tried both ways of getting a list with a too-deep
../. The clone prints the errors and still finishes.

> > +                     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.

I'd rather keep this one small. There is only one error path for now,
so I left the cleanup out. Happy to send it as a separate patch if you
prefer.

> > -     [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.

The old expect was never compared because the test only checked for the
die(). Now it is compared and the bad bundle is not in it.

> > +     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?

Good point. It names the url now. I strip any user:password from it
first so that it doesnt leak credentials.