Web lists-archives.com

Re: [PATCH 22/23] ref-filter: limit traversal to prefix




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