On Mon, May 08, 2023 at 04:22:08PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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.
Good point, will tweak; thanks.
> > @@ -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.
Agreed.
> > +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.
Yeah, I figured that this series was already getting pretty long, but
that it would be expedient to propagate forward this pattern. But it
should be cleaned up. Let's tag it with #leftoverbits accordingly.
Thanks,
Taylor
