Re: [PATCH 1/4] ref-cache.c: fix prefix matching in ref iteration

From: Victoria Dye <vdye@github.com>
Date: Oct 9, 2023, 16:21 UTC
Message-ID: <3585d72f-9f06-d190-ad5a-bec6db3f647f@github.com>
In-Reply-To: <ZSPQLjJwq-7SjsDT@tanuki>

Patrick Steinhardt wrote:

Show 21 quoted lines

>> 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.