From: Karthik Nayak Date: Sun, 22 Feb 2026 17:04:47 GMT Subject: Re: [PATCH 1/4] ref-filter: factor out refname component counting Message-ID: In-Reply-To: <20260219112149.GA3529@coredump.intra.peff.net> Jeff King writes: > On Tue, Feb 17, 2026 at 10:07:29AM -0800, Junio C Hamano wrote: > >> Jeff King 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. > I'm embarrassed and frankly don't remember this code :) Your new patch looks sensible to me. > 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