BUG: "git branch --contains <commit> <name>" does nothing, silently fails

Ran into this when preparing my --no-contains series, this is a long standing bug:

    $ ./git branch -D test; ./git branch --contains v2.8.0 test; echo
$?; git rev-parse test
    error: branch 'test' not found.
    0
    test
    fatal: ambiguous argument 'test': unknown revision or path not in
the working tree.

I.e. this should return an error like "git tag" does:

    $ ./git tag -d test; ./git tag --contains v2.8.0 test; echo $?;
git rev-parse test
    error: tag 'test' not found.
    fatal: --contains option is only allowed with -l.
    128
    test
    fatal: ambiguous argument 'test': unknown revision or path not in
the working tree.

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Ran into this when preparing my --no-contains series, this is a long standing bug:

    $ ./git branch -D test; ./git branch --contains v2.8.0 test; echo
$?; git rev-parse test
    error: branch 'test' not found.
    0
    test
    fatal: ambiguous argument 'test': unknown revision or path not in
the working tree.

Isn't that asking to list branches starting with "test" that contain v2.8.0? There are none to report (since you just made sure to delete it), so the empty output is correct.

Quoted 10 lines

I.e. this should return an error like "git tag" does:

    $ ./git tag -d test; ./git tag --contains v2.8.0 test; echo $?;
git rev-parse test
    error: tag 'test' not found.
    fatal: --contains option is only allowed with -l.
    128
    test
    fatal: ambiguous argument 'test': unknown revision or path not in
the working tree.

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

Change these invocations which currently error out without the -l, to behave as if though -l was provided:

    git tag -l [--contains|--points-at|--[no-]merged] <commit>

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 <[email protected]> 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 <[email protected]>

---

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(-)

Documentation/git-tag.txt +0 -0

index 525737a5d8..c80d9e11ba 100644

Documentation/git-tag.txt +0 -0

 --sort=<key>::
        Sort based on the key given.  Prefix `-` to sort in

builtin/tag.c +6 -6

index ad29be6923..6ab65bcf6b 100644
@@ -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') {

t/t7004-tag.sh +21 -5

index b4698ab5f5..e0306ee5a8 100755
@@ -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

Quoted 5 lines

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

I haven't looked at the patch text of this one closely yet, but I think the goals of both make sense, so we would eventually want to have them both.
...
So perhaps you would want this applied first, so that existing three can already benefit from "implicit --list" before waiting for the other one?

Quoted 5 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines

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

I haven't looked at the patch text of this one closely yet, but I think the goals of both make sense, so we would eventually want to have them both.

I also think that "if you said --contains, --merged, etc. you are already asking to give you a list and cannot be creating a new one", which is the topic of this patch, makes sense even if nobody were interested in asking "--no-contains".

So perhaps you would want this applied first, so that existing three can already benefit from "implicit --list" before waiting for the other one?

Change these invocations which currently error out without the -l, to behave as if though -l was provided:

    git tag -l [--contains|--points-at|--[no-]merged] <commit>

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

builtin/tag.c +1 -0

+	/* Just plain "git tag" is like "git tag --list" */
 	if (argc == 0 && !cmdmode)
 		cmdmode = 'l';
 

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Change these invocations which currently error out without the -l, to behave as if though -l was provided:

    git tag -l [--contains|--points-at|--[no-]merged] <commit>

Shouldn't this be

        git tag -l [[--[no-]contains|--points-at|--[no-]merged] <commit>] [<pattern>]

i.e. if you are giving <commit> you need how that commit is used in filtering, but you do not have to give any such filter when listing, and <pattern>, when given, is used to further limit the output, but it also is optional.

Quoted 1 line

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

s/Implicit/implicit/ (ask "git shortlog --no-merges" over recent history)

Documentation/git-tag.txt OPTIONS · line 99

Quoted 11 lines

Documentation/git-tag.txt +3 -0

index 525737a5d8..c80d9e11ba 100644
@@ -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.

Who are "we"?

When any option that only makes sense in the list mode (e.g. --contains) is given, the command defaults to the --list mode.

By the way, do we catch it as a command line error when options like --points-at are given when we are creating a new tag?

builtin/tag.c int cmd_tag(int argc, const char **argv, const char *prefix) · line 458

Quoted 10 lines

builtin/tag.c +2 -0

index ad29be6923..6ab65bcf6b 100644
@@ -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 */

/*
* We write multi-line comments like this,
* without anything other than slash-asterisk or
* asterisk-slash on the first and last lines. */

builtin/tag.c int cmd_tag(int argc, const char **argv, const char *prefix) · line 460

Quoted 2 lines

builtin/tag.c +2 -0

+     if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
+             cmdmode = 'l';

Don't we want to make sure we do the defaulting only upon !cmdmode? Doesn't this start ignoring

        tag -a -m foo --points-at HEAD bar

as an error otherwise?

Quoted 3 lines

builtin/tag.c +1 -0

+     /* Just plain "git tag" is like "git tag --list" */
      if (argc == 0 && !cmdmode)
              cmdmode = 'l';

builtin/tag.c int cmd_tag(int argc, const char **argv, const char *prefix) · line 494

Quoted 10 lines

builtin/tag.c +0 -6

@@ -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"));

And I do not think removal of these check is a good idea at all. Perhaps you were too focused on '-l' that you forgot that people may be giving an explicit option like -a or -s, in which case these error checks are still very sensible things to have, no?

Jeff King pointed out in <[email protected]> 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.