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

[Jeff King <peff@peff.net>]

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