Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming
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
