From: Jeff King <peff@peff.net>
Date: Thu, 19 Feb 2026 11:21:49 GMT
Subject: Re: [PATCH 1/4] ref-filter: factor out refname component counting
Message-ID: <20260219112149.GA3529@coredump.intra.peff.net>
In-Reply-To: <xmqqqzqjckgu.fsf@gitster.g>

On Tue, Feb 17, 2026 at 10:07:29AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +	if (len < 0) {
> > +		int i;
> > +		const char *p = refname;
> > +
> > +		/* Find total no of '/' separated path-components */
> > +		for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
> > +			;
> 
> Sorry, but I have no idea what this loop (copied verbatim from the
> original) is trying to do.
> 
> We start at the beginning of the refname string, and while we are in
> the leading run of '/' we increment i to find the end of that
> run. E.g., we start with refname="///foo", p points at the leftmost
> '/', i runs from 0 to 3 at which point p[i] points at the first
> non-'/' character, at which point we do *p++, to make p point at the
> second slash?  Is the dereferencing of the pointer in *p++ a no-op
> that is there only to confuse readers?
> 
> And then p moves to the right until p[i] points at the end of the
> string.  It does count the number of slashes in 'i', but there is no
> satisfying simple answer to this question: "what does p mean while
> this loop runs?".
> 
> Anyway, the conversion looks very faithful to the original.

Heh, I missed your message initially but was independently staring at
this because Coverity complained that the dereference in "*p++" is
useless. Which is...kind of right. It is a void context, so the
dereferenced char goes nowhere and it is a noop. But if you don't do it,
then gcc complains that the two sides of the ternary have mis-matched
types (an int and a pointer). Which is true, but since nobody looks at
the result, it does not matter.

Writing it like:

  int i = 0;
  while (p[i]) {
	if (p[i] == '/')
		i++;
	else
		p++;
  }

perhaps resolves the syntactic confusion. Leaving only the semantic
confusion. ;)

I guess the thinking was that "p+i" represents the traversal, with "i"
encoding the counted slashes (so we must increment _one_ of them each
time). But I cannot fathom how that is easier than counting the slashes
like:

  int slashes = 0;
  for (p = refname; *p; p++) {
	if (*p == '/')
		slashes++;
  }

Which made me wonder if I am missing some corner case, and it is not
just counting slashes. But it must be, because "i" is never incremented
except when we see a slash.

+cc Karthik, the original author, for any wisdom, but the commit is now
almost 10 years old.

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.

-Peff