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

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

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

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.

I briefly looked through builtin/branch.c, couldn't find a trivial way
to patch it, unfamiliar with the code, didn't want to forget about it,
hence this E-Mail.

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

[Jeff King]

On Fri, Mar 10, 2017 at 11:43:15AM +0100, Ævar Arnfjörð Bjarmason wrote:

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

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

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 10, 2017 at 1:42 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 10, 2017 at 11:43:15AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
>> 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".

Yes you're right, sorry about the noise, e.g. this works:

    git branch --contains v2.8.0 'a*'
    git branch --contains v2.8.0 --list 'a*'

Whereas with "git tag" you always need -l as you point out.

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

[Æ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>

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 11, 2017 at 1:08 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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>

Oops, this should be:

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

Will fix in v2 pending other comments.

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

The "this option" I'm referring to is --list, this is a patch to the
--list section in "git help tag".

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Ah, I see that you already have the other topic polished to v4
(sorry, but I am a bit behind the list traffic).  If it makes your
life easier, it is OK to make the current three to four by adding
"--no-contains" first and then make all/any of them imply "--list"
on top.  

IOW, either is fine.  Personally, I have a suspicion that sending
both at the same time, expecting the maintainer to be able to
resolve the potential conflicts correctly, would also be fine ;-)

Thanks.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

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

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

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

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?

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

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

> +	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?

> +	/* 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"));

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?

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 12, 2017 at 3:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> 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?

Yes, let's do this one first. I'll address the comments that have come
up & just make this all part of one series on top of JK's patches.

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 12, 2017 at 4:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
>> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option
>
> s/Implicit/implicit/ (ask "git shortlog --no-merges" over recent history)
>
>> 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.
>
> 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?
>
>> 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 */
>
>         /*
>          * We write multi-line comments like this,
>          * without anything other than slash-asterisk or
>          * asterisk-slash on the first and last lines.
>          */
>
>> +     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?
>
>> +     /* 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"));
>
> 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?

I'll fix this up and make sure to do more sanity checks with the
different option combinations before resending.

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

[Jeff King]

On Sat, Mar 11, 2017 at 12:08:55PM +0000, Ævar Arnfjörð Bjarmason wrote:

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

Trying to think of counter-arguments, the best I could come up with is
that the situation is potentially ambiguous, and some user could be
confused by us doing the wrong thing. I don't find that particularly
compelling, especially as the "wrong thing" is to list tags, which is
not a destructive operation.

So I think this is an improvement. As for the patch itself:

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

I was about to complain that this needs "!cmdmode", but I just looked
forward in the thread and saw that Junio already covered that in more
detail. I concur.

Thanks for working on this.

-Peff