From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Karthik Nayak <karthik.188@gmail.com>,
Collin Funk <collin.funk1@gmail.com>,
git@vger.kernel.org
Subject: [PATCH] ref-filter: clarify lstrip/rstrip component counting
Date: Fri, 20 Feb 2026 01:00:03 -0500 [thread overview]
Message-ID: <20260220060003.GA26256@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq8qco5zpm.fsf@gitster.g>
On Thu, Feb 19, 2026 at 10:56:53AM -0800, Junio C Hamano wrote:
> > Is it worth rewriting to the "slashes" form above for clarity? I was
> > afraid to touch it just to shut up Coverity, but now we have two
> > confused people.
>
> Yup, I think the answer to my "what does p mean?" question is "by
> itself p has *no* meaning, but (p-refname) is maintained to be the
> number of non-slash bytes we scanned so far, while i is the number
> of slashes."
>
> And from that point of view, your "count slashes in the most stupid
> way that even 5 year old understands" certainly does make the result
> far easier to read.
Here it is in patch form. Probably not worth as many words as I wrote in
the commit message, but most of it is just summarizing our earlier
findings.
I do notice that this function may not do what we want for
"/absolute/ref/name" or for "refs//with//double//slashes". But I don't
think it should see either of those, as it would always get normalized
refnames from Git itself. So I think we can ignore it for now.
-- >8 --
Subject: [PATCH] ref-filter: clarify lstrip/rstrip component counting
When a strip option to the %(refname) placeholder is asked to leave N
path components, we first count up the path components to know how many
to remove. That happens with a loop like this:
/* Find total no of '/' separated path-components */
for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
;
which is a little hard to understand for two reasons.
First, the dereference in "*p++" is seemingly useless, since nobody
looks at the result. And static analyzers like Coverity will complain
about that. But removing the "*" will cause gcc to complain with
-Wint-conversion, since the two sides of the ternary do not match (one
is a pointer and the other an int).
Second, it is not clear what the meaning of "p" is at each iteration of
the loop, as its position with respect to our walk over the string
depends on how many slashes we've seen. The answer is that by itself, it
doesn't really mean anything: "p + i" represents the current state of
our walk, with "i" counting up slashes, and "p" by itself essentially
meaningless.
None of this behaves incorrectly, but ultimately the loop is just
counting the slashes in the refname. We can do that much more simply
with a for-loop iterating over the string and a separate slash counter.
We can also drop the comment, which is somewhat misleading. We are
counting slashes, not components (and a comment later in the function
makes it clear that we must add one to compensate). In the new code it
is obvious that we are counting slashes here.
Signed-off-by: Jeff King <peff@peff.net>
---
ref-filter.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index ac32b0e6bb..6bbb6fac18 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2176,19 +2176,20 @@ static inline char *copy_advance(char *dst, const char *src)
static int normalize_component_count(const char *refname, int len)
{
if (len < 0) {
- int i;
- const char *p = refname;
+ int slashes = 0;
+
+ for (const char *p = refname; *p; p++) {
+ if (*p == '/')
+ slashes++;
+ }
- /* Find total no of '/' separated path-components */
- for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
- ;
/*
* The number of components we need to strip is now
* the total minus the components to be left (Plus one
* because we count the number of '/', but the number
* of components is one more than the no of '/').
*/
- len = i + len + 1;
+ len = slashes + len + 1;
}
return len;
}
--
2.53.0.528.g678f28d038
next prev parent reply other threads:[~2026-02-20 6:00 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-14 5:15 [PATCH] ref-filter: don't declare a strdup'd variable const before writing to it Collin Funk
2026-02-15 8:57 ` [PATCH 0/4] cleaning up ref-filter lstrip/rstrip code Jeff King
2026-02-15 9:00 ` [PATCH 1/4] ref-filter: factor out refname component counting Jeff King
2026-02-17 18:07 ` Junio C Hamano
2026-02-19 11:21 ` Jeff King
2026-02-19 18:56 ` Junio C Hamano
2026-02-20 6:00 ` Jeff King [this message]
2026-02-22 17:04 ` Karthik Nayak
2026-02-15 9:02 ` [PATCH 2/4] ref-filter: simplify lstrip_ref_components() memory handling Jeff King
2026-02-15 9:05 ` [PATCH 3/4] ref-filter: simplify rstrip_ref_components() " Jeff King
2026-02-15 9:07 ` [PATCH 4/4] ref-filter: avoid strrchr() in rstrip_ref_components() Jeff King
2026-02-16 7:23 ` Patrick Steinhardt
2026-02-15 9:11 ` [PATCH 0/4] cleaning up ref-filter lstrip/rstrip code Jeff King
2026-02-15 22:23 ` Collin Funk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260220060003.GA26256@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=collin.funk1@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=karthik.188@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox