{"thread":{"id":"64995","subject":"[PATCH] ref-filter: don't declare a strdup'd variable const before writing to it","startedAt":"2026-02-14T05:16:35Z","lastAt":"2026-02-22T17:04:50Z","messageCount":14,"participants":["Collin Funk","Jeff King","Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"536000","messageId":"c752a4a6c750bc485804b43d7b525754e39e5fe0.1771046151.git.collin.funk1@gmail.com","threadId":"64995","inReplyTo":null,"subject":"[PATCH] ref-filter: don't declare a strdup'd variable const before writing to it","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2026-02-14T05:15:57Z","receivedAt":"2026-02-14T05:16:35Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"I generally don't like the casts like in rstrip_ref_components and\nrstrip_ref_components because they force you to write this:\n\n    free((char *)free_ptr);\n\nAnd the const doesn't really benefit readability, in my opinion.\n\nThat is a bit of a seperate topic than fixing the warning, though, so\nI left them as-is.\n\n-- 8< --\n\nWith glibc-2.43 there is the following warning:\n\n    ../ref-filter.c: In function ‘rstrip_ref_components’:\n    ../ref-filter.c:2237:27: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]\n     2237 |                 char *p = strrchr(start, '/');\n          |\n\nWe can remove the const from \"start\" since it is the result of strdup\nand we end up writing to it through \"p\".\n\nSigned-off-by: Collin Funk <collin.funk1@gmail.com>\n---\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3917c4ccd9..183cb6bbd7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2214,7 +2214,7 @@ static const char *lstrip_ref_components(const char *refname, int len)\n static const char *rstrip_ref_components(const char *refname, int len)\n {\n \tlong remaining = len;\n-\tconst char *start = xstrdup(refname);\n+\tchar *start = xstrdup(refname);\n \tconst char *to_free = start;\n \n \tif (len < 0) {\n-- \n2.53.0\n\n"},{"id":"536043","messageId":"20260215085755.GA86262@coredump.intra.peff.net","threadId":"64995","inReplyTo":"c752a4a6c750bc485804b43d7b525754e39e5fe0.1771046151.git.collin.funk1@gmail.com","subject":"[PATCH 0/4] cleaning up ref-filter lstrip/rstrip code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T08:57:55Z","receivedAt":"2026-02-15T08:57:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 13, 2026 at 09:15:57PM -0800, Collin Funk wrote:\n\n> I generally don't like the casts like in rstrip_ref_components and\n> rstrip_ref_components because they force you to write this:\n> \n>     free((char *)free_ptr);\n> \n> And the const doesn't really benefit readability, in my opinion.\n\nAgreed. It is especially egregious in this case because the const\nvariable is called to_free, and so its only purpose is to be non-const. ;)\n\n> That is a bit of a seperate topic than fixing the warning, though, so\n> I left them as-is.\n\nIt is a separate topic, but I feel like this is a good opportunity to\nmake this code less horrible. That is, there are some obvious\nlow-hanging cleanups that make the code more readable, and as a side\neffect we clean up the const confusion. In such cases I think it is\nworth veering off the path a little.\n\nI was going to catalog the numerous flaws I found, but by the time I\nexplained them, I had basically written patches and commit messages. So\nhere is what I would propose instead. I hope I'm not stealing your\nthunder nor knocking us too far off our goal.\n\nThe first three I hope are no-brainers, and the final one fixes the\nglibc const issue. The fourth is perhaps more risky.\n\n  [1/4]: ref-filter: factor out refname component counting\n  [2/4]: ref-filter: simplify lstrip_ref_components() memory handling\n  [3/4]: ref-filter: simplify rstrip_ref_components() memory handling\n  [4/4]: ref-filter: open-code slash search in rstrip_ref_components()\n\n ref-filter.c | 54 +++++++++++++++++-----------------------------------\n 1 file changed, 17 insertions(+), 37 deletions(-)\n\n-Peff\n"},{"id":"536044","messageId":"20260215090052.GA695631@coredump.intra.peff.net","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"[PATCH 1/4] ref-filter: factor out refname component counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T09:00:52Z","receivedAt":"2026-02-15T09:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"lstrip\" and \"rstrip\" options to the %(refname) placeholder both\naccept a negative length, which asks us to keep that many path\ncomponents (rather than stripping that many).\n\nThe code to count components and convert the negative value to a\npositive was copied from lstrip to rstrip in 1a34728e6b (ref-filter: add\nan 'rstrip=<N>' option to atoms which deal with refnames, 2017-01-10).\n\nLet's factor it out into a separate function. This reduces duplication\nand also makes the lstrip/rstrip functions much easier to follow, since\nthe bulk of their code is now the actual stripping.\n\nNote that the computed \"remaining\" value is currently stored as a\n\"long\", so in theory that's what our function should return. But this is\npurely historical. When the variable was added in 0571979bd6 (tag: do\nnot show ambiguous tag names as \"tags/foo\", 2016-01-25), we parsed the\nvalue from strtol(), and thus used a long. But these days we take \"len\"\nas an int, and also use an int to count up components. So let's just\nconsistently use int here. This value could only overflow in a\npathological case (e.g., 4GB worth of \"a/a/...\") and even then will not\nresult in out-of-bounds memory access (we keep stripping until we run\nout of string to parse).\n\nThe minimal Myers diff here is a little hard to read; with --patience\nthe code movement is shown much more clearly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI did generate this with --patience. Using --color-words also helps show\nthat it's a pure code movement.\n\n ref-filter.c | 56 +++++++++++++++++++++-------------------------------\n 1 file changed, 22 insertions(+), 34 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3917c4ccd9..9153331f42 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2173,28 +2173,32 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static int normalize_component_count(const char *refname, int len)\n+{\n+\tif (len < 0) {\n+\t\tint i;\n+\t\tconst char *p = refname;\n+\n+\t\t/* Find total no of '/' separated path-components */\n+\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n+\t\t\t;\n+\t\t/*\n+\t\t * The number of components we need to strip is now\n+\t\t * the total minus the components to be left (Plus one\n+\t\t * because we count the number of '/', but the number\n+\t\t * of components is one more than the no of '/').\n+\t\t */\n+\t\tlen = i + len + 1;\n+\t}\n+\treturn len;\n+}\n+\n static const char *lstrip_ref_components(const char *refname, int len)\n {\n-\tlong remaining = len;\n+\tint remaining = normalize_component_count(refname, len);\n \tconst char *start = xstrdup(refname);\n \tconst char *to_free = start;\n \n-\tif (len < 0) {\n-\t\tint i;\n-\t\tconst char *p = refname;\n-\n-\t\t/* Find total no of '/' separated path-components */\n-\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n-\t\t\t;\n-\t\t/*\n-\t\t * The number of components we need to strip is now\n-\t\t * the total minus the components to be left (Plus one\n-\t\t * because we count the number of '/', but the number\n-\t\t * of components is one more than the no of '/').\n-\t\t */\n-\t\tremaining = i + len + 1;\n-\t}\n-\n \twhile (remaining > 0) {\n \t\tswitch (*start++) {\n \t\tcase '\\0':\n@@ -2213,26 +2217,10 @@ static const char *lstrip_ref_components(const char *refname, int len)\n \n static const char *rstrip_ref_components(const char *refname, int len)\n {\n-\tlong remaining = len;\n+\tint remaining = normalize_component_count(refname, len);\n \tconst char *start = xstrdup(refname);\n \tconst char *to_free = start;\n \n-\tif (len < 0) {\n-\t\tint i;\n-\t\tconst char *p = refname;\n-\n-\t\t/* Find total no of '/' separated path-components */\n-\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n-\t\t\t;\n-\t\t/*\n-\t\t * The number of components we need to strip is now\n-\t\t * the total minus the components to be left (Plus one\n-\t\t * because we count the number of '/', but the number\n-\t\t * of components is one more than the no of '/').\n-\t\t */\n-\t\tremaining = i + len + 1;\n-\t}\n-\n \twhile (remaining-- > 0) {\n \t\tchar *p = strrchr(start, '/');\n \t\tif (!p) {\n-- \n2.53.0.438.gad17e1cd28\n\n"},{"id":"536045","messageId":"20260215090223.GB695631@coredump.intra.peff.net","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"[PATCH 2/4] ref-filter: simplify lstrip_ref_components() memory handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T09:02:23Z","receivedAt":"2026-02-15T09:02:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We're walking forward in the string, skipping path components from\nleft-to-right. So when we've stripped as much as we want, the pointer we\nhave is a complete NUL-terminated string and we can just return it\n(after duplicating it, of course). So there is no need for a temporary\nallocated string.\n\nBut we do make an extra temporary copy due to f0062d3b74 (ref-filter:\nfree item->value and item->value->s, 2018-10-18). This is probably from\ncargo-culting the technique used in rstrip_ref_components(), which\n_does_ need a separate string (since it is stripping from the end and\nties off the temporary string with a NUL).\n\nLet's drop the extra allocation. This is slightly more efficient, but\nmore importantly makes the code much simpler.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9153331f42..eb09fda21b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2196,23 +2196,18 @@ static int normalize_component_count(const char *refname, int len)\n static const char *lstrip_ref_components(const char *refname, int len)\n {\n \tint remaining = normalize_component_count(refname, len);\n-\tconst char *start = xstrdup(refname);\n-\tconst char *to_free = start;\n \n \twhile (remaining > 0) {\n-\t\tswitch (*start++) {\n+\t\tswitch (*refname++) {\n \t\tcase '\\0':\n-\t\t\tfree((char *)to_free);\n \t\t\treturn xstrdup(\"\");\n \t\tcase '/':\n \t\t\tremaining--;\n \t\t\tbreak;\n \t\t}\n \t}\n \n-\tstart = xstrdup(start);\n-\tfree((char *)to_free);\n-\treturn start;\n+\treturn xstrdup(refname);\n }\n \n static const char *rstrip_ref_components(const char *refname, int len)\n-- \n2.53.0.438.gad17e1cd28\n\n"},{"id":"536046","messageId":"20260215090534.GC695631@coredump.intra.peff.net","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"[PATCH 3/4] ref-filter: simplify rstrip_ref_components() memory handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T09:05:34Z","receivedAt":"2026-02-15T09:05:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We're stripping path components from the end of a string, which we do by\nassigning a NUL as we parse each component, shortening the string. This\nrequires an extra temporary buffer to avoid munging our input string.\n\nBut the way that we allocate the buffer is unusual. We have an extra\n\"to_free\" variable. Usually this is used when the access variable is\nconceptually const, like:\n\n   const char *foo;\n   char *to_free = NULL;\n\n   if (...)\n           foo = to_free = xstrdup(...);\n   else\n           foo = some_const_string;\n   ...\n   free(to_free);\n\nBut that's not what's happening here. Our \"start\" variable always points\nto the allocated buffer, and to_free is redundant. Worse, it is marked\nas const itself, requiring a cast when we free it.\n\nLet's drop to_free entirely, and mark \"start\" as non-const, making the\nmemory handling more clear. As a bonus, this also silences a warning\nfrom glibc-2.43 that our call to strrchr() implicitly strips away the\nconst-ness of \"start\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex eb09fda21b..1008b2fd5a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2213,13 +2213,12 @@ static const char *lstrip_ref_components(const char *refname, int len)\n static const char *rstrip_ref_components(const char *refname, int len)\n {\n \tint remaining = normalize_component_count(refname, len);\n-\tconst char *start = xstrdup(refname);\n-\tconst char *to_free = start;\n+\tchar *start = xstrdup(refname);\n \n \twhile (remaining-- > 0) {\n \t\tchar *p = strrchr(start, '/');\n \t\tif (!p) {\n-\t\t\tfree((char *)to_free);\n+\t\t\tfree(start);\n \t\t\treturn xstrdup(\"\");\n \t\t} else\n \t\t\tp[0] = '\\0';\n-- \n2.53.0.438.gad17e1cd28\n\n"},{"id":"536047","messageId":"20260215090744.GD695631@coredump.intra.peff.net","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"[PATCH 4/4] ref-filter: avoid strrchr() in rstrip_ref_components()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T09:07:44Z","receivedAt":"2026-02-15T09:07:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To strip path components from our refname string, we repeatedly call\nstrrchr() to find the trailing slash, shortening the string each time by\nassigning NUL over it. This has two downsides:\n\n  1. Calling strrchr() in a loop is quadratic, since each call has to\n     call strlen() under the hood to find the end of the string (even\n     though we know exactly where it is from the last loop iteration).\n\n  2. We need a temporary buffer, since we're munging the string with NUL\n     as we shorten it (which we must do, because strrchr() has no other\n     way of knowing what we consider the end of the string).\n\nUsing memrchr() would let us fix both of these, but it isn't portable.\nSo instead, let's just open-code the string traversal from back to\nfront as we loop.\n\nI doubt that the quadratic nature is a serious concern. You can see it\nin practice with something like:\n\n  git init\n  git commit --allow-empty -m foo\n  echo \"$(git rev-parse HEAD) refs/heads$(perl -e 'print \"/a\" x 500_000')\" >.git/packed-refs\n  time git for-each-ref --format='%(refname:rstrip=-1)'\n\nThat takes ~5.5s to run on my machine before this patch, and ~11ms\nafter. But I don't think there's a reasonable way for somebody to infect\nyou with such a garbage ref, as the wire protocol is limited to 64k\npkt-lines. The difference is measurable for me for a 32k-component ref\n(about 19ms vs 7ms), so perhaps you could create some chaos by pushing a\nlot of them. But we also run into filesystem limits (if the loose\nbackend is in use), and in practice it seems like there are probably\nsimpler and more effective ways to waste CPU.\n\nLikewise the extra allocation probably isn't really measurable. In fact,\nsince our goal is to return an allocated string, we end up having to\nmake the same allocation anyway (though it is sized to the result,\nrather than the input). My main goal was simplicity in avoiding the need\nto handle cleaning it up in the early return path.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1008b2fd5a..ac32b0e6bb 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2213,17 +2213,15 @@ static const char *lstrip_ref_components(const char *refname, int len)\n static const char *rstrip_ref_components(const char *refname, int len)\n {\n \tint remaining = normalize_component_count(refname, len);\n-\tchar *start = xstrdup(refname);\n+\tconst char *end = refname + strlen(refname);\n \n-\twhile (remaining-- > 0) {\n-\t\tchar *p = strrchr(start, '/');\n-\t\tif (!p) {\n-\t\t\tfree(start);\n+\twhile (remaining > 0) {\n+\t\tif (end == refname)\n \t\t\treturn xstrdup(\"\");\n-\t\t} else\n-\t\t\tp[0] = '\\0';\n+\t\tif (*--end == '/')\n+\t\t\tremaining--;\n \t}\n-\treturn start;\n+\treturn xmemdupz(refname, end - refname);\n }\n \n static const char *show_ref(struct refname_atom *atom, const char *refname)\n-- \n2.53.0.438.gad17e1cd28\n"},{"id":"536049","messageId":"20260215091116.GA695914@coredump.intra.peff.net","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] cleaning up ref-filter lstrip/rstrip code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-15T09:11:16Z","receivedAt":"2026-02-15T09:11:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 15, 2026 at 03:57:55AM -0500, Jeff King wrote:\n\n> > That is a bit of a seperate topic than fixing the warning, though, so\n> > I left them as-is.\n> \n> It is a separate topic, but I feel like this is a good opportunity to\n> make this code less horrible. That is, there are some obvious\n> low-hanging cleanups that make the code more readable, and as a side\n> effect we clean up the const confusion. In such cases I think it is\n> worth veering off the path a little.\n> \n> I was going to catalog the numerous flaws I found, but by the time I\n> explained them, I had basically written patches and commit messages. So\n> here is what I would propose instead. I hope I'm not stealing your\n> thunder nor knocking us too far off our goal.\n> \n> The first three I hope are no-brainers, and the final one fixes the\n> glibc const issue. The fourth is perhaps more risky.\n> \n>   [1/4]: ref-filter: factor out refname component counting\n>   [2/4]: ref-filter: simplify lstrip_ref_components() memory handling\n>   [3/4]: ref-filter: simplify rstrip_ref_components() memory handling\n>   [4/4]: ref-filter: open-code slash search in rstrip_ref_components()\n> \n>  ref-filter.c | 54 +++++++++++++++++-----------------------------------\n>  1 file changed, 17 insertions(+), 37 deletions(-)\n\nBTW, you might notice one further opportunity for cleanup: these\nfunctions return an allocated string via a \"const char *\". But that\nissue is endemic to the ref-filter code, courtesy of f0062d3b74\n(ref-filter: free item->value and item->value->s, 2018-10-18), and we\nshould probably look into cleaning it up all at once.\n\nAnd that crosses my line of \"way off topic, let's leave it for another\nday\". See, I do have _some_ restraint. ;)\n\n-Peff\n"},{"id":"536076","messageId":"87cy25ve6r.fsf@gmail.com","threadId":"64995","inReplyTo":"20260215085755.GA86262@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] cleaning up ref-filter lstrip/rstrip code","fromName":"Collin Funk","fromEmail":"collin.funk1@gmail.com","sentAt":"2026-02-15T22:23:40Z","receivedAt":"2026-02-15T22:23:42Z","isPatch":true,"sender":{"key":"collin.funk1@gmail.com","avatar":"https://avatars.githubusercontent.com/u/65689063?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 13, 2026 at 09:15:57PM -0800, Collin Funk wrote:\n>\n>> I generally don't like the casts like in rstrip_ref_components and\n>> rstrip_ref_components because they force you to write this:\n>> \n>>     free((char *)free_ptr);\n>> \n>> And the const doesn't really benefit readability, in my opinion.\n>\n> Agreed. It is especially egregious in this case because the const\n> variable is called to_free, and so its only purpose is to be non-const. ;)\n>\n>> That is a bit of a seperate topic than fixing the warning, though, so\n>> I left them as-is.\n>\n> It is a separate topic, but I feel like this is a good opportunity to\n> make this code less horrible. That is, there are some obvious\n> low-hanging cleanups that make the code more readable, and as a side\n> effect we clean up the const confusion. In such cases I think it is\n> worth veering off the path a little.\n>\n> I was going to catalog the numerous flaws I found, but by the time I\n> explained them, I had basically written patches and commit messages. So\n> here is what I would propose instead. I hope I'm not stealing your\n> thunder nor knocking us too far off our goal.\n\nNo need to worry about stealing my thunder.\n\n> The first three I hope are no-brainers, and the final one fixes the\n> glibc const issue. The fourth is perhaps more risky.\n>\n>   [1/4]: ref-filter: factor out refname component counting\n>   [2/4]: ref-filter: simplify lstrip_ref_components() memory handling\n>   [3/4]: ref-filter: simplify rstrip_ref_components() memory handling\n>   [4/4]: ref-filter: open-code slash search in rstrip_ref_components()\n\nThe cleanups look good and certainly make the code more understandable.\n\nAlso, I confirm that the last one fixes the glibc-2.43 warning.\n\nCollin\n"},{"id":"536091","messageId":"aZLGAMiDdZ_vplND@pks.im","threadId":"64995","inReplyTo":"20260215090744.GD695631@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] ref-filter: avoid strrchr() in rstrip_ref_components()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-16T07:23:44Z","receivedAt":"2026-02-16T07:23:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Feb 15, 2026 at 04:07:44AM -0500, Jeff King wrote:\n> To strip path components from our refname string, we repeatedly call\n> strrchr() to find the trailing slash, shortening the string each time by\n> assigning NUL over it. This has two downsides:\n> \n>   1. Calling strrchr() in a loop is quadratic, since each call has to\n>      call strlen() under the hood to find the end of the string (even\n>      though we know exactly where it is from the last loop iteration).\n\nAh, indeed, that's something I missed.\n\n>   2. We need a temporary buffer, since we're munging the string with NUL\n>      as we shorten it (which we must do, because strrchr() has no other\n>      way of knowing what we consider the end of the string).\n\nRight, upon reading the preceding patch I figured that we can improve\nthis function even further and avoid the call to `xstrdup()` in the case\nwhere we have less components than we're being asked to strip.\n\n> Using memrchr() would let us fix both of these, but it isn't portable.\n> So instead, let's just open-code the string traversal from back to\n> front as we loop.\n> \n> I doubt that the quadratic nature is a serious concern. You can see it\n> in practice with something like:\n> \n>   git init\n>   git commit --allow-empty -m foo\n>   echo \"$(git rev-parse HEAD) refs/heads$(perl -e 'print \"/a\" x 500_000')\" >.git/packed-refs\n>   time git for-each-ref --format='%(refname:rstrip=-1)'\n> \n> That takes ~5.5s to run on my machine before this patch, and ~11ms\n> after. But I don't think there's a reasonable way for somebody to infect\n> you with such a garbage ref, as the wire protocol is limited to 64k\n> pkt-lines. The difference is measurable for me for a 32k-component ref\n> (about 19ms vs 7ms), so perhaps you could create some chaos by pushing a\n> lot of them. But we also run into filesystem limits (if the loose\n> backend is in use), and in practice it seems like there are probably\n> simpler and more effective ways to waste CPU.\n\nAgreed, not much of a concern, but good regardless to see it being\naddressed.\n\n> Likewise the extra allocation probably isn't really measurable. In fact,\n> since our goal is to return an allocated string, we end up having to\n> make the same allocation anyway (though it is sized to the result,\n> rather than the input). My main goal was simplicity in avoiding the need\n> to handle cleaning it up in the early return path.\n\nLikewise.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 1008b2fd5a..ac32b0e6bb 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2213,17 +2213,15 @@ static const char *lstrip_ref_components(const char *refname, int len)\n>  static const char *rstrip_ref_components(const char *refname, int len)\n>  {\n>  \tint remaining = normalize_component_count(refname, len);\n> -\tchar *start = xstrdup(refname);\n> +\tconst char *end = refname + strlen(refname);\n>  \n> -\twhile (remaining-- > 0) {\n> -\t\tchar *p = strrchr(start, '/');\n> -\t\tif (!p) {\n> -\t\t\tfree(start);\n> +\twhile (remaining > 0) {\n> +\t\tif (end == refname)\n>  \t\t\treturn xstrdup(\"\");\n> -\t\t} else\n> -\t\t\tp[0] = '\\0';\n> +\t\tif (*--end == '/')\n> +\t\t\tremaining--;\n\nWe start scannign from the trailing NUL byte, so this would also cause\nus to detect if the refname had \"/\" as a suffix. But I assume that's a\ncase we don't even need to care about, as refs cannot end with a slash\nanyway.\n\nAnother edge case is if we were passed the empty string, but as we\nalready abort in case we see that `end == refname` we're good there,\ntoo.\n\n>  \t}\n> -\treturn start;\n> +\treturn xmemdupz(refname, end - refname);\n>  }\n\nSo overall this and all the preceding patches look good to me. Thanks!\n\nPatrick\n"},{"id":"536212","messageId":"xmqqqzqjckgu.fsf@gitster.g","threadId":"64995","inReplyTo":"20260215090052.GA695631@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] ref-filter: factor out refname component counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-17T18:07:29Z","receivedAt":"2026-02-17T18:07:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +\tif (len < 0) {\n> +\t\tint i;\n> +\t\tconst char *p = refname;\n> +\n> +\t\t/* Find total no of '/' separated path-components */\n> +\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n> +\t\t\t;\n\nSorry, but I have no idea what this loop (copied verbatim from the\noriginal) is trying to do.\n\nWe start at the beginning of the refname string, and while we are in\nthe leading run of '/' we increment i to find the end of that\nrun. E.g., we start with refname=\"///foo\", p points at the leftmost\n'/', i runs from 0 to 3 at which point p[i] points at the first\nnon-'/' character, at which point we do *p++, to make p point at the\nsecond slash?  Is the dereferencing of the pointer in *p++ a no-op\nthat is there only to confuse readers?\n\nAnd then p moves to the right until p[i] points at the end of the\nstring.  It does count the number of slashes in 'i', but there is no\nsatisfying simple answer to this question: \"what does p mean while\nthis loop runs?\".\n\nAnyway, the conversion looks very faithful to the original.\n"},{"id":"536395","messageId":"20260219112149.GA3529@coredump.intra.peff.net","threadId":"64995","inReplyTo":"xmqqqzqjckgu.fsf@gitster.g","subject":"Re: [PATCH 1/4] ref-filter: factor out refname component counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-19T11:21:49Z","receivedAt":"2026-02-19T11:21:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 17, 2026 at 10:07:29AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +\tif (len < 0) {\n> > +\t\tint i;\n> > +\t\tconst char *p = refname;\n> > +\n> > +\t\t/* Find total no of '/' separated path-components */\n> > +\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n> > +\t\t\t;\n> \n> Sorry, but I have no idea what this loop (copied verbatim from the\n> original) is trying to do.\n> \n> We start at the beginning of the refname string, and while we are in\n> the leading run of '/' we increment i to find the end of that\n> run. E.g., we start with refname=\"///foo\", p points at the leftmost\n> '/', i runs from 0 to 3 at which point p[i] points at the first\n> non-'/' character, at which point we do *p++, to make p point at the\n> second slash?  Is the dereferencing of the pointer in *p++ a no-op\n> that is there only to confuse readers?\n> \n> And then p moves to the right until p[i] points at the end of the\n> string.  It does count the number of slashes in 'i', but there is no\n> satisfying simple answer to this question: \"what does p mean while\n> this loop runs?\".\n> \n> Anyway, the conversion looks very faithful to the original.\n\nHeh, I missed your message initially but was independently staring at\nthis because Coverity complained that the dereference in \"*p++\" is\nuseless. Which is...kind of right. It is a void context, so the\ndereferenced char goes nowhere and it is a noop. But if you don't do it,\nthen gcc complains that the two sides of the ternary have mis-matched\ntypes (an int and a pointer). Which is true, but since nobody looks at\nthe result, it does not matter.\n\nWriting it like:\n\n  int i = 0;\n  while (p[i]) {\n\tif (p[i] == '/')\n\t\ti++;\n\telse\n\t\tp++;\n  }\n\nperhaps resolves the syntactic confusion. Leaving only the semantic\nconfusion. ;)\n\nI guess the thinking was that \"p+i\" represents the traversal, with \"i\"\nencoding the counted slashes (so we must increment _one_ of them each\ntime). But I cannot fathom how that is easier than counting the slashes\nlike:\n\n  int slashes = 0;\n  for (p = refname; *p; p++) {\n\tif (*p == '/')\n\t\tslashes++;\n  }\n\nWhich made me wonder if I am missing some corner case, and it is not\njust counting slashes. But it must be, because \"i\" is never incremented\nexcept when we see a slash.\n\n+cc Karthik, the original author, for any wisdom, but the commit is now\nalmost 10 years old.\n\nIs it worth rewriting to the \"slashes\" form above for clarity? I was\nafraid to touch it just to shut up Coverity, but now we have two\nconfused people.\n\n-Peff\n"},{"id":"536432","messageId":"xmqq8qco5zpm.fsf@gitster.g","threadId":"64995","inReplyTo":"20260219112149.GA3529@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] ref-filter: factor out refname component counting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-19T18:56:53Z","receivedAt":"2026-02-19T18:56:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> And then p moves to the right until p[i] points at the end of the\n>> string.  It does count the number of slashes in 'i', but there is no\n>> satisfying simple answer to this question: \"what does p mean while\n>> this loop runs?\".\n>> ...\n> Which made me wonder if I am missing some corner case, and it is not\n> just counting slashes. But it must be, because \"i\" is never incremented\n> except when we see a slash.\n>\n> +cc Karthik, the original author, for any wisdom, but the commit is now\n> almost 10 years old.\n>\n> Is it worth rewriting to the \"slashes\" form above for clarity? I was\n> afraid to touch it just to shut up Coverity, but now we have two\n> confused people.\n\nYup, I think the answer to my \"what does p mean?\" question is \"by\nitself p has *no* meaning, but (p-refname) is maintained to be the\nnumber of non-slash bytes we scanned so far, while i is the number\nof slashes.\"\n\nAnd from that point of view, your \"count slashes in the most stupid\nway that even 5 year old understands\" certainly does make the result\nfar easier to read.\n\nThanks.\n"},{"id":"536460","messageId":"20260220060003.GA26256@coredump.intra.peff.net","threadId":"64995","inReplyTo":"xmqq8qco5zpm.fsf@gitster.g","subject":"[PATCH] ref-filter: clarify lstrip/rstrip component counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-02-20T06:00:03Z","receivedAt":"2026-02-20T06:00:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 19, 2026 at 10:56:53AM -0800, Junio C Hamano wrote:\n\n> > Is it worth rewriting to the \"slashes\" form above for clarity? I was\n> > afraid to touch it just to shut up Coverity, but now we have two\n> > confused people.\n> \n> Yup, I think the answer to my \"what does p mean?\" question is \"by\n> itself p has *no* meaning, but (p-refname) is maintained to be the\n> number of non-slash bytes we scanned so far, while i is the number\n> of slashes.\"\n> \n> And from that point of view, your \"count slashes in the most stupid\n> way that even 5 year old understands\" certainly does make the result\n> far easier to read.\n\nHere it is in patch form. Probably not worth as many words as I wrote in\nthe commit message, but most of it is just summarizing our earlier\nfindings.\n\nI do notice that this function may not do what we want for\n\"/absolute/ref/name\" or for \"refs//with//double//slashes\". But I don't\nthink it should see either of those, as it would always get normalized\nrefnames from Git itself. So I think we can ignore it for now.\n\n-- >8 --\nSubject: [PATCH] ref-filter: clarify lstrip/rstrip component counting\n\nWhen a strip option to the %(refname) placeholder is asked to leave N\npath components, we first count up the path components to know how many\nto remove. That happens with a loop like this:\n\n\t/* Find total no of '/' separated path-components */\n\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n\t\t;\n\nwhich is a little hard to understand for two reasons.\n\nFirst, the dereference in \"*p++\" is seemingly useless, since nobody\nlooks at the result. And static analyzers like Coverity will complain\nabout that. But removing the \"*\" will cause gcc to complain with\n-Wint-conversion, since the two sides of the ternary do not match (one\nis a pointer and the other an int).\n\nSecond, it is not clear what the meaning of \"p\" is at each iteration of\nthe loop, as its position with respect to our walk over the string\ndepends on how many slashes we've seen. The answer is that by itself, it\ndoesn't really mean anything: \"p + i\" represents the current state of\nour walk, with \"i\" counting up slashes, and \"p\" by itself essentially\nmeaningless.\n\nNone of this behaves incorrectly, but ultimately the loop is just\ncounting the slashes in the refname. We can do that much more simply\nwith a for-loop iterating over the string and a separate slash counter.\n\nWe can also drop the comment, which is somewhat misleading. We are\ncounting slashes, not components (and a comment later in the function\nmakes it clear that we must add one to compensate). In the new code it\nis obvious that we are counting slashes here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ac32b0e6bb..6bbb6fac18 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2176,19 +2176,20 @@ static inline char *copy_advance(char *dst, const char *src)\n static int normalize_component_count(const char *refname, int len)\n {\n \tif (len < 0) {\n-\t\tint i;\n-\t\tconst char *p = refname;\n+\t\tint slashes = 0;\n+\n+\t\tfor (const char *p = refname; *p; p++) {\n+\t\t\tif (*p == '/')\n+\t\t\t\tslashes++;\n+\t\t}\n \n-\t\t/* Find total no of '/' separated path-components */\n-\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n-\t\t\t;\n \t\t/*\n \t\t * The number of components we need to strip is now\n \t\t * the total minus the components to be left (Plus one\n \t\t * because we count the number of '/', but the number\n \t\t * of components is one more than the no of '/').\n \t\t */\n-\t\tlen = i + len + 1;\n+\t\tlen = slashes + len + 1;\n \t}\n \treturn len;\n }\n-- \n2.53.0.528.g678f28d038\n\n"},{"id":"536650","messageId":"CAOLa=ZRr-Oa-aSzMBOnKWdyjMxuo6cd6mpcCydDrN7SMe2ahjQ@mail.gmail.com","threadId":"64995","inReplyTo":"20260219112149.GA3529@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] ref-filter: factor out refname component counting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-22T17:04:47Z","receivedAt":"2026-02-22T17:04:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Feb 17, 2026 at 10:07:29AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>> > +\tif (len < 0) {\n>> > +\t\tint i;\n>> > +\t\tconst char *p = refname;\n>> > +\n>> > +\t\t/* Find total no of '/' separated path-components */\n>> > +\t\tfor (i = 0; p[i]; p[i] == '/' ? i++ : *p++)\n>> > +\t\t\t;\n>>\n>> Sorry, but I have no idea what this loop (copied verbatim from the\n>> original) is trying to do.\n>>\n>> We start at the beginning of the refname string, and while we are in\n>> the leading run of '/' we increment i to find the end of that\n>> run. E.g., we start with refname=\"///foo\", p points at the leftmost\n>> '/', i runs from 0 to 3 at which point p[i] points at the first\n>> non-'/' character, at which point we do *p++, to make p point at the\n>> second slash?  Is the dereferencing of the pointer in *p++ a no-op\n>> that is there only to confuse readers?\n>>\n>> And then p moves to the right until p[i] points at the end of the\n>> string.  It does count the number of slashes in 'i', but there is no\n>> satisfying simple answer to this question: \"what does p mean while\n>> this loop runs?\".\n>>\n>> Anyway, the conversion looks very faithful to the original.\n>\n> Heh, I missed your message initially but was independently staring at\n> this because Coverity complained that the dereference in \"*p++\" is\n> useless. Which is...kind of right. It is a void context, so the\n> dereferenced char goes nowhere and it is a noop. But if you don't do it,\n> then gcc complains that the two sides of the ternary have mis-matched\n> types (an int and a pointer). Which is true, but since nobody looks at\n> the result, it does not matter.\n>\n> Writing it like:\n>\n>   int i = 0;\n>   while (p[i]) {\n> \tif (p[i] == '/')\n> \t\ti++;\n> \telse\n> \t\tp++;\n>   }\n>\n> perhaps resolves the syntactic confusion. Leaving only the semantic\n> confusion. ;)\n>\n> I guess the thinking was that \"p+i\" represents the traversal, with \"i\"\n> encoding the counted slashes (so we must increment _one_ of them each\n> time). But I cannot fathom how that is easier than counting the slashes\n> like:\n>\n>   int slashes = 0;\n>   for (p = refname; *p; p++) {\n> \tif (*p == '/')\n> \t\tslashes++;\n>   }\n>\n> Which made me wonder if I am missing some corner case, and it is not\n> just counting slashes. But it must be, because \"i\" is never incremented\n> except when we see a slash.\n>\n> +cc Karthik, the original author, for any wisdom, but the commit is now\n> almost 10 years old.\n>\n\nI'm embarrassed and frankly don't remember this code :) Your new patch\nlooks sensible to me.\n\n> Is it worth rewriting to the \"slashes\" form above for clarity? I was\n> afraid to touch it just to shut up Coverity, but now we have two\n> confused people.\n>\n> -Peff\n"}]}