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

From: Jeff King <peff@peff.net>
Date: Feb 25, 2026, 14:10 UTC
Message-ID: <20260225141059.GE2139176@coredump.intra.peff.net>
In-Reply-To: <pull.2211.git.git.1771984857879.gitgitgadget@gmail.com>

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

Show 8 quoted lines

> 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