Re: [PATCH 06/15] builtin/for-each-ref.c: add `--exclude` option

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index c01fa6fefe..449da61e11 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -14,6 +14,7 @@ static char const * const for_each_ref_usage[] = {
>  	N_("git for-each-ref [--points-at <object>]"),
>  	N_("git for-each-ref [--merged [<commit>]] [--no-merged [<commit>]]"),
>  	N_("git for-each-ref [--contains [<commit>]] [--no-contains [<commit>]]"),
> +	N_("git for-each-ref [--exclude=<pattern> ...]"),
>  	NULL
>  };

I think the original is already wrong, but the easiest thing we can
do in order to avoid making it worse is to drop this hunk, as the
existing usage is this:

static char const * const for_each_ref_usage[] = {
	N_("git for-each-ref [<options>] [<pattern>]"),
	N_("git for-each-ref [--points-at <object>]"),
	N_("git for-each-ref [--merged [<commit>]] [--no-merged [<commit>]]"),
	N_("git for-each-ref [--contains [<commit>]] [--no-contains [<commit>]]"),
	NULL
};

and this series merely adds a new "--exclude=<pattern>" as one of
the "<options>". 

As we can see from the fact that for example

 $ git for-each-ref --no-merged next refs/heads/\?\?/\*

works just fine, exactly the same thing can be said about the other
--points-at/--merged/--no-merged/--contains/--no-contains options.

The SYNOPSIS section of the manual page is fine.

> diff --git a/ref-filter.c b/ref-filter.c
> index 6c5eed144f..93dc9b331f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2169,6 +2169,15 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)
>  	return match_pattern(filter, filter->name_patterns, refname);
>  }
>  
> +static int filter_exclude_match(struct ref_filter *filter, const char *refname)
> +{
> +	if (!filter->exclude.nr)
> +		return 0;
> +	if (filter->match_as_path)
> +		return match_name_as_path(filter, filter->exclude.v, refname);
> +	return match_pattern(filter, filter->exclude.v, refname);
> +}

Earlier I made a comment about .name_patterns member becoming
unnecessary, but I think what should need to happen is instead
match_pattern() and match_name_as_path() to lose the "filter"
parameter, and take a boolean "ignore_case" instead.

>  struct ref_filter {
>  	const char **name_patterns;
> +	struct strvec exclude;

At some point after the dust settles, we may want to tweak
name_patterns so that these two appear to complement each other more
clearly, e.g. use "struct strvec include" to replace "name_patterns"
or something.  But that is an unrelated tangent.

In any case, there wasn't anything surprising or unexpected in the
code.  Looking good.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 5c00607608..7e8d578522 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -447,6 +447,41 @@ test_expect_success 'exercise glob patterns with prefixes' '
>  	test_cmp expected actual
>  '
>  
> +cat >expected <<\EOF
> +refs/tags/bar
> +refs/tags/baz
> +refs/tags/testtag
> +EOF
> +
> +test_expect_success 'exercise patterns with prefix exclusions' '
> +	for tag in foo/one foo/two foo/three bar baz
> +	do
> +		git tag "$tag" || return 1
> +	done &&
> +	test_when_finished "git tag -d foo/one foo/two foo/three bar baz" &&
> +	git for-each-ref --format="%(refname)" \
> +		refs/tags/ --exclude=refs/tags/foo >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<\EOF
> +refs/tags/bar
> +refs/tags/baz
> +refs/tags/foo/one
> +refs/tags/testtag
> +EOF
> +
> +test_expect_success 'exercise patterns with pattern exclusions' '
> +	for tag in foo/one foo/two foo/three bar baz
> +	do
> +		git tag "$tag" || return 1
> +	done &&
> +	test_when_finished "git tag -d foo/one foo/two foo/three bar baz" &&
> +	git for-each-ref --format="%(refname)" \
> +		refs/tags/ --exclude="refs/tags/foo/t*" >actual &&
> +	test_cmp expected actual
> +'

These are doing as Romans do, so I won't comment on the outdated
pattern of preparing the expectation outside the test script.  After
the dust settles, somebody needs to go in and clean it up.

Thanks.