Re: [PATCH] tag: Implicitly supply --list given another list-like option

> I ran into what turned out to be not-a-bug in "branch" where it,
> unlike "tag" before this patch, accepts input like:
>
>     git branch --contains v2.8.0 <pattern>
>
> Jeff King pointed out in
> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to
> that::
>
>     The difference between "branch" and "tag" here is that "branch
>     --contains" implies "--list" (and the argument becomes a pattern).
>     Whereas "tag --contains" just detects the situation and complains.
>
>     If anything, I'd think we would consider teaching "tag" to behave
>     more like "branch".
>
> I agree. This change does that, the only tests that broke as a result
> of this were tests that were explicitly checking that this
> "branch-like" usage wasn't permitted, i.e. no actual breakages
> occurred, and I can't imagine an invocation that would negatively
> impact backwards compatibility, i.e. these invocations all just
> errored out before.
>
> Spelunking through the history via:
>
>     git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'
>
> Reveals that there was no good reason for this in the first place. The
> --contains option added in v1.6.1.1-243-g32c35cfb1e made this an
> error, and all the other subsequent options I'm changing here just
> copy/pasted its pattern.
>
> I've changed the failing tests to check that this invocation mode is
> permitted instead, and added extra tests for the list-like options we
> weren't testing.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Junio: This will merge conflict with my in-flight --no-contains
> patch. I can re-send either one depending on which you want to accept
> first, this patch will need an additional test for --no-contains. I
> just wanted to get this on the ML for review before the --no-contains
> patch hit "master".
>
>  Documentation/git-tag.txt |  3 +++
>  builtin/tag.c             | 12 ++++++------
>  t/t7004-tag.sh            | 25 +++++++++++++++++++++----
>  3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 525737a5d8..c80d9e11ba 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -94,6 +94,9 @@ OPTIONS
>         lists all tags. The pattern is a shell wildcard (i.e., matched
>         using fnmatch(3)).  Multiple patterns may be given; if any of
>         them matches, the tag is shown.
> ++
> +We supply this option implicitly if any other list-like option is
> +provided. E.g. `--contains`, `--points-at` etc.

>
>  --sort=<key>::
>         Sort based on the key given.  Prefix `-` to sort in
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..6ab65bcf6b 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>         }
>         create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>
> +       /* We implicitly supply --list with --contains, --points-at,
> +          --merged and --no-merged, just like git-branch */
> +       if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> +               cmdmode = 'l';
> +
> +       /* Just plain "git tag" is like "git tag --list" */
>         if (argc == 0 && !cmdmode)
>                 cmdmode = 'l';
>
> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>         }
>         if (filter.lines != -1)
>                 die(_("-n option is only allowed with -l."));
> -       if (filter.with_commit)
> -               die(_("--contains option is only allowed with -l."));
> -       if (filter.points_at.nr)
> -               die(_("--points-at option is only allowed with -l."));
> -       if (filter.merge_commit)
> -               die(_("--merged and --no-merged option are only allowed with -l"));
>         if (cmdmode == 'd')
>                 return for_each_tag_name(argv, delete_tag, NULL);
>         if (cmdmode == 'v') {
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index b4698ab5f5..e0306ee5a8 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1453,6 +1453,11 @@ test_expect_success 'checking that initial commit is in all tags' "
>         test_cmp expected actual
>  "
>
> +test_expect_success 'checking that --contains can be used in non-list mode' '
> +       git tag --contains $hash1 v* >actual &&
> +       test_cmp expected actual
> +'
> +
>  # mixing modes and options:
>
>  test_expect_success 'mixing incompatibles modes and options is forbidden' '
> @@ -1466,8 +1471,10 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
>
>  # check points-at
>
> -test_expect_success '--points-at cannot be used in non-list mode' '
> -       test_must_fail git tag --points-at=v4.0 foo
> +test_expect_success '--points-at can be used in non-list mode' '
> +       echo v4.0 >expect &&
> +       git tag --points-at=v4.0 "v*" >actual &&
> +       test_cmp expect actual
>  '
>
>  test_expect_success '--points-at finds lightweight tags' '
> @@ -1744,8 +1751,13 @@ test_expect_success 'setup --merged test tags' '
>         git tag mergetest-3 HEAD
>  '
>
> -test_expect_success '--merged cannot be used in non-list mode' '
> -       test_must_fail git tag --merged=mergetest-2 foo
> +test_expect_success '--merged can be used in non-list mode' '
> +       cat >expect <<-\EOF &&
> +       mergetest-1
> +       mergetest-2
> +       EOF
> +       git tag --merged=mergetest-2 "mergetest*" >actual &&
> +       test_cmp expect actual
>  '
>
>  test_expect_success '--merged shows merged tags' '
> @@ -1765,6 +1777,11 @@ test_expect_success '--no-merged show unmerged tags' '
>         test_cmp expect actual
>  '
>
> +test_expect_success '--no-merged can be used in non-list mode' '
> +       git tag --no-merged=mergetest-2 mergetest-* >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'ambiguous branch/tags not marked' '
>         git tag ambiguous &&
>         git branch ambiguous &&
> --
> 2.11.0
>