Patrick Steinhardt wrote: >> Allowing prefix="refs/heads/v1.0" to yield entry="refs/heads/v1" >> (case #2 above that this patch fixes the behaviour for) would cause >> ref_iterator_advance() to return a ref outside the hierarhcy, >> wouldn't it? So it appears to me that either one of the two would >> be true: >> >> * the code is structured in such a way that such a condition does >> not actually happen (in which case this patch would be a no-op), >> or >> >> * there is a bug in the current code that is fixed by this patch, >> whose externally observable behaviour can be verified with a >> test. >> >> It is not quite clear to me which is the case here. The code with >> the patch looks more logical than the original, but I am not sure >> how to demonstrate the existing breakage (if any). > > Agreed, I also had a bit of a hard time to figure out whether this is an > actual bug fix, a performance improvement or merely a refactoring. > I originally operated on the assumption that it was the first case, which is why I didn't include a test in this patch. Commands like 'for-each-ref', 'show-ref', etc. either use an empty prefix or a directory prefix with a trailing slash, which won't trigger this issue. I encountered the problem while working on a builtin that filtered refs by a user-specified prefix - the results included refs that should not have been matched, which led me to this fix. Scanning through the codebase again, though, I do see a way to replicate the issue: $ git update-ref refs/bisect/b HEAD $ git rev-parse --abbrev-ref --bisect refs/bisect/b Because 'rev-parse --bisect' uses the "refs/bisect/bad" prefix (no trailing slash) and does no additional filtering in its 'for_each_fullref_in' callback, refs like "refs/bisect/b" and "refs/bisect/ba" are (incorrectly) matched. I'll re-roll with the added test.
