On 05/17/2017 03:38 PM, Jeff King wrote:
> On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:
>
>> From: Jeff King <peff@xxxxxxxx>
>
> This patch did originate with me, but I know you had to fix several
> things to integrate it in your series. So I'll review it anyway, and
> give you full blame for any bugs. :)
>
>> When we are matching refnames "as path" against a pattern, then we
>> know that the beginning of any refname that can match the pattern has
>> to match the part of the pattern up to the first glob character. For
>> example, if the pattern is `refs/heads/foo*bar`, then it can only
>> match a reference that has the prefix `refs/heads/foo`.
>
> That first sentence confused me as to what "as path" meant (I know
> because I worked on this code, and even then it took me a minute to
> parse it).
>
> Maybe just "When we are matching refnames against a pattern" and then
> later something like:
>
> Note that this applies only when the "match_as_path" flag is set
> (i.e., when for-each-ref is the caller), as the matching rules for
> git-branch and git-tag are subtly different.
That is clearer. I'll make the change.
>> +/*
>> + * Find the longest prefix of pattern we can pass to
>> + * for_each_fullref_in(), namely the part of pattern preceding the
>> + * first glob character.
>> + */
>> +static void find_longest_prefix(struct strbuf *out, const char *pattern)
>> +{
>> + const char *p;
>> +
>> + for (p = pattern; *p && !is_glob_special(*p); p++)
>> + ;
>> +
>> + strbuf_add(out, pattern, p - pattern);
>> +}
>
> If I were reviewing this from scratch, I'd probably ask whether it is OK
> in:
>
> refs/heads/m*
>
> to return "refs/heads/m" as the prefix, and not stop at the last
> non-wildcard component ("refs/heads/"). But I happen to know we
> discussed this off-list and you checked that for_each_ref and friends
> are happy with an arbitrary prefix. But I'm calling it out here for
> other reviewers.
I'll add a comment about this, too.
Thanks,
Michael