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

From: Jeff King <peff@peff.net>
Date: May 17, 2017, 13:38 UTC
Message-ID: <20170517133835.s3m3giccjo3jqstg@sigill.intra.peff.net>
In-Reply-To: <1f50142ccbb84a4e5d7a1cd67e6f9d30edc1d73a.1495014840.git.mhagger@alum.mit.edu>

On Wed, May 17, 2017 at 02:05:45PM +0200, Michael Haggerty wrote:

> From: Jeff King <peff@peff.net>

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

Show 5 quoted lines

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

Show 14 quoted lines

> +/*
> + * 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.

Show 7 quoted lines

> +/*
> + * 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