From: Jeff King Date: Wed, 25 Feb 2026 14:10:59 GMT Subject: Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming Message-ID: <20260225141059.GE2139176@coredump.intra.peff.net> In-Reply-To: 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