{"thread":{"id":"45342","subject":"BUG: \"git branch --contains \" does nothing, silently fails","startedAt":"2017-03-10T10:43:55Z","lastAt":"2017-03-12T11:20:22Z","messageCount":11,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"313742","messageId":"CACBZZX7OSH_dSJ+kswbv2g1Hv6JieDxHHRmqazppgih0d1+hVg@mail.gmail.com","threadId":"45342","inReplyTo":null,"subject":"BUG: \"git branch --contains \" does nothing, silently fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-10T10:43:15Z","receivedAt":"2017-03-10T10:43:55Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ran into this when preparing my --no-contains series, this is a long\nstanding bug:\n\n $ ./git branch -D test; ./git branch --contains v2.8.0 test; echo\n$?; git rev-parse test\n error: branch 'test' not found.\n 0\n test\n fatal: ambiguous argument 'test': unknown revision or path not in\nthe working tree.\n\nI.e. this should return an error like \"git tag\" does:\n\n $ ./git tag -d test; ./git tag --contains v2.8.0 test; echo $?;\ngit rev-parse test\n error: tag 'test' not found.\n fatal: --contains option is only allowed with -l.\n 128\n test\n fatal: ambiguous argument 'test': unknown revision or path not in\nthe working tree.\n\nI briefly looked through builtin/branch.c, couldn't find a trivial way\nto patch it, unfamiliar with the code, didn't want to forget about it,\nhence this E-Mail.\n"},{"id":"313747","messageId":"20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net","threadId":"45342","inReplyTo":"CACBZZX7OSH_dSJ+kswbv2g1Hv6JieDxHHRmqazppgih0d1+hVg@mail.gmail.com","subject":"Re: BUG: \"git branch --contains \" does nothing, silently fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T12:42:48Z","receivedAt":"2017-03-10T12:43:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 11:43:15AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Ran into this when preparing my --no-contains series, this is a long\n> standing bug:\n> \n> $ ./git branch -D test; ./git branch --contains v2.8.0 test; echo\n> $?; git rev-parse test\n> error: branch 'test' not found.\n> 0\n> test\n> fatal: ambiguous argument 'test': unknown revision or path not in\n> the working tree.\n\nIsn't that asking to list branches starting with \"test\" that contain\nv2.8.0? There are none to report (since you just made sure to delete\nit), so the empty output is correct.\n\n> I.e. this should return an error like \"git tag\" does:\n> \n> $ ./git tag -d test; ./git tag --contains v2.8.0 test; echo $?;\n> git rev-parse test\n> error: tag 'test' not found.\n> fatal: --contains option is only allowed with -l.\n> 128\n> test\n> fatal: ambiguous argument 'test': unknown revision or path not in\n> the working tree.\n\nThe difference between \"branch\" and \"tag\" here is that \"branch\n--contains\" implies \"--list\" (and the argument becomes a pattern).\nWhereas \"tag --contains\" just detects the situation and complains.\n\nIf anything, I'd think we would consider teaching \"tag\" to behave more\nlike \"branch\".\n\n-Peff\n"},{"id":"313748","messageId":"CACBZZX7Npivd9bSsA2_D1Xgk9Qjru00KbeNFRgNQUJ5_NBh3bA@mail.gmail.com","threadId":"45342","inReplyTo":"20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net","subject":"Re: BUG: \"git branch --contains \" does nothing, silently fails","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-10T13:23:06Z","receivedAt":"2017-03-10T13:23:36Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 10, 2017 at 1:42 PM, Jeff King wrote:\n> On Fri, Mar 10, 2017 at 11:43:15AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Ran into this when preparing my --no-contains series, this is a long\n>> standing bug:\n>>\n>> $ ./git branch -D test; ./git branch --contains v2.8.0 test; echo\n>> $?; git rev-parse test\n>> error: branch 'test' not found.\n>> 0\n>> test\n>> fatal: ambiguous argument 'test': unknown revision or path not in\n>> the working tree.\n>\n> Isn't that asking to list branches starting with \"test\" that contain\n> v2.8.0? There are none to report (since you just made sure to delete\n> it), so the empty output is correct.\n>\n>> I.e. this should return an error like \"git tag\" does:\n>>\n>> $ ./git tag -d test; ./git tag --contains v2.8.0 test; echo $?;\n>> git rev-parse test\n>> error: tag 'test' not found.\n>> fatal: --contains option is only allowed with -l.\n>> 128\n>> test\n>> fatal: ambiguous argument 'test': unknown revision or path not in\n>> the working tree.\n>\n> The difference between \"branch\" and \"tag\" here is that \"branch\n> --contains\" implies \"--list\" (and the argument becomes a pattern).\n> Whereas \"tag --contains\" just detects the situation and complains.\n>\n> If anything, I'd think we would consider teaching \"tag\" to behave more\n> like \"branch\".\n\nYes you're right, sorry about the noise, e.g. this works:\n\n git branch --contains v2.8.0 'a*'\n git branch --contains v2.8.0 --list 'a*'\n\nWhereas with \"git tag\" you always need -l as you point out.\n"},{"id":"313822","messageId":"20170311120855.16756-1-avarab@gmail.com","threadId":"45342","inReplyTo":"20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net","subject":"[PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-11T12:08:55Z","receivedAt":"2017-03-11T12:09:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change these invocations which currently error out without the -l, to\nbehave as if though -l was provided:\n\n git tag -l [--contains|--points-at|--[no-]merged] \n\nI ran into what turned out to be not-a-bug in \"branch\" where it,\nunlike \"tag\" before this patch, accepts input like:\n\n git branch --contains v2.8.0 \n\nJeff King pointed out in\n<20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to\nthat::\n\n The difference between \"branch\" and \"tag\" here is that \"branch\n --contains\" implies \"--list\" (and the argument becomes a pattern).\n Whereas \"tag --contains\" just detects the situation and complains.\n\n If anything, I'd think we would consider teaching \"tag\" to behave\n more like \"branch\".\n\nI agree. This change does that, the only tests that broke as a result\nof this were tests that were explicitly checking that this\n\"branch-like\" usage wasn't permitted, i.e. no actual breakages\noccurred, and I can't imagine an invocation that would negatively\nimpact backwards compatibility, i.e. these invocations all just\nerrored out before.\n\nSpelunking through the history via:\n\n git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'\n\nReveals that there was no good reason for this in the first place. The\n--contains option added in v1.6.1.1-243-g32c35cfb1e made this an\nerror, and all the other subsequent options I'm changing here just\ncopy/pasted its pattern.\n\nI've changed the failing tests to check that this invocation mode is\npermitted instead, and added extra tests for the list-like options we\nweren't testing.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nJunio: This will merge conflict with my in-flight --no-contains\npatch. I can re-send either one depending on which you want to accept\nfirst, this patch will need an additional test for --no-contains. I\njust wanted to get this on the ML for review before the --no-contains\npatch hit \"master\".\n\n Documentation/git-tag.txt | 3 +++\n builtin/tag.c | 12 ++++++------\n t/t7004-tag.sh | 25 +++++++++++++++++++++----\n 3 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 525737a5d8..c80d9e11ba 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -94,6 +94,9 @@ OPTIONS\n \tlists all tags. The pattern is a shell wildcard (i.e., matched\n \tusing fnmatch(3)). Multiple patterns may be given; if any of\n \tthem matches, the tag is shown.\n++\n+We supply this option implicitly if any other list-like option is\n+provided. E.g. `--contains`, `--points-at` etc.\n \n --sort=::\n \tSort based on the key given. Prefix `-` to sort in\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ad29be6923..6ab65bcf6b 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tcreate_tag_object = (opt.sign || annotate || msg.given || msgfile);\n \n+\t/* We implicitly supply --list with --contains, --points-at,\n+\t --merged and --no-merged, just like git-branch */\n+\tif (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n+\t\tcmdmode = 'l';\n+\n+\t/* Just plain \"git tag\" is like \"git tag --list\" */\n \tif (argc == 0 && !cmdmode)\n \t\tcmdmode = 'l';\n \n@@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (filter.lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (filter.with_commit)\n-\t\tdie(_(\"--contains option is only allowed with -l.\"));\n-\tif (filter.points_at.nr)\n-\t\tdie(_(\"--points-at option is only allowed with -l.\"));\n-\tif (filter.merge_commit)\n-\t\tdie(_(\"--merged and --no-merged option are only allowed with -l\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\n \tif (cmdmode == 'v') {\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f5..e0306ee5a8 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1453,6 +1453,11 @@ test_expect_success 'checking that initial commit is in all tags' \"\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'checking that --contains can be used in non-list mode' '\n+\tgit tag --contains $hash1 v* >actual &&\n+\ttest_cmp expected actual\n+'\n+\n # mixing modes and options:\n \n test_expect_success 'mixing incompatibles modes and options is forbidden' '\n@@ -1466,8 +1471,10 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \n # check points-at\n \n-test_expect_success '--points-at cannot be used in non-list mode' '\n-\ttest_must_fail git tag --points-at=v4.0 foo\n+test_expect_success '--points-at can be used in non-list mode' '\n+\techo v4.0 >expect &&\n+\tgit tag --points-at=v4.0 \"v*\" >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--points-at finds lightweight tags' '\n@@ -1744,8 +1751,13 @@ test_expect_success 'setup --merged test tags' '\n \tgit tag mergetest-3 HEAD\n '\n \n-test_expect_success '--merged cannot be used in non-list mode' '\n-\ttest_must_fail git tag --merged=mergetest-2 foo\n+test_expect_success '--merged can be used in non-list mode' '\n+\tcat >expect <<-\\EOF &&\n+\tmergetest-1\n+\tmergetest-2\n+\tEOF\n+\tgit tag --merged=mergetest-2 \"mergetest*\" >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--merged shows merged tags' '\n@@ -1765,6 +1777,11 @@ test_expect_success '--no-merged show unmerged tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--no-merged can be used in non-list mode' '\n+\tgit tag --no-merged=mergetest-2 mergetest-* >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'ambiguous branch/tags not marked' '\n \tgit tag ambiguous &&\n \tgit branch ambiguous &&\n-- \n2.11.0\n\n"},{"id":"313823","messageId":"CACBZZX4AJqvtZQdZZFU8+1So5G9F11_SN4=za+8p=KYim9SFSQ@mail.gmail.com","threadId":"45342","inReplyTo":"20170311120855.16756-1-avarab@gmail.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-11T12:14:48Z","receivedAt":"2017-03-11T12:15:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 11, 2017 at 1:08 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Change these invocations which currently error out without the -l, to\n> behave as if though -l was provided:\n>\n> git tag -l [--contains|--points-at|--[no-]merged] \n\nOops, this should be:\n\n git tag -l [--contains|--points-at|--[no-]merged] \n\nWill fix in v2 pending other comments.\n\n> I ran into what turned out to be not-a-bug in \"branch\" where it,\n> unlike \"tag\" before this patch, accepts input like:\n>\n> git branch --contains v2.8.0 \n>\n> Jeff King pointed out in\n> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to\n> that::\n>\n> The difference between \"branch\" and \"tag\" here is that \"branch\n> --contains\" implies \"--list\" (and the argument becomes a pattern).\n> Whereas \"tag --contains\" just detects the situation and complains.\n>\n> If anything, I'd think we would consider teaching \"tag\" to behave\n> more like \"branch\".\n>\n> I agree. This change does that, the only tests that broke as a result\n> of this were tests that were explicitly checking that this\n> \"branch-like\" usage wasn't permitted, i.e. no actual breakages\n> occurred, and I can't imagine an invocation that would negatively\n> impact backwards compatibility, i.e. these invocations all just\n> errored out before.\n>\n> Spelunking through the history via:\n>\n> git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'\n>\n> Reveals that there was no good reason for this in the first place. The\n> --contains option added in v1.6.1.1-243-g32c35cfb1e made this an\n> error, and all the other subsequent options I'm changing here just\n> copy/pasted its pattern.\n>\n> I've changed the failing tests to check that this invocation mode is\n> permitted instead, and added extra tests for the list-like options we\n> weren't testing.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> Junio: This will merge conflict with my in-flight --no-contains\n> patch. I can re-send either one depending on which you want to accept\n> first, this patch will need an additional test for --no-contains. I\n> just wanted to get this on the ML for review before the --no-contains\n> patch hit \"master\".\n>\n> Documentation/git-tag.txt | 3 +++\n> builtin/tag.c | 12 ++++++------\n> t/t7004-tag.sh | 25 +++++++++++++++++++++----\n> 3 files changed, 30 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 525737a5d8..c80d9e11ba 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -94,6 +94,9 @@ OPTIONS\n> lists all tags. The pattern is a shell wildcard (i.e., matched\n> using fnmatch(3)). Multiple patterns may be given; if any of\n> them matches, the tag is shown.\n> ++\n> +We supply this option implicitly if any other list-like option is\n> +provided. E.g. `--contains`, `--points-at` etc.\n\nThe \"this option\" I'm referring to is --list, this is a patch to the\n--list section in \"git help tag\".\n\n>\n> --sort=::\n> Sort based on the key given. Prefix `-` to sort in\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index ad29be6923..6ab65bcf6b 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> }\n> create_tag_object = (opt.sign || annotate || msg.given || msgfile);\n>\n> + /* We implicitly supply --list with --contains, --points-at,\n> + --merged and --no-merged, just like git-branch */\n> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n> + cmdmode = 'l';\n> +\n> + /* Just plain \"git tag\" is like \"git tag --list\" */\n> if (argc == 0 && !cmdmode)\n> cmdmode = 'l';\n>\n> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> }\n> if (filter.lines != -1)\n> die(_(\"-n option is only allowed with -l.\"));\n> - if (filter.with_commit)\n> - die(_(\"--contains option is only allowed with -l.\"));\n> - if (filter.points_at.nr)\n> - die(_(\"--points-at option is only allowed with -l.\"));\n> - if (filter.merge_commit)\n> - die(_(\"--merged and --no-merged option are only allowed with -l\"));\n> if (cmdmode == 'd')\n> return for_each_tag_name(argv, delete_tag, NULL);\n> if (cmdmode == 'v') {\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index b4698ab5f5..e0306ee5a8 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1453,6 +1453,11 @@ test_expect_success 'checking that initial commit is in all tags' \"\n> test_cmp expected actual\n> \"\n>\n> +test_expect_success 'checking that --contains can be used in non-list mode' '\n> + git tag --contains $hash1 v* >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> # mixing modes and options:\n>\n> test_expect_success 'mixing incompatibles modes and options is forbidden' '\n> @@ -1466,8 +1471,10 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n>\n> # check points-at\n>\n> -test_expect_success '--points-at cannot be used in non-list mode' '\n> - test_must_fail git tag --points-at=v4.0 foo\n> +test_expect_success '--points-at can be used in non-list mode' '\n> + echo v4.0 >expect &&\n> + git tag --points-at=v4.0 \"v*\" >actual &&\n> + test_cmp expect actual\n> '\n>\n> test_expect_success '--points-at finds lightweight tags' '\n> @@ -1744,8 +1751,13 @@ test_expect_success 'setup --merged test tags' '\n> git tag mergetest-3 HEAD\n> '\n>\n> -test_expect_success '--merged cannot be used in non-list mode' '\n> - test_must_fail git tag --merged=mergetest-2 foo\n> +test_expect_success '--merged can be used in non-list mode' '\n> + cat >expect <<-\\EOF &&\n> + mergetest-1\n> + mergetest-2\n> + EOF\n> + git tag --merged=mergetest-2 \"mergetest*\" >actual &&\n> + test_cmp expect actual\n> '\n>\n> test_expect_success '--merged shows merged tags' '\n> @@ -1765,6 +1777,11 @@ test_expect_success '--no-merged show unmerged tags' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success '--no-merged can be used in non-list mode' '\n> + git tag --no-merged=mergetest-2 mergetest-* >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_expect_success 'ambiguous branch/tags not marked' '\n> git tag ambiguous &&\n> git branch ambiguous &&\n> --\n> 2.11.0\n>\n"},{"id":"313839","messageId":"xmqqefy3w6k0.fsf@gitster.mtv.corp.google.com","threadId":"45342","inReplyTo":"CACBZZX4AJqvtZQdZZFU8+1So5G9F11_SN4=za+8p=KYim9SFSQ@mail.gmail.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T02:51:59Z","receivedAt":"2017-03-12T02:52:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Junio: This will merge conflict with my in-flight --no-contains\n>> patch. I can re-send either one depending on which you want to accept\n>> first, this patch will need an additional test for --no-contains. I\n>> just wanted to get this on the ML for review before the --no-contains\n>> patch hit \"master\".\n\nI haven't looked at the patch text of this one closely yet, but I\nthink the goals of both make sense, so we would eventually want to\nhave them both.\n\nI also think that \"if you said --contains, --merged, etc. you are\nalready asking to give you a list and cannot be creating a new one\",\nwhich is the topic of this patch, makes sense even if nobody were\ninterested in asking \"--no-contains\".\n\nSo perhaps you would want this applied first, so that existing three\ncan already benefit from \"implicit --list\" before waiting for the\nother one?\n\n"},{"id":"313840","messageId":"xmqqa88rw656.fsf@gitster.mtv.corp.google.com","threadId":"45342","inReplyTo":"xmqqefy3w6k0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T03:00:53Z","receivedAt":"2017-03-12T03:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I haven't looked at the patch text of this one closely yet, but I\n> think the goals of both make sense, so we would eventually want to\n> have them both.\n> ...\n> So perhaps you would want this applied first, so that existing three\n> can already benefit from \"implicit --list\" before waiting for the\n> other one?\n\nAh, I see that you already have the other topic polished to v4\n(sorry, but I am a bit behind the list traffic). If it makes your\nlife easier, it is OK to make the current three to four by adding\n\"--no-contains\" first and then make all/any of them imply \"--list\"\non top. \n\nIOW, either is fine. Personally, I have a suspicion that sending\nboth at the same time, expecting the maintainer to be able to\nresolve the potential conflicts correctly, would also be fine ;-)\n\nThanks.\n"},{"id":"313841","messageId":"xmqq4lyzw5az.fsf@gitster.mtv.corp.google.com","threadId":"45342","inReplyTo":"20170311120855.16756-1-avarab@gmail.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T03:19:00Z","receivedAt":"2017-03-12T03:19:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change these invocations which currently error out without the -l, to\n> behave as if though -l was provided:\n>\n> git tag -l [--contains|--points-at|--[no-]merged] \n\nShouldn't this be\n\n\tgit tag -l [[--[no-]contains|--points-at|--[no-]merged]\t] []\n\ni.e. if you are giving you need how that commit is used in\nfiltering, but you do not have to give any such filter when listing,\nand , when given, is used to further limit the output, but\nit also is optional.\n\n> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option\n\ns/Implicit/implicit/ (ask \"git shortlog --no-merges\" over recent history)\n\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 525737a5d8..c80d9e11ba 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -94,6 +94,9 @@ OPTIONS\n> \tlists all tags. The pattern is a shell wildcard (i.e., matched\n> \tusing fnmatch(3)). Multiple patterns may be given; if any of\n> \tthem matches, the tag is shown.\n> ++\n> +We supply this option implicitly if any other list-like option is\n> +provided. E.g. `--contains`, `--points-at` etc.\n\nWho are \"we\"? \n\n\tWhen any option that only makes sense in the list mode\n\t(e.g. `--contains`) is given, the command defaults to\n\tthe `--list` mode.\n\nBy the way, do we catch it as a command line error when options like\n`--points-at` are given when we are creating a new tag?\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index ad29be6923..6ab65bcf6b 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t}\n> \tcreate_tag_object = (opt.sign || annotate || msg.given || msgfile);\n> \n> +\t/* We implicitly supply --list with --contains, --points-at,\n> +\t --merged and --no-merged, just like git-branch */\n\n\t/*\n * We write multi-line comments like this,\n\t * without anything other than slash-asterisk or\n\t * asterisk-slash on the first and last lines.\n\t */\n\n> +\tif (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n> +\t\tcmdmode = 'l';\n\nDon't we want to make sure we do the defaulting only upon !cmdmode?\nDoesn't this start ignoring\n\n\ttag -a -m foo --points-at HEAD bar\n\nas an error otherwise?\n\n> +\t/* Just plain \"git tag\" is like \"git tag --list\" */\n> \tif (argc == 0 && !cmdmode)\n> \t\tcmdmode = 'l';\n\n> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t}\n> \tif (filter.lines != -1)\n> \t\tdie(_(\"-n option is only allowed with -l.\"));\n> -\tif (filter.with_commit)\n> -\t\tdie(_(\"--contains option is only allowed with -l.\"));\n> -\tif (filter.points_at.nr)\n> -\t\tdie(_(\"--points-at option is only allowed with -l.\"));\n> -\tif (filter.merge_commit)\n> -\t\tdie(_(\"--merged and --no-merged option are only allowed with -l\"));\n\nAnd I do not think removal of these check is a good idea at all.\nPerhaps you were too focused on '-l' that you forgot that people may\nbe giving an explicit option like -a or -s, in which case these\nerror checks are still very sensible things to have, no?\n"},{"id":"313848","messageId":"CACBZZX52UYdGa1--wAz+beHNCMST4NoopJvFg68hLDaeaq=DNQ@mail.gmail.com","threadId":"45342","inReplyTo":"xmqqefy3w6k0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-12T09:15:02Z","receivedAt":"2017-03-12T09:15:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Mar 12, 2017 at 3:51 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> Junio: This will merge conflict with my in-flight --no-contains\n>>> patch. I can re-send either one depending on which you want to accept\n>>> first, this patch will need an additional test for --no-contains. I\n>>> just wanted to get this on the ML for review before the --no-contains\n>>> patch hit \"master\".\n>\n> I haven't looked at the patch text of this one closely yet, but I\n> think the goals of both make sense, so we would eventually want to\n> have them both.\n>\n> I also think that \"if you said --contains, --merged, etc. you are\n> already asking to give you a list and cannot be creating a new one\",\n> which is the topic of this patch, makes sense even if nobody were\n> interested in asking \"--no-contains\".\n>\n> So perhaps you would want this applied first, so that existing three\n> can already benefit from \"implicit --list\" before waiting for the\n> other one?\n\nYes, let's do this one first. I'll address the comments that have come\nup & just make this all part of one series on top of JK's patches.\n"},{"id":"313849","messageId":"CACBZZX6pvjn8_BGhK8+3LyJpVWSjE9utvHuYzGLC5YZeDj5ykg@mail.gmail.com","threadId":"45342","inReplyTo":"xmqq4lyzw5az.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-12T09:15:33Z","receivedAt":"2017-03-12T09:16:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Mar 12, 2017 at 4:19 AM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change these invocations which currently error out without the -l, to\n>> behave as if though -l was provided:\n>>\n>> git tag -l [--contains|--points-at|--[no-]merged] \n>\n> Shouldn't this be\n>\n> git tag -l [[--[no-]contains|--points-at|--[no-]merged] ] []\n>\n> i.e. if you are giving you need how that commit is used in\n> filtering, but you do not have to give any such filter when listing,\n> and , when given, is used to further limit the output, but\n> it also is optional.\n>\n>> Subject: Re: [PATCH] tag: Implicitly supply --list given another list-like option\n>\n> s/Implicit/implicit/ (ask \"git shortlog --no-merges\" over recent history)\n>\n>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n>> index 525737a5d8..c80d9e11ba 100644\n>> --- a/Documentation/git-tag.txt\n>> +++ b/Documentation/git-tag.txt\n>> @@ -94,6 +94,9 @@ OPTIONS\n>> lists all tags. The pattern is a shell wildcard (i.e., matched\n>> using fnmatch(3)). Multiple patterns may be given; if any of\n>> them matches, the tag is shown.\n>> ++\n>> +We supply this option implicitly if any other list-like option is\n>> +provided. E.g. `--contains`, `--points-at` etc.\n>\n> Who are \"we\"?\n>\n> When any option that only makes sense in the list mode\n> (e.g. `--contains`) is given, the command defaults to\n> the `--list` mode.\n>\n> By the way, do we catch it as a command line error when options like\n> `--points-at` are given when we are creating a new tag?\n>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index ad29be6923..6ab65bcf6b 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>> }\n>> create_tag_object = (opt.sign || annotate || msg.given || msgfile);\n>>\n>> + /* We implicitly supply --list with --contains, --points-at,\n>> + --merged and --no-merged, just like git-branch */\n>\n> /*\n> * We write multi-line comments like this,\n> * without anything other than slash-asterisk or\n> * asterisk-slash on the first and last lines.\n> */\n>\n>> + if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n>> + cmdmode = 'l';\n>\n> Don't we want to make sure we do the defaulting only upon !cmdmode?\n> Doesn't this start ignoring\n>\n> tag -a -m foo --points-at HEAD bar\n>\n> as an error otherwise?\n>\n>> + /* Just plain \"git tag\" is like \"git tag --list\" */\n>> if (argc == 0 && !cmdmode)\n>> cmdmode = 'l';\n>\n>> @@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>> }\n>> if (filter.lines != -1)\n>> die(_(\"-n option is only allowed with -l.\"));\n>> - if (filter.with_commit)\n>> - die(_(\"--contains option is only allowed with -l.\"));\n>> - if (filter.points_at.nr)\n>> - die(_(\"--points-at option is only allowed with -l.\"));\n>> - if (filter.merge_commit)\n>> - die(_(\"--merged and --no-merged option are only allowed with -l\"));\n>\n> And I do not think removal of these check is a good idea at all.\n> Perhaps you were too focused on '-l' that you forgot that people may\n> be giving an explicit option like -a or -s, in which case these\n> error checks are still very sensible things to have, no?\n\nI'll fix this up and make sure to do more sanity checks with the\ndifferent option combinations before resending.\n"},{"id":"313851","messageId":"20170312111952.ryhkkrgldub26qky@sigill.intra.peff.net","threadId":"45342","inReplyTo":"20170311120855.16756-1-avarab@gmail.com","subject":"Re: [PATCH] tag: Implicitly supply --list given another list-like option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T11:19:52Z","receivedAt":"2017-03-12T11:20:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 11, 2017 at 12:08:55PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Jeff King pointed out in\n> <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to\n> that::\n> \n> The difference between \"branch\" and \"tag\" here is that \"branch\n> --contains\" implies \"--list\" (and the argument becomes a pattern).\n> Whereas \"tag --contains\" just detects the situation and complains.\n> \n> If anything, I'd think we would consider teaching \"tag\" to behave\n> more like \"branch\".\n> \n> I agree. This change does that, the only tests that broke as a result\n> of this were tests that were explicitly checking that this\n> \"branch-like\" usage wasn't permitted, i.e. no actual breakages\n> occurred, and I can't imagine an invocation that would negatively\n> impact backwards compatibility, i.e. these invocations all just\n> errored out before.\n\nTrying to think of counter-arguments, the best I could come up with is\nthat the situation is potentially ambiguous, and some user could be\nconfused by us doing the wrong thing. I don't find that particularly\ncompelling, especially as the \"wrong thing\" is to list tags, which is\nnot a destructive operation.\n\nSo I think this is an improvement. As for the patch itself:\n\n> +\t/* We implicitly supply --list with --contains, --points-at,\n> +\t --merged and --no-merged, just like git-branch */\n> +\tif (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n> +\t\tcmdmode = 'l';\n\nI was about to complain that this needs \"!cmdmode\", but I just looked\nforward in the thread and saw that Junio already covered that in more\ndetail. I concur.\n\nThanks for working on this.\n\n-Peff\n"}]}