{"thread":{"id":"65072","subject":"[PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming","startedAt":"2026-02-25T02:01:00Z","lastAt":"2026-02-25T15:36:48Z","messageCount":3,"participants":["cui via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"537061","messageId":"pull.2211.git.git.1771984857879.gitgitgadget@gmail.com","threadId":"65072","inReplyTo":null,"subject":"[PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming","fromName":"cui via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-02-25T02:00:57Z","receivedAt":"2026-02-25T02:01:00Z","isPatch":true,"sender":{"key":"name:cui","avatar":null},"body":"From: cuiweixie \n\nif i == -1, url[i] will be UB.\n\nSigned-off-by: cuiweixie \n---\n fetch: fix wrong evaluation order in URL trailing-slash trimming\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2211%2Fcuiweixie%2Fbugfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2211/cuiweixie/bugfix-v1\nPull-Request: https://github.com/git/git/pull/2211\n\n builtin/fetch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a3bc7e9380..306138c6e5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -722,7 +722,7 @@ static void display_state_init(struct display_state *display_state, struct ref *\n \t\tdisplay_state->url = xstrdup(\"foreign\");\n \n \tdisplay_state->url_len = strlen(display_state->url);\n-\tfor (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)\n+\tfor (i = display_state->url_len - 1; 0 <= i && display_state->url[i] == '/'; i--)\n \t\t;\n \tdisplay_state->url_len = i + 1;\n \tif (4 < i && !strncmp(\".git\", display_state->url + i - 3, 4))\n\nbase-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4\n-- \ngitgitgadget\n"},{"id":"537087","messageId":"20260225141059.GE2139176@coredump.intra.peff.net","threadId":"65072","inReplyTo":"pull.2211.git.git.1771984857879.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-25T14:10:59Z","receivedAt":"2026-02-25T14:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 25, 2026 at 02:00:57AM +0000, cui via GitGitGadget wrote:\n\n> if i == -1, url[i] will be UB.\n> [...]\n> \tdisplay_state->url_len = strlen(display_state->url);\n> -\tfor (i = display_state->url_len - 1; display_state->url[i] == '/' && 0 <= i; i--)\n> +\tfor (i = display_state->url_len - 1; 0 <= i && display_state->url[i] == '/'; i--)\n> \t\t;\n> \tdisplay_state->url_len = i + 1;\n> \tif (4 < i && !strncmp(\".git\", display_state->url + i - 3, 4))\n\nYeah, the original is obviously nonsense. Probably it is not worth too\nmuch effort to add a test here, but I wondered if this can even be\ntriggered in practice.\n\nWe would hit it when there is no non-slash character in the URL. I'm not\nsure it's possible to get this far with that, but it makes sense to me\nto write it as you have.\n\nI can't help but think this would be easier to read without an empty\nloop body, like:\n\n for (len = strlen(display_state->url); len > 0; len--) {\n\tif (display_state->url[len-1] != '/')\n\t\tbreak;\n }\n\nwhich makes it much more clear we never leave the bounds of the string\n(and also works with a size_t, which is a more appropriate type than int\nhere).\n\nBut it may not be worth polishing this bit of code too much (if we did,\nI'd also suggest strip_suffix_mem() to drop \".git\" rather than all of\nthose magic numbers. Or even stuffing it in a strbuf and using\nstrbuf_setlen() and strbuf_strip_suffix().\n\nBut anyway, your patch seems like an obvious improvement in the\nmeantime.\n\n-Peff\n"},{"id":"537091","messageId":"xmqq4in4decx.fsf@gitster.g","threadId":"65072","inReplyTo":"20260225141059.GE2139176@coredump.intra.peff.net","subject":"Re: [PATCH] fetch: fix wrong evaluation order in URL trailing-slash trimming","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-25T15:36:46Z","receivedAt":"2026-02-25T15:36:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I can't help but think this would be easier to read without an empty\n> loop body, like:\n>\n> for (len = strlen(display_state->url); len > 0; len--) {\n> \tif (display_state->url[len-1] != '/')\n> \t\tbreak;\n> }\n>\n> which makes it much more clear we never leave the bounds of the string\n> (and also works with a size_t, which is a more appropriate type than int\n> here).\n\nYes, this is vastly more readable, even though what it does is\nexactly the same as the original.\n\nOr instead of having strlen() to scan the entire string once and\nthen ourselves scan backwards from the end, scan forward ourselves\nonly once while noting where the last non-slash byte was, or\nsomething.\n\n> But it may not be worth polishing this bit of code too much (if we did,\n> I'd also suggest strip_suffix_mem() to drop \".git\" rather than all of\n> those magic numbers. Or even stuffing it in a strbuf and using\n> strbuf_setlen() and strbuf_strip_suffix().\n\nTrue. None of these \"we could do it this way too\" bikeshedding has\nmuch value. The patch posted is an obvious and trivial enough\nimprovement.\n\nThanks.\n"}]}