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.
> +/*
> + * 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.
> +/*
> + * This is the same as for_each_fullref_in(), but it tries to iterate
> + * only over the patterns we'll care about. Note that it _doesn't_ do a full
> + * pattern match, so the callback still has to match each ref individually.
> + */
> +static int for_each_fullref_in_pattern(struct ref_filter *filter,
> [...]
The rest of it looks good to me.
-Peff