[PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming

[PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming

[cuiweixie]

if i == -1, url[i] will be UB.

Signed-off-by: cuiweixie <cuiweixie@gmail.com>
---
 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a3bc7e9380..306138c6e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -722,7 +722,7 @@ static void display_state_init(struct display_state *display_state, struct ref *
 		display_state->url = xstrdup("foreign");
 
 	display_state->url_len = strlen(display_state->url);
-	for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
+	for (i = display_state->url_len - 1; 0 <= i && display_state->url[i] == '/'; i--)
 		;
 	display_state->url_len = i + 1;
 	if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))
-- 
2.39.5 (Apple Git-154)

[PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming

[cui via GitGitGadget]

From: cuiweixie <cuiweixie@gmail.com>

if i == -1, url[i] will be UB.

Signed-off-by: cuiweixie <cuiweixie@gmail.com>
---
    fetch: fix wrong evaluation order in URL trailing-slash trimming

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2211%2Fcuiweixie%2Fbugfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2211/cuiweixie/bugfix-v1
Pull-Request: https://github.com/git/git/pull/2211

 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a3bc7e9380..306138c6e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -722,7 +722,7 @@ static void display_state_init(struct display_state *display_state, struct ref *
 		display_state->url = xstrdup("foreign");
 
 	display_state->url_len = strlen(display_state->url);
-	for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
+	for (i = display_state->url_len - 1; 0 <= i && display_state->url[i] == '/'; i--)
 		;
 	display_state->url_len = i + 1;
 	if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))

base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4
-- 
gitgitgadget

Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming

[Jeff King]

On Wed, Feb 25, 2026 at 02:00:57AM +0000, cui via GitGitGadget wrote:

> if i == -1, url[i] will be UB.
> [...]
>  	display_state->url_len = strlen(display_state->url);
> -	for (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)
> +	for (i = display_state->url_len - 1; 0 <= i && display_state->url[i] == '/'; i--)
>  		;
>  	display_state->url_len = i + 1;
>  	if (4 < i && !strncmp(".git", display_state->url + i - 3, 4))

Yeah, the original is obviously nonsense. Probably it is not worth too
much effort to add a test here, but I wondered if this can even be
triggered in practice.

We would hit it when there is no non-slash character in the URL. I'm not
sure it's possible to get this far with that, but it makes sense to me
to write it as you have.

I can't help but think this would be easier to read without an empty
loop body, like:

  for (len = strlen(display_state->url); len > 0; len--) {
	if (display_state->url[len-1] != '/')
		break;
  }

which makes it much more clear we never leave the bounds of the string
(and also works with a size_t, which is a more appropriate type than int
here).

But it may not be worth polishing this bit of code too much (if we did,
I'd also suggest strip_suffix_mem() to drop ".git" rather than all of
those magic numbers. Or even stuffing it in a strbuf and using
strbuf_setlen() and strbuf_strip_suffix().

But anyway, your patch seems like an obvious improvement in the
meantime.

-Peff

Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I can't help but think this would be easier to read without an empty
> loop body, like:
>
>   for (len = strlen(display_state->url); len > 0; len--) {
> 	if (display_state->url[len-1] != '/')
> 		break;
>   }
>
> which makes it much more clear we never leave the bounds of the string
> (and also works with a size_t, which is a more appropriate type than int
> here).

Yes, this is vastly more readable, even though what it does is
exactly the same as the original.

Or instead of having strlen() to scan the entire string once and
then ourselves scan backwards from the end, scan forward ourselves
only once while noting where the last non-slash byte was, or
something.

> But it may not be worth polishing this bit of code too much (if we did,
> I'd also suggest strip_suffix_mem() to drop ".git" rather than all of
> those magic numbers. Or even stuffing it in a strbuf and using
> strbuf_setlen() and strbuf_strip_suffix().

True.  None of these "we could do it this way too" bikeshedding has
much value.  The patch posted is an obvious and trivial enough
improvement.

Thanks.