{"thread":{"id":"45419","subject":"[PATCH 1/8] tag: Remove a TODO item from the test suite","startedAt":"2017-03-18T10:34:25Z","lastAt":"2017-03-20T20:06:24Z","messageCount":41,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jakub Narębski","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"314483","messageId":"20170318103256.27141-2-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 1/8] tag: Remove a TODO item from the test suite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:49Z","receivedAt":"2017-03-18T10:34:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the test for \"git tag -l\" to not have an associated TODO\ncomment saying that it should return non-zero if there's no tags.\n\nThis was added in commit ef5a6fb597 (\"Add test-script for git-tag\",\n2007-06-28) when the tests for \"tag\" were initially added, but at this\npoint changing this would be inconsistent with how \"git tag\" is a\nsynonym for \"git tag -l\", and would needlessly break external code\nthat relies on this porcelain command.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7004-tag.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b4698ab5f5..876ccfc830 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -16,7 +16,6 @@ tag_exists () {\n \tgit show-ref --quiet --verify refs/tags/\"$1\"\n }\n \n-# todo: git tag -l now returns always zero, when fixed, change this test\n test_expect_success 'listing all tags in an empty tree should succeed' '\n \tgit tag -l &&\n \tgit tag\n@@ -136,7 +135,6 @@ test_expect_success \\\n \t'listing a tag using a matching pattern should output that tag' \\\n \t'test $(git tag -l mytag) = mytag'\n \n-# todo: git tag -l now returns always zero, when fixed, change this test\n test_expect_success \\\n \t'listing tags using a non-matching pattern should suceed' \\\n \t'git tag -l xxx'\n-- \n2.11.0\n\n"},{"id":"314484","messageId":"20170318103256.27141-6-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 5/8] tag: Implicitly supply --list given the -n option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:53Z","receivedAt":"2017-03-18T10:34:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"tag\" command to treat the \"-n\" invocation as a list-like\noption in addition to --contains, --points-at etc.\n\nMost of the work for this was done in my preceding \"tag: Implicitly\nsupply --list given another list-like option\", but I've split off this\npatch since it's more contentious. Now invocations these invocations\nwill be synonymous:\n\n git tag -n 100\n git tag -n --list 100\n\nWhereas before the former would die. This doesn't technically\nintroduce any more ambiguity than change to the other list-like\noptions, but it does introduce the possibility for more confusion\nsince instead of the latter of these dying:\n\n git tag -n100\n git tag -n 100\n\nIt now works entirely differently, i.e. invokes list mode with a\nfilter for \"100\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-tag.txt | 9 +++++----\n builtin/tag.c | 2 +-\n t/t7004-tag.sh | 17 ++++++++++++++++-\n 3 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 2acd3b6beb..e7793afad1 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -82,10 +82,11 @@ OPTIONS\n \n -n::\n \t specifies how many lines from the annotation, if any,\n-\tare printed when using -l.\n-\tThe default is not to print any annotation lines.\n-\tIf no number is given to `-n`, only the first line is printed.\n-\tIf the tag is not annotated, the commit message is displayed instead.\n+\tare printed when using -l. Implies `--list`.\n++\n+The default is not to print any annotation lines.\n+If no number is given to `-n`, only the first line is printed.\n+If the tag is not annotated, the commit message is displayed instead.\n \n -l::\n --list::\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 3483636e59..2da28a5ce6 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -457,7 +457,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (!cmdmode && !create_tag_object) {\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n-\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n+\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 5c94932f0f..ba1ab1f21c 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -639,6 +639,11 @@ test_expect_success \\\n \tgit tag -n0 -l tag-one-line >actual &&\n \ttest_cmp expect actual &&\n \n+\tgit tag -n0 | grep \"^tag-one-line\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit tag -n0 tag-one-line >actual &&\n+\ttest_cmp expect actual &&\n+\n \techo \"tag-one-line A msg\" >expect &&\n \tgit tag -n1 -l | grep \"^tag-one-line\" >actual &&\n \ttest_cmp expect actual &&\n@@ -652,6 +657,17 @@ test_expect_success \\\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'The -n 100 invocation means -n --list 100, not -n100' '\n+\t>expect &&\n+\tgit tag -n 100 >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit tag -m \"A msg\" 100 &&\n+\techo \"100 A msg\" >expect &&\n+\tgit tag -n 100 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success \\\n \t'listing the zero-lines message of a non-signed tag should succeed' '\n \tgit tag -m \"\" tag-zero-lines &&\n@@ -1495,7 +1511,6 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -a -s -m -F &&\n \ttest_must_fail git tag -a -s -m -F -l &&\n \ttest_must_fail git tag -l -v &&\n-\ttest_must_fail git tag -n 100 &&\n \ttest_must_fail git tag -n 100 -v &&\n \ttest_must_fail git tag -l -m msg &&\n \ttest_must_fail git tag -l -F some file &&\n-- \n2.11.0\n\n"},{"id":"314485","messageId":"20170318103256.27141-3-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 2/8] tag: Refactor the options handling code to be less bizarro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:50Z","receivedAt":"2017-03-18T10:34:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the options handling code so that \"cmdmode = 'l'\" isn't set\non the likes of \"tag -a\". This change introduces no functional\nchanges, but makes subsequent patches easier to reason about, as both\n\"annotate = 1\" \"cmdmode = 'l'\" won't be unexpectedly set if \"tag\" is\nmerely invoked as \"tag -a\", \"tag -s\" etc.\n\nBefore this the cmdmode variable would be set to 'l' indicating that\n\"tag\" was operating in list mode, this was then used a couple of lines\nlater by e.g. the codepath for \"tag -a\" where the command would only\nbail out with usage_with_options() if the likes of \"-a\" were\nset, *and* some cmdmode (i.e. 'l' in this case) had been set.\n\nThis behavior was an emergent property of other earlier changes,\nstarting with commit 6fa8342b12 (tag: Check that options are only\nallowed in the appropriate mode, 2008-11-05), to its present form in\ncommit e6b722db09 (\"tag: use OPT_CMDMODE\", 2013-07-30).\n\nChange the test suite to more exhaustively assert that already\nexisting behavior related to this option parsing is kept.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/tag.c | 4 ++--\n t/t7004-tag.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ad29be6923..0bba3fd070 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -454,10 +454,10 @@ 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-\tif (argc == 0 && !cmdmode)\n+\tif (argc == 0 && !cmdmode && !create_tag_object)\n \t\tcmdmode = 'l';\n \n-\tif ((create_tag_object || force) && (cmdmode != 0))\n+\tif ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))\n \t\tusage_with_options(git_tag_usage, options);\n \n \tfinalize_colopts(&colopts, -1);\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 876ccfc830..74fc82a3c0 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1455,6 +1455,19 @@ test_expect_success 'checking that initial commit is in all tags' \"\n \n test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -a &&\n+\ttest_must_fail git tag -a -l &&\n+\ttest_must_fail git tag -s &&\n+\ttest_must_fail git tag -s -l &&\n+\ttest_must_fail git tag -m &&\n+\ttest_must_fail git tag -m -l &&\n+\ttest_must_fail git tag -m \"hlagh\" &&\n+\ttest_must_fail git tag -m \"hlagh\" -l &&\n+\ttest_must_fail git tag -F &&\n+\ttest_must_fail git tag -F -l &&\n+\ttest_must_fail git tag -f &&\n+\ttest_must_fail git tag -f -l &&\n+\ttest_must_fail git tag -a -s -m -F &&\n+\ttest_must_fail git tag -a -s -m -F -l &&\n \ttest_must_fail git tag -l -v &&\n \ttest_must_fail git tag -n 100 &&\n \ttest_must_fail git tag -l -m msg &&\n-- \n2.11.0\n\n"},{"id":"314487","messageId":"20170318103256.27141-8-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 7/8] tag: Add tests for --with and --without","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:55Z","receivedAt":"2017-03-18T10:34:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the test suite to test for these options. Before this change\nthere were no tests for this at all. This change doesn't exhaustively\ntest for them as well as their --contains and --no-contains aliases,\nbut at least it's something.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t7004-tag.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 428e21c369..f7ff4e034b 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1592,7 +1592,7 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag --contains --no-contains\n '\n \n-for option in --contains --no-contains --merged --no-merged --points-at\n+for option in --contains --with --no-contains --without --merged --no-merged --points-at\n do\n \ttest_expect_success \"mixing incompatible modes with $option is forbidden\" \"\n \t\ttest_must_fail git tag -d $option HEAD &&\n-- \n2.11.0\n\n"},{"id":"314488","messageId":"20170318103256.27141-9-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 8/8] tag: Change --point-at to default to HEAD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:56Z","receivedAt":"2017-03-18T10:34:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the --points-at option to default to HEAD for consistency with\nits siblings --contains, --merged etc. which default to HEAD. This\nchanges behavior added in commit ae7706b9ac (tag: add --points-at list\noption, 2012-02-08).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-tag.txt | 3 ++-\n builtin/tag.c | 3 ++-\n t/t7004-tag.sh | 8 +++++++-\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex d0b506f120..1d66348b6a 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -136,7 +136,8 @@ This option is only applicable when listing tags without annotation lines.\n \tnot specified). Implies `--list`.\n \n --points-at ::\n-\tOnly list tags of the given object. Implies `--list`.\n+\tOnly list tags of the given object (HEAD if not\n+\tspecified). Implies `--list`.\n \n -m ::\n --message=::\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex f91ae171b7..300f831fb1 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -433,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n-\t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n+\t\t\tN_(\"print only tags of the object\"), PARSE_OPT_LASTARG_DEFAULT,\n+\t\t\tparse_opt_object_name, (intptr_t) \"HEAD\"\n \t\t},\n \t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex f7ff4e034b..112d96b4ce 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1601,7 +1601,8 @@ do\n \t\"\n \ttest_expect_success \"Doing 'git tag --list-like $option is permitted\" \"\n \t\tgit tag -n $option HEAD HEAD &&\n-\t\tgit tag $option HEAD HEAD\n+\t\tgit tag $option HEAD HEAD &&\n+\t\tgit tag $option\n \t\"\n done\n \n@@ -1613,6 +1614,11 @@ test_expect_success '--points-at can be used in non-list mode' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--points-at is a synonym for --points-at HEAD' '\n+\tgit tag --points-at >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--points-at finds lightweight tags' '\n \techo v4.0 >expect &&\n \tgit tag --points-at v4.0 >actual &&\n-- \n2.11.0\n\n"},{"id":"314489","messageId":"20170318103256.27141-4-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 3/8] tag: Change misleading --list documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:51Z","receivedAt":"2017-03-18T10:34:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the documentation for --list so that it's described as a\ntoggle, not as an option that takes a as an argument.\n\nJunio initially documented this in b867c7c23a (\"git-tag: -l to list\ntags (usability).\", 2006-02-17), but later Jeff King changed \"tag\" to\naccept multiple patterns in 588d0e834b (\"tag: accept multiple patterns\nfor --list\", 2011-06-20).\n\nHowever, documenting this as \"-l \" was never correct, as\nthese both worked before Jeff's change:\n\n git tag -l 'v*'\n git tag 'v*' -l\n\nOne would expect an option that was documented like that to only\naccept:\n\n git tag --list\n git tag --list 'v*rc*'\n\nAnd after Jeff's change, one that took multiple patterns:\n\n git tag --list 'v*rc*' --list '*2.8*'\n\nBut since it's actually a toggle all of these work as well, and\nproduce identical output to the last example above:\n\n git tag --list 'v*rc*' '*2.8*'\n git tag --list 'v*rc*' '*2.8*' --list --list --list\n git tag --list 'v*rc*' '*2.8*' --list -l --list -l --list\n\nNow the documentation is more in tune with how the \"branch\" command\ndescribes its --list option since commit cddd127b9a (\"branch:\nintroduce --list option\", 2011-08-28).\n\nChange the test suite to assert that these invocations work for the\ncases that weren't already being tested for.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-tag.txt | 16 +++++++++-------\n t/t7004-tag.sh | 21 +++++++++++++++++++++\n 2 files changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 525737a5d8..848e8c1b73 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -87,13 +87,15 @@ OPTIONS\n \tIf no number is given to `-n`, only the first line is printed.\n \tIf the tag is not annotated, the commit message is displayed instead.\n \n--l ::\n---list ::\n-\tList tags with names that match the given pattern (or all if no\n-\tpattern is given). Running \"git tag\" without arguments also\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+-l::\n+--list::\n+\tActivate the list mode. `git tag ` would try to create a\n+\ttag, use `git tag --list ` to list matching branches, (or\n+\tall if no pattern is given).\n++\n+Running \"git tag\" without arguments also lists all tags. The pattern\n+is a shell wildcard (i.e., matched using fnmatch(3)). Multiple\n+patterns may be given; if any of them matches, the tag is shown.\n \n --sort=::\n \tSort based on the key given. Prefix `-` to sort in\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 74fc82a3c0..d36cd51fe2 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '\n \tgit tag\n '\n \n+cat >expect <actual &&\n+\ttest_cmp expect actual &&\n+\tgit tag --list --list >actual &&\n+\ttest_cmp expect actual &&\n+\tgit tag --list -l --list >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'listing all tags if one exists should output that tag' '\n \ttest $(git tag -l) = mytag &&\n \ttest $(git tag) = mytag\n@@ -336,6 +348,15 @@ test_expect_success 'tag -l can accept multiple patterns' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '\n+\tgit tag -l \"v1*\" \"v0*\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit tag -l \"v1*\" --list \"v0*\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit tag -l \"v1*\" \"v0*\" -l --list >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'listing tags in column' '\n \tCOLUMNS=40 git tag -l --column=row >actual &&\n \tcat >expected <<\\EOF &&\n-- \n2.11.0\n\n"},{"id":"314490","messageId":"20170318103256.27141-5-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 4/8] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:52Z","receivedAt":"2017-03-18T10:34:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"tag\" command to implicitly turn on its --list mode when\nprovided with a list-like option such as --contains, --points-at etc.\n\nThis is for consistency with how \"branch\" works. When \"branch\" is\ngiven a list-like option, such as --contains, it implicitly provides\n--list. Before this change \"tag\" would error out on those sorts of\ninvocations. I.e. while both of these worked for \"branch\":\n\n git branch --contains v2.8.0 \n git branch --list --contains v2.8.0 \n\nOnly the latter form worked for \"tag\":\n\n git tag --contains v2.8.0 '*rc*'\n git tag --list --contains v2.8.0 '*rc*'\n\nNow \"tag\", like \"branch\" will implicitly supply --list in when a\nlist-like option is provided, and no other conflicting non-list\noptions (such as -d) are present on the command-line.\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 not allowing this in the\nfirst place. The --contains option added in 32c35cfb1e (\"git-tag: Add\n--contains option\", 2009-01-26) made this an error, and all the other\nsubsequent list-like options that were added copied its pattern of\nmaking this usage an error.\n\nThe only tests that break as a result of this change are tests that\nwere explicitly checking that this \"branch-like\" usage wasn't\npermitted. Change those failing tests to check that this invocation\nmode is permitted, add extra tests for the list-like options we\nweren't testing, and add tests to ensure that e.g. we don't toggle the\n--list in the presence of other conflicting non-list options.\n\nWith this change errors messages such as \"--contains option is only\nallowed with -l\" don't make sense anymore, since options like\n--contain turn on -l. Instead we error out when list-like options such\nas --contain are used in conjunction with conflicting options such as\n-d or -v.\n\nThis change does not consider \"-n\" a list-like option, even though\nthat might be logical. Permitting it would allow:\n\n git tag -n 100\n\nAs a synonym for:\n\n git tag -n --list 100\n\nWhich, while not technically ambiguous as the option must already be\nprovided as -n rather than -n , would be confusing.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-tag.txt | 10 +++++++---\n builtin/tag.c | 25 +++++++++++++++----------\n t/t7004-tag.sh | 39 +++++++++++++++++++++++++++++++++++----\n 3 files changed, 57 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 848e8c1b73..2acd3b6beb 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -96,6 +96,10 @@ OPTIONS\n Running \"git tag\" without arguments also lists all tags. The pattern\n is a shell wildcard (i.e., matched using fnmatch(3)). Multiple\n patterns may be given; if any of them matches, the tag is shown.\n++\n+This option is implicitly supplied if any other list-like option such\n+as `--contains` is provided. See the documentation for each of those\n+options for details.\n \n --sort=::\n \tSort based on the key given. Prefix `-` to sort in\n@@ -124,10 +128,10 @@ This option is only applicable when listing tags without annotation lines.\n \n --contains []::\n \tOnly list tags which contain the specified commit (HEAD if not\n-\tspecified).\n+\tspecified). Implies `--list`.\n \n --points-at ::\n-\tOnly list tags of the given object.\n+\tOnly list tags of the given object. Implies `--list`.\n \n -m ::\n --message=::\n@@ -178,7 +182,7 @@ This option is only applicable when listing tags without annotation lines.\n --[no-]merged []::\n \tOnly list tags whose tips are reachable, or not reachable\n \tif `--no-merged` is used, from the specified commit (`HEAD`\n-\tif not specified).\n+\tif not specified). Implies `--list`.\n \n CONFIGURATION\n -------------\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 0bba3fd070..3483636e59 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -454,8 +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-\tif (argc == 0 && !cmdmode && !create_tag_object)\n-\t\tcmdmode = 'l';\n+\tif (!cmdmode && !create_tag_object) {\n+\t\tif (argc == 0)\n+\t\t\tcmdmode = 'l';\n+\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n+\t\t\tcmdmode = 'l';\n+\t}\n \n \tif ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))\n \t\tusage_with_options(git_tag_usage, options);\n@@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n \t\treturn ret;\n+\t} else {\n+\t\tif (filter.lines != -1)\n+\t\t\tdie(_(\"-n option is only allowed in list mode.\"));\n+\t\tif (filter.with_commit)\n+\t\t\tdie(_(\"--contains option is only allowed in list mode.\"));\n+\t\tif (filter.points_at.nr)\n+\t\t\tdie(_(\"--points-at option is only allowed in list mode.\"));\n+\t\tif (filter.merge_commit)\n+\t\t\tdie(_(\"--merged and --no-merged options are only allowed in list mode.\"));\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 d36cd51fe2..5c94932f0f 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1472,6 +1472,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@@ -1491,15 +1496,31 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -a -s -m -F -l &&\n \ttest_must_fail git tag -l -v &&\n \ttest_must_fail git tag -n 100 &&\n+\ttest_must_fail git tag -n 100 -v &&\n \ttest_must_fail git tag -l -m msg &&\n \ttest_must_fail git tag -l -F some file &&\n \ttest_must_fail git tag -v -s\n '\n \n+for option in --contains --merged --no-merged --points-at\n+do\n+\ttest_expect_success \"mixing incompatible modes with $option is forbidden\" \"\n+\t\ttest_must_fail git tag -d $option HEAD &&\n+\t\ttest_must_fail git tag -d $option HEAD some-tag &&\n+\t\ttest_must_fail git tag -v $option HEAD\n+\t\"\n+\ttest_expect_success \"Doing 'git tag --list-like $option is permitted\" \"\n+\t\tgit tag -n $option HEAD HEAD &&\n+\t\tgit tag $option HEAD HEAD\n+\t\"\n+done\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@@ -1776,8 +1797,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@@ -1797,6 +1823,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":"314491","messageId":"20170318103256.27141-7-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"[PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:54Z","receivedAt":"2017-03-18T10:34:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the tag, branch & for-each-ref commands to have a --no-contains\noption in addition to their longstanding --contains options.\n\nThis allows for finding the last-good rollout tag given a known-bad\n. Given a hypothetically bad commit cf5c7253e0 the git version\nrevert to can be found with this hacky two-liner:\n\n (git tag -l 'v[0-9]*'; git tag -l --contains cf5c7253e0 'v[0-9]*') |\n sort | uniq -c | grep -E '^ *1 ' | awk '{print $2}' | tail -n 10\n\nWith this new --no-contains the same can be achieved with:\n\n git tag -l --no-contains cf5c7253e0 'v[0-9]*' | sort | tail -n 10\n\nAs the filtering machinery is shared between the tag, branch &\nfor-each-ref commands, implement this for those commands too. A\npractical use for this with \"branch\" is e.g. finding branches which\ndiverged between v2.8 & v2.10:\n\n git branch --contains v2.8.0 --no-contains v2.10.0\n\nThe \"describe\" command also has a --contains option, but its semantics\nare unrelated to what tag/branch/for-each-ref use --contains for. A\n--no-contains option for \"describe\" wouldn't make any sense, other\nthan being exactly equivalent to not supplying --contains at all,\nwhich would be confusing at best.\n\nAdd a --without option to \"tag\" as an alias for --no-contains, for\nconsistency with --with and --contains. The --with option is\nundocumented, and possibly the only user of it is\nJunio (). But it's\ntrivial to support, so let's do that.\n\nThe additions to the the test suite are inverse copies of the\ncorresponding --contains tests. With this change --no-contains for\ntag, branch & for-each-ref is just as well tested as the existing\n--contains option.\n\nIn addition to those tests, add a test for \"tag\" which asserts that\n--no-contains won't find tree/blob tags, which is slightly\nunintuitive, but consistent with how --contains works & is documented.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-branch.txt | 15 ++--\n Documentation/git-for-each-ref.txt | 6 +-\n Documentation/git-tag.txt | 6 +-\n builtin/branch.c | 4 +-\n builtin/for-each-ref.c | 3 +-\n builtin/tag.c | 8 ++-\n contrib/completion/git-completion.bash | 4 +-\n parse-options.h | 4 +-\n ref-filter.c | 19 +++--\n ref-filter.h | 1 +\n t/t3201-branch-contains.sh | 51 ++++++++++++-\n t/t6302-for-each-ref-filter.sh | 16 +++++\n t/t7004-tag.sh | 128 +++++++++++++++++++++++++++++++--\n 13 files changed, 240 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 092f1bcf9f..5e52fc9b91 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git branch' [--color[=] | --no-color] [-r | -a]\n \t[--list] [-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column]\n-\t[(--merged | --no-merged | --contains) []] [--sort=]\n+\t[(--merged | --no-merged | --contains | --no-contains) []] [--sort=]\n \t[--points-at ] [--format=] [...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n 'git branch' (--set-upstream-to= | -u ) []\n@@ -35,7 +35,7 @@ as branch creation.\n \n With `--contains`, shows only the branches that contain the named commit\n (in other words, the branches whose tip commits are descendants of the\n-named commit). With `--merged`, only branches merged into the named\n+named commit), `--no-contains` inverts it. With `--merged`, only branches merged into the named\n commit (i.e. the branches whose tip commits are reachable from the named\n commit) will be listed. With `--no-merged` only branches not merged into\n the named commit will be listed. If the argument is missing it\n@@ -213,6 +213,10 @@ start-point is either a local or remote-tracking branch.\n \tOnly list branches which contain the specified commit (HEAD\n \tif not specified). Implies `--list`.\n \n+--no-contains []::\n+\tOnly list branches which don't contain the specified commit\n+\t(HEAD if not specified). Implies `--list`.\n+\n --merged []::\n \tOnly list branches whose tips are reachable from the\n \tspecified commit (HEAD if not specified). Implies `--list`.\n@@ -296,13 +300,16 @@ If you are creating a branch that you want to checkout immediately, it is\n easier to use the git checkout command with its `-b` option to create\n a branch and check it out with a single command.\n \n-The options `--contains`, `--merged` and `--no-merged` serve three related\n-but different purposes:\n+The options `--contains`, `--no-contains`, `--merged` and `--no-merged`\n+serve four related but different purposes:\n \n - `--contains ` is used to find all branches which will need\n special attention if were to be rebased or amended, since those\n branches contain the specified .\n \n+- `--no-contains ` is the inverse of that, i.e. branches that don't\n+ contain the specified .\n+\n - `--merged` is used to find all branches which can be safely deleted,\n since those branches are fully contained by HEAD.\n \ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 111e1be6f5..83b93c75a8 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl]\n \t\t [(--sort=)...] [--format=] [...]\n \t\t [--points-at ] [(--merged | --no-merged) []]\n-\t\t [--contains []]\n+\t\t [(--contains | --no-contains) []]\n \n DESCRIPTION\n -----------\n@@ -79,6 +79,10 @@ OPTIONS\n \tOnly list refs which contain the specified commit (HEAD if not\n \tspecified).\n \n+--no-contains []::\n+\tOnly list refs which don't contain the specified commit (HEAD\n+\tif not specified).\n+\n --ignore-case::\n \tSorting and filtering refs are case insensitive.\n \ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex e7793afad1..d0b506f120 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git tag' [-a | -s | -u ] [-f] [-m | -F ]\n \t [ | ]\n 'git tag' -d ...\n-'git tag' [-n[]] -l [--contains ] [--points-at ]\n+'git tag' [-n[]] -l [--[no-]contains ] [--points-at ]\n \t[--column[=] | --no-column] [--create-reflog] [--sort=]\n \t[--format=] [--[no-]merged []] [...]\n 'git tag' -v [--format=] ...\n@@ -131,6 +131,10 @@ This option is only applicable when listing tags without annotation lines.\n \tOnly list tags which contain the specified commit (HEAD if not\n \tspecified). Implies `--list`.\n \n+--no-contains []::\n+\tOnly list tags which don't contain the specified commit (HEAD if\n+\tnot specified). Implies `--list`.\n+\n --points-at ::\n \tOnly list tags of the given object. Implies `--list`.\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 94f7de7fa5..dd96319726 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -548,7 +548,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT('r', \"remotes\", &filter.kind, N_(\"act on remote-tracking branches\"),\n \t\t\tFILTER_REFS_REMOTES),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n+\t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only branches that don't contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n+\t\tOPT_WITHOUT(&filter.no_commit, N_(\"print only branches that don't contain the commit\")),\n \t\tOPT__ABBREV(&filter.abbrev),\n \n \t\tOPT_GROUP(N_(\"Specific git-branch actions:\")),\n@@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n+\tif (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!new_upstream +\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex df41fa0350..a11542c4fd 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {\n \tN_(\"git for-each-ref [] []\"),\n \tN_(\"git for-each-ref [--points-at ]\"),\n \tN_(\"git for-each-ref [(--merged | --no-merged) []]\"),\n-\tN_(\"git for-each-ref [--contains []]\"),\n+\tN_(\"git for-each-ref [(--contains | --no-contains) []]\"),\n \tNULL\n };\n \n@@ -43,6 +43,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_MERGED(&filter, N_(\"print only refs that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only refs that are not merged\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only refs which contain the commit\")),\n+\t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only refs which don't contain the commit\")),\n \t\tOPT_BOOL(0, \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n \t\tOPT_END(),\n \t};\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 2da28a5ce6..f91ae171b7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -22,7 +22,7 @@\n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ] []\"),\n \tN_(\"git tag -d ...\"),\n-\tN_(\"git tag -l [-n[]] [--contains ] [--points-at ]\"\n+\tN_(\"git tag -l [-n[]] [--[no-]contains ] [--points-at ]\"\n \t\t\"\\n\\t\\t[--format=] [--[no-]merged []] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n@@ -424,7 +424,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(N_(\"Tag listing options\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_WITHOUT(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n \t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n@@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (!cmdmode && !create_tag_object) {\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n-\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n+\t\telse if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \n@@ -492,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"-n option is only allowed in list mode.\"));\n \t\tif (filter.with_commit)\n \t\t\tdie(_(\"--contains option is only allowed in list mode.\"));\n+\t\tif (filter.no_commit)\n+\t\t\tdie(_(\"--no-contains option is only allowed in list mode.\"));\n \t\tif (filter.points_at.nr)\n \t\t\tdie(_(\"--points-at option is only allowed in list mode.\"));\n \t\tif (filter.merge_commit)\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex fc32286a43..ec8fce5820 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1093,7 +1093,7 @@ _git_branch ()\n \t--*)\n \t\t__gitcomp \"\n \t\t\t--color --no-color --verbose --abbrev= --no-abbrev\n-\t\t\t--track --no-track --contains --merged --no-merged\n+\t\t\t--track --no-track --contains --no-contains --merged --no-merged\n \t\t\t--set-upstream-to= --edit-description --list\n \t\t\t--unset-upstream --delete --move --remotes\n \t\t\t--column --no-column --sort= --points-at\n@@ -2862,7 +2862,7 @@ _git_tag ()\n \t\t__gitcomp \"\n \t\t\t--list --delete --verify --annotate --message --file\n \t\t\t--sign --cleanup --local-user --force --column --sort=\n-\t\t\t--contains --points-at --merged --no-merged --create-reflog\n+\t\t\t--contains --no-contains --points-at --merged --no-merged --create-reflog\n \t\t\t\"\n \t\t;;\n \tesac\ndiff --git a/parse-options.h b/parse-options.h\nindex dcd8a0926c..0eac90b510 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -258,7 +258,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);\n \t PARSE_OPT_LASTARG_DEFAULT | flag, \\\n \t parse_opt_commits, (intptr_t) \"HEAD\" \\\n \t}\n-#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"contains\", v, h, 0)\n+#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"contains\", v, h, PARSE_OPT_NONEG)\n+#define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"no-contains\", v, h, PARSE_OPT_NONEG)\n #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH(\"with\", v, h, PARSE_OPT_HIDDEN)\n+#define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH(\"without\", v, h, PARSE_OPT_HIDDEN)\n \n #endif\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7eeecc608f..092331fd81 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1487,6 +1487,7 @@ struct ref_filter_cbdata {\n \tstruct ref_array *array;\n \tstruct ref_filter *filter;\n \tstruct contains_cache contains_cache;\n+\tstruct contains_cache no_contains_cache;\n };\n \n /*\n@@ -1586,11 +1587,11 @@ static enum contains_result contains_tag_algo(struct commit *candidate,\n }\n \n static int commit_contains(struct ref_filter *filter, struct commit *commit,\n-\t\t\t struct contains_cache *cache)\n+\t\t\t struct commit_list *list, struct contains_cache *cache)\n {\n \tif (filter->with_commit_tag_algo)\n-\t\treturn contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;\n-\treturn is_descendant_of(commit, filter->with_commit);\n+\t\treturn contains_tag_algo(commit, list, cache) == CONTAINS_YES;\n+\treturn is_descendant_of(commit, list);\n }\n \n /*\n@@ -1780,13 +1781,17 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit || filter->verbose) {\n+\tif (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {\n \t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n-\t\t/* We perform the filtering for the '--contains' option */\n+\t\t/* We perform the filtering for the '--contains' option... */\n \t\tif (filter->with_commit &&\n-\t\t !commit_contains(filter, commit, &ref_cbdata->contains_cache))\n+\t\t !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))\n+\t\t\treturn 0;\n+\t\t/* ...or for the `--no-contains' option */\n+\t\tif (filter->no_commit &&\n+\t\t commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))\n \t\t\treturn 0;\n \t}\n \n@@ -1887,6 +1892,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tfilter->kind = type & FILTER_REFS_KIND_MASK;\n \n \tinit_contains_cache(&ref_cbdata.contains_cache);\n+\tinit_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Simple per-ref filtering */\n \tif (!filter->kind)\n@@ -1911,6 +1917,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t}\n \n \tclear_contains_cache(&ref_cbdata.contains_cache);\n+\tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n \tif (filter->merge_commit)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex e738c5dfd3..dde40f6849 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -53,6 +53,7 @@ struct ref_filter {\n \tconst char **name_patterns;\n \tstruct sha1_array points_at;\n \tstruct commit_list *with_commit;\n+\tstruct commit_list *no_commit;\n \n \tenum {\n \t\tREF_FILTER_MERGED_NONE = 0,\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 7f3ec47241..506415dbd3 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='branch --contains , --merged, and --no-merged'\n+test_description='branch --contains , --no-contains --merged, and --no-merged'\n \n . ./test-lib.sh\n \n@@ -45,6 +45,22 @@ test_expect_success 'branch --contains master' '\n \n '\n \n+test_expect_success 'branch --no-contains=master' '\n+\n+\tgit branch --no-contains=master >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+\n+'\n+\n+test_expect_success 'branch --no-contains master' '\n+\n+\tgit branch --no-contains master >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'branch --contains=side' '\n \n \tgit branch --contains=side >actual &&\n@@ -55,6 +71,16 @@ test_expect_success 'branch --contains=side' '\n \n '\n \n+test_expect_success 'branch --no-contains=side' '\n+\n+\tgit branch --no-contains=side >actual &&\n+\t{\n+\t\techo \" master\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'branch --contains with pattern implies --list' '\n \n \tgit branch --contains=master master >actual &&\n@@ -65,6 +91,14 @@ test_expect_success 'branch --contains with pattern implies --list' '\n \n '\n \n+test_expect_success 'branch --no-contains with pattern implies --list' '\n+\n+\tgit branch --no-contains=master master >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_expect_success 'side: branch --merged' '\n \n \tgit branch --merged >actual &&\n@@ -126,7 +160,9 @@ test_expect_success 'branch --no-merged with pattern implies --list' '\n test_expect_success 'implicit --list conflicts with modification options' '\n \n \ttest_must_fail git branch --contains=master -d &&\n-\ttest_must_fail git branch --contains=master -m foo\n+\ttest_must_fail git branch --contains=master -m foo &&\n+\ttest_must_fail git branch --no-contains=master -d &&\n+\ttest_must_fail git branch --no-contains=master -m foo\n \n '\n \n@@ -159,4 +195,15 @@ test_expect_success 'branch --merged with --verbose' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'branch --contains combined with --no-contains' '\n+\tgit branch --contains zzz --no-contains topic >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t master\n+\t side\n+\t zzz\n+\tEOF\n+\ttest_cmp expect actual\n+\n+'\n+\n test_done\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex a09a1a46ef..4902ba5f16 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -93,6 +93,22 @@ test_expect_success 'filtering with --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'filtering with --no-contains' '\n+\tcat >expect <<-\\EOF &&\n+\trefs/tags/one\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname)\" --no-contains=two >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'filtering with --contains and --no-contains' '\n+\tcat >expect <<-\\EOF &&\n+\trefs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname)\" --contains=two --no-contains=three >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex ba1ab1f21c..428e21c369 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1420,6 +1420,23 @@ test_expect_success 'checking that first commit is in all tags (relative)' \"\n \ttest_cmp expected actual\n \"\n \n+# All the --contains tests above, but with --no-contains\n+test_expect_success 'checking that first commit is not listed in any tag with --no-contains (hash)' \"\n+\t>expected &&\n+\tgit tag -l --no-contains $hash1 v* >actual &&\n+\ttest_cmp expected actual\n+\"\n+\n+test_expect_success 'checking that first commit is in all tags (tag)' \"\n+\tgit tag -l --no-contains v1.0 v* >actual &&\n+\ttest_cmp expected actual\n+\"\n+\n+test_expect_success 'checking that first commit is in all tags (relative)' \"\n+\tgit tag -l --no-contains HEAD~2 v* >actual &&\n+\ttest_cmp expected actual\n+\"\n+\n cat > expected < expected <actual &&\n+\ttest_cmp expected actual\n+\"\n \n cat > expected < expected <actual &&\n+\ttest_cmp expected actual\n+\"\n+\n # how about a simple merge?\n \n test_expect_success 'creating simple branch' '\n@@ -1459,6 +1500,19 @@ test_expect_success 'checking that branch head only has one tag' \"\n \ttest_cmp expected actual\n \"\n \n+cat > expected <actual &&\n+\ttest_cmp expected actual\n+\"\n+\n test_expect_success 'merging original branch into this branch' '\n \tgit merge --strategy=ours master &&\n git tag v4.0\n@@ -1480,6 +1534,20 @@ v1.0.1\n v1.1.3\n v2.0\n v3.0\n+EOF\n+\n+test_expect_success 'checking that original branch head with --no-contains lists all but one tag now' \"\n+\tgit tag -l --no-contains $hash3 v* >actual &&\n+\ttest_cmp expected actual\n+\"\n+\n+cat > expected <expected &&\n+\tgit tag -l --no-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@@ -1514,10 +1588,11 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -n 100 -v &&\n \ttest_must_fail git tag -l -m msg &&\n \ttest_must_fail git tag -l -F some file &&\n-\ttest_must_fail git tag -v -s\n+\ttest_must_fail git tag -v -s &&\n+\ttest_must_fail git tag --contains --no-contains\n '\n \n-for option in --contains --merged --no-merged --points-at\n+for option in --contains --no-contains --merged --no-merged --points-at\n do\n \ttest_expect_success \"mixing incompatible modes with $option is forbidden\" \"\n \t\ttest_must_fail git tag -d $option HEAD &&\n@@ -1777,7 +1852,7 @@ run_with_limited_stack () {\n test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'\n \n # we require ulimit, this excludes Windows\n-test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '\n+test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a deep repo' '\n \t>expect &&\n \ti=1 &&\n \twhile test $i -lt 8000\n@@ -1793,7 +1868,9 @@ EOF\"\n \tgit checkout master &&\n \tgit tag far-far-away HEAD^ &&\n \trun_with_limited_stack git tag --contains HEAD >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\trun_with_limited_stack git tag --no-contains HEAD >actual &&\n+\ttest_line_count \">\" 10 actual\n '\n \n test_expect_success '--format should list tags as per format given' '\n@@ -1851,4 +1928,47 @@ test_expect_success 'ambiguous branch/tags not marked' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--contains combined with --no-contains' '\n+\t(\n+\t\tgit init no-contains &&\n+\t\tcd no-contains &&\n+\t\ttest_commit v0.1 &&\n+\t\ttest_commit v0.2 &&\n+\t\ttest_commit v0.3 &&\n+\t\ttest_commit v0.4 &&\n+\t\ttest_commit v0.5 &&\n+\t\tcat >expected <<-\\EOF &&\n+\t\tv0.2\n+\t\tv0.3\n+\t\tv0.4\n+\t\tEOF\n+\t\tgit tag --contains v0.2 --no-contains v0.5 >actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+# As the docs say, list tags which contain a specified *commit*. We\n+# don't recurse down to tags for trees or blobs pointed to by *those*\n+# commits.\n+test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n+\tcd no-contains &&\n+\tblob=$(git rev-parse v0.3:v0.3.t) &&\n+\ttree=$(git rev-parse v0.3^{tree}) &&\n+\tgit tag tag-blob $blob &&\n+\tgit tag tag-tree $tree &&\n+\tgit tag --contains v0.3 >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tv0.3\n+\tv0.4\n+\tv0.5\n+\tEOF\n+\ttest_cmp expected actual &&\n+\tgit tag --no-contains v0.3 >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tv0.1\n+\tv0.2\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.11.0\n\n"},{"id":"314495","messageId":"20170318103256.27141-1-avarab@gmail.com","threadId":"45419","inReplyTo":null,"subject":"[PATCH 0/8] Various changes to the \"tag\" command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T10:32:48Z","receivedAt":"2017-03-18T11:33:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series incorporates and replaces all the \"tag\" patches I have\nfloating around the list, and adds a lot in the mix which discovered\nwhile working on the initial two patches, but which made sense as\nseparate patches.\n\nIt's based no top of Jeff's gitster/jk/ref-filter-flags-cleanup.\n\nI'm bundling this all together because a lot of these patches touch\nthe exact same code, and in other cases subsequent patches make use of\ntest suite improvements I made earlier in the series. So although some\ncould be split out entirely (e.g. the --point-at patch), I'd like to\njust present them all for review together & split out any if there's\nstrong objections to basing them on top of each other.\n\nI think this series changes addresses all the points brought up by\nJunio/Jeff about my previous patches, except there's no extensive\ndiscussion of how the filtering mechanism works in general as pointed\nout by Junio in .\n\nI think it makes sense to have that, but in the interest of getting\nsomething out the door I'm not working on that for now.\n\nÆvar Arnfjörð Bjarmason (8):\n tag: Remove a TODO item from the test suite\n tag: Refactor the options handling code to be less bizarro\n tag: Change misleading --list documentation\n tag: Implicitly supply --list given another list-like option\n tag: Implicitly supply --list given the -n option\n ref-filter: Add --no-contains option to tag/branch/for-each-ref\n tag: Add tests for --with and --without\n tag: Change --point-at to default to HEAD\n\n Documentation/git-branch.txt | 15 ++-\n Documentation/git-for-each-ref.txt | 6 +-\n Documentation/git-tag.txt | 44 ++++---\n builtin/branch.c | 4 +-\n builtin/for-each-ref.c | 3 +-\n builtin/tag.c | 36 ++++--\n contrib/completion/git-completion.bash | 4 +-\n parse-options.h | 4 +-\n ref-filter.c | 19 ++-\n ref-filter.h | 1 +\n t/t3201-branch-contains.sh | 51 +++++++-\n t/t6302-for-each-ref-filter.sh | 16 +++\n t/t7004-tag.sh | 226 +++++++++++++++++++++++++++++++--\n 13 files changed, 371 insertions(+), 58 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"314518","messageId":"xmqqvar6310x.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318103256.27141-2-avarab@gmail.com","subject":"Re: [PATCH 1/8] tag: Remove a TODO item from the test suite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T18:14:38Z","receivedAt":"2017-03-18T18:14:46Z","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 the test for \"git tag -l\" to not have an associated TODO\n> comment saying that it should return non-zero if there's no tags.\n>\n> This was added in commit ef5a6fb597 (\"Add test-script for git-tag\",\n> 2007-06-28) when the tests for \"tag\" were initially added, but at this\n> point changing this would be inconsistent with how \"git tag\" is a\n> synonym for \"git tag -l\", and would needlessly break external code\n> that relies on this porcelain command.\n\nMakes sense.\n\nI'll retitle s/Remove/remove/ so that \"git shortlog --no-merges\"\nwould look more consistent, though.\n\nThanks.\n"},{"id":"314520","messageId":"xmqqr31u301u.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318103256.27141-3-avarab@gmail.com","subject":"Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T18:35:41Z","receivedAt":"2017-03-18T18:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index ad29be6923..0bba3fd070 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -454,10 +454,10 @@ 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> -\tif (argc == 0 && !cmdmode)\n> +\tif (argc == 0 && !cmdmode && !create_tag_object)\n> \t\tcmdmode = 'l';\n\nSo with this change, if we cannot infer that we are creating a tag\nobject from other options, we leave cmdmode to its original 0.\n\n> -\tif ((create_tag_object || force) && (cmdmode != 0))\n> +\tif ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))\n> \t\tusage_with_options(git_tag_usage, options);\n\nAnd then immediately after that, we complain by detecting that we\nknow we are creating a tag and a non-zero cmdmode is in effect\n(i.e. 'l', 'd' or 'v', none of which is about creating a tag). The\nway we used to detect that we are doing something other than tag\ncreation was by seeing cmdmode is set to anything. Because of your\nearlier change, that no longer is true. You need to separately\ncheck (!cmdmode && !argc).\n\nBy following the logic that way, I can see how this change at this\nstep is a no-op, but I have to say that the code with this patch\nlooks much more bizarre than the original.\n\nI am not sure why you want to do the first change at this step in\nthe first place. Is it because you'd want to take over (!cmdmode &&\n!argc) condtion to default to 'list'? With the change in 4/8 and\n5/8, you are ensuring that cmdmode is set to 'l' for these new cases\nbefore the code hits the check to call usage_with_options(). And at\nthat point, you can use the original \"are we creating and !cmdmode\nsays we are not? That's contradiction\" logic without making it more\nbizarre with this patch, no?\n"},{"id":"314521","messageId":"20170318184203.16890-1-avarab@gmail.com","threadId":"45419","inReplyTo":"xmqqvar6310x.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] doc/SubmittingPatches: A couple of minor improvements","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T18:42:01Z","receivedAt":"2017-03-18T18:42:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 7:14 PM, Junio C Hamano wrote:\n> I'll retitle s/Remove/remove/ so that \"git shortlog --no-merges\"\n> would look more consistent, though.\n\nI already found a few grammar / phrasing issues with the commit\nmessages, so I'll just change this on my side for a resend.\n\nBut I noticed that this casing rule wasn't documented in\nSubmittingPatches, so here's a patch for that, and while I'm at it\nanother small improvement that I've been meaning to make to it based\non a local alias I have.\n\nÆvar Arnfjörð Bjarmason (2):\n doc/SubmittingPatches: clarify the casing convention for \"area:\n change...\"\n doc/SubmittingPatches: show how to get a CLI commit summary\n\n Documentation/SubmittingPatches | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\n-- \n2.11.0\n\n"},{"id":"314522","messageId":"20170318184203.16890-2-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318184203.16890-1-avarab@gmail.com","subject":"[PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for \"area: change...\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T18:42:02Z","receivedAt":"2017-03-18T18:42:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the section which describes how the first line of the subject\nshould look like to say that the \":\" in \"area: \" shouldn't be treated\nlike a full stop for the purposes of letter casing.\n\nChange the two subject examples to make this new paragraph clearer,\ni.e. \"unstar\" is not a common word, and \"git-cherry-pick.txt\" is a\nmuch longer string than \"githooks.txt\". Pick two recent commits from\ngit.git that fit better for the description.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/SubmittingPatches | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 3faf7eb884..9ef624ce38 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -98,12 +98,17 @@ should skip the full stop. It is also conventional in most cases to\n prefix the first line with \"area: \" where the area is a filename or\n identifier for the general area of the code being modified, e.g.\n \n- . archive: ustar header checksum is computed unsigned\n- . git-cherry-pick.txt: clarify the use of revision range notation\n+ . doc: clarify distinction between sign-off and pgp-signing\n+ . githooks.txt: improve the intro section\n \n If in doubt which identifier to use, run \"git log --no-merges\" on the\n files you are modifying to see the current conventions.\n \n+It's customary to start the remainder of the first line after \"area: \"\n+with a lower-case letter. E.g. \"doc: clarify...\", not \"doc:\n+Clarify...\", or \"githooks.txt: improve...\", not \"githooks.txt:\n+Improve...\".\n+\n The body should provide a meaningful commit message, which:\n \n . explains the problem the change tries to solve, iow, what is wrong\n-- \n2.11.0\n\n"},{"id":"314523","messageId":"20170318184203.16890-3-avarab@gmail.com","threadId":"45419","inReplyTo":"20170318184203.16890-1-avarab@gmail.com","subject":"[PATCH 2/2] doc/SubmittingPatches: show how to get a CLI commit summary","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T18:42:03Z","receivedAt":"2017-03-18T18:42:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Amend the section which describes how to get a commit summary to show\nhow do to that with \"git show\", currently the documentation only shows\nhow to do that with gitk.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/SubmittingPatches | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 9ef624ce38..78c8e36a4b 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -134,8 +134,17 @@ with the subject enclosed in a pair of double-quotes, like this:\n noticed that ...\n \n The \"Copy commit summary\" command of gitk can be used to obtain this\n-format.\n+format, or this invocation of \"git show\":\n \n+ git show -s --date=format:%Y-%m-%d --pretty='commit %h (\"%s\", %ad)' \n+\n+To turn that into a handy alias:\n+\n+ git config --global alias.git-commit-summary \"show -s --date=format:%Y-%m-%d --pretty='commit %h (\\\"%s\\\", %ad)'\"\n+\n+And then to get the commit summary:\n+\n+ git git-commit-summary \n \n (3) Generate your patch using Git tools out of your commits.\n \n-- \n2.11.0\n\n"},{"id":"314525","messageId":"xmqqmvci2zoc.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318103256.27141-4-avarab@gmail.com","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T18:43:47Z","receivedAt":"2017-03-18T18:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> However, documenting this as \"-l \" was never correct, as\n> these both worked before Jeff's change:\n>\n> git tag -l 'v*'\n> git tag 'v*' -l\n\nActually, we do not particularly care about the latter, and quite\nhonestly, I'd prefer we do not advertise and encourage the latter.\nMost Git commands take dashed options first and then non-dashed\narguments, and so should \"git tag\". A more important example to\nshow why \"-l \" that pretends is an argument to\nthe option is wrong is this:\n\n\tgit tag -l --merged X 'v*'\n\nand this one\n\n> git tag --list 'v*rc*' '*2.8*'\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 74fc82a3c0..d36cd51fe2 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '\n> \tgit tag\n> '\n> \n> +cat >expect < +mytag\n> +EOF\n> +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n> +\tgit tag -l -l >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit tag --list --list >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit tag --list -l --list >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nOK. I do not care too deeply about this one, but somebody may want\nto tighten up the command line parsing to detect conflicting or\nduplicated cmdmode as an error in the future, and at that time this\nwill require updating. I am not sure if we want to promise that\ngiving multiple -l will keep working.\n\n> +test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '\n> +\tgit tag -l \"v1*\" \"v0*\" >actual &&\n\nThis is good thing to promise that we will keep it working.\n\n> +\ttest_cmp expect actual &&\n> +\tgit tag -l \"v1*\" --list \"v0*\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit tag -l \"v1*\" \"v0*\" -l --list >actual &&\n> +\ttest_cmp expect actual\n\nI'd prefer we do *not* promise that it will keep working if you give\npattern and then later any dashed-option like -l to the command by\nhaving tests like these.\n\nThanks.\n"},{"id":"314527","messageId":"xmqqinn62z74.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318103256.27141-9-avarab@gmail.com","subject":"Re: [PATCH 8/8] tag: Change --point-at to default to HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T18:54:07Z","receivedAt":"2017-03-18T18:58: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> Change the --points-at option to default to HEAD for consistency with\n> its siblings --contains, --merged etc. which default to HEAD. This\n> changes behavior added in commit ae7706b9ac (tag: add --points-at list\n> option, 2012-02-08).\n\nMakes a lot of sense to me.\n\n> +test_expect_success '--points-at is a synonym for --points-at HEAD' '\n> +\tgit tag --points-at >actual &&\n\nEven if \"expect\" is the same one established earlier, it is easier\nto read and understand individual tests if you explicitly said what\nthis one expects.\n\nThanks.\n"},{"id":"314529","messageId":"xmqq7f3m2yph.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318184203.16890-2-avarab@gmail.com","subject":"Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for \"area: change...\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T19:04:42Z","receivedAt":"2017-03-18T19:05:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> prefix the first line with \"area: \" where the area is a filename or\n> identifier for the general area of the code being modified, e.g.\n> \n> - . archive: ustar header checksum is computed unsigned\n> - . git-cherry-pick.txt: clarify the use of revision range notation\n> + . doc: clarify distinction between sign-off and pgp-signing\n> + . githooks.txt: improve the intro section\n\nSorry, but I fail to spot why this is an improvement (it is not\nmaking things worse, either).\n\n> If in doubt which identifier to use, run \"git log --no-merges\" on the\n> files you are modifying to see the current conventions.\n> \n> +It's customary to start the remainder of the first line after \"area: \"\n> +with a lower-case letter. E.g. \"doc: clarify...\", not \"doc:\n> +Clarify...\", or \"githooks.txt: improve...\", not \"githooks.txt:\n> +Improve...\".\n\n"},{"id":"314530","messageId":"xmqq37ea2ykh.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170318184203.16890-3-avarab@gmail.com","subject":"Re: [PATCH 2/2] doc/SubmittingPatches: show how to get a CLI commit summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T19:07:42Z","receivedAt":"2017-03-18T19:07:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The \"Copy commit summary\" command of gitk can be used to obtain this\n> -format.\n> +format, or this invocation of \"git show\":\n> \n> + git show -s --date=format:%Y-%m-%d --pretty='commit %h (\"%s\", %ad)' \n\nI've seen (I think I stole it from Peff) this one recommended often\non the list, which is shorter:\n\n $ git show --date=short -s --pretty='format:%h (\"%s\", %ad)' \n\n\n\n"},{"id":"314532","messageId":"CACBZZX4Zs=00QXv5eaafQd8-n7OakcL3WeQh8XxiMo-9a+kAzQ@mail.gmail.com","threadId":"45419","inReplyTo":"xmqqr31u301u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T19:13:12Z","receivedAt":"2017-03-18T19:17:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 7:35 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index ad29be6923..0bba3fd070 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>> }\n>> create_tag_object = (opt.sign || annotate || msg.given || msgfile);\n>>\n>> - if (argc == 0 && !cmdmode)\n>> + if (argc == 0 && !cmdmode && !create_tag_object)\n>> cmdmode = 'l';\n>\n> So with this change, if we cannot infer that we are creating a tag\n> object from other options, we leave cmdmode to its original 0.\n>\n>> - if ((create_tag_object || force) && (cmdmode != 0))\n>> + if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))\n>> usage_with_options(git_tag_usage, options);\n>\n> And then immediately after that, we complain by detecting that we\n> know we are creating a tag and a non-zero cmdmode is in effect\n> (i.e. 'l', 'd' or 'v', none of which is about creating a tag). The\n> way we used to detect that we are doing something other than tag\n> creation was by seeing cmdmode is set to anything. Because of your\n> earlier change, that no longer is true. You need to separately\n> check (!cmdmode && !argc).\n>\n> By following the logic that way, I can see how this change at this\n> step is a no-op, but I have to say that the code with this patch\n> looks much more bizarre than the original.\n>\n> I am not sure why you want to do the first change at this step in\n> the first place. Is it because you'd want to take over (!cmdmode &&\n> !argc) condtion to default to 'list'? With the change in 4/8 and\n> 5/8, you are ensuring that cmdmode is set to 'l' for these new cases\n> before the code hits the check to call usage_with_options(). And at\n> that point, you can use the original \"are we creating and !cmdmode\n> says we are not? That's contradiction\" logic without making it more\n> bizarre with this patch, no?\n\nNothing about this patch is needed for the rest of the series. I just\ntried rebasing it out now and all tests pass & everything works as\nexpected.\n\nThe reason I'm submitting it is because while this works *now* and\nthere's no cases I can see currently where cmdmode is 'l' after the\ncurrent `((create_tag_object || force) && (cmdmode != 0))`, during a\nlengthy debugging session when I was hacking on a subsequent patch in\nthis series it took me a long time to track down a segfault later in\nthe file because surely it was impossible that I was in cmdmode = 'l'\nwith only \"git tag -a\".\n\nPartly that was late night hacking session blindness after having read\nthe !argc and assuming that the -a would be counted towards argc.\n\nBut I thought it was very a very bizarre pattern to set us to cmdmode\n= 'l' when we're not in that mode at all just to, as can be seen in\nthe diff, get around a slightly more verbose one-time if-check.\n\nSo I think it's worth it to include this for less confusion in any\nsubsequent patches to tag.c.\n"},{"id":"314533","messageId":"CACBZZX4+HRnb1SV_URqJyxyaeBJ40p_hh_kGEoRj089xJGARHQ@mail.gmail.com","threadId":"45419","inReplyTo":"xmqq7f3m2yph.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for \"area: change...\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T19:16:40Z","receivedAt":"2017-03-18T19:17:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 8:04 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> prefix the first line with \"area: \" where the area is a filename or\n>> identifier for the general area of the code being modified, e.g.\n>>\n>> - . archive: ustar header checksum is computed unsigned\n>> - . git-cherry-pick.txt: clarify the use of revision range notation\n>> + . doc: clarify distinction between sign-off and pgp-signing\n>> + . githooks.txt: improve the intro section\n>\n> Sorry, but I fail to spot why this is an improvement (it is not\n> making things worse, either).\n\nBecause...\n\n>> If in doubt which identifier to use, run \"git log --no-merges\" on the\n>> files you are modifying to see the current conventions.\n>>\n>> +It's customary to start the remainder of the first line after \"area: \"\n>> +with a lower-case letter. E.g. \"doc: clarify...\", not \"doc:\n>> +Clarify...\", or \"githooks.txt: improve...\", not \"githooks.txt:\n>> +Improve...\".\n\n...it makes this subsequent example more succinct and clear, because\ne.g. \"githooks.txt\" is shorter than \"git-cherry-pick.txt\", and\n\"clarify\" is obviously a normal looking word which you'd expect to be\ncapitalized after a full stop, but it might take a couple of readings\nto understand that \"unstar\" without a hyphen isn't some jargon.\n"},{"id":"314537","messageId":"xmqqo9wy1j3s.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"CACBZZX4Zs=00QXv5eaafQd8-n7OakcL3WeQh8XxiMo-9a+kAzQ@mail.gmail.com","subject":"Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T19:27:03Z","receivedAt":"2017-03-18T19:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But I thought it was very a very bizarre pattern to set us to cmdmode\n> = 'l' when we're not in that mode at all just to, as can be seen in\n> the diff, get around a slightly more verbose one-time if-check.\n\nWhen I wrote my response, I viewed that setting as committing to be\nin the \"list\" mode, not as a workaround. So checking with !cmdmode\nto make sure that the command line is not asking to create a tag\nmakes tons of sense; the new test makes it unreadable from that\npoint of view.\n"},{"id":"314539","messageId":"CACBZZX5LN8nhs85K18XVg4Y9_qb9zKGBoFnnQHSsRZVOP=OkDw@mail.gmail.com","threadId":"45419","inReplyTo":"xmqqmvci2zoc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T19:49:19Z","receivedAt":"2017-03-18T19:51:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> However, documenting this as \"-l \" was never correct, as\n>> these both worked before Jeff's change:\n>>\n>> git tag -l 'v*'\n>> git tag 'v*' -l\n>\n> Actually, we do not particularly care about the latter, and quite\n> honestly, I'd prefer we do not advertise and encourage the latter.\n> Most Git commands take dashed options first and then non-dashed\n> arguments, and so should \"git tag\". A more important example to\n> show why \"-l \" that pretends is an argument to\n> the option is wrong is this:\n\nI for one do care about the latter in my CLI use. I.e. I'm fairly used\nto GNU-style getopt parsing where if you type \"ls foo*\" and forget the\n-l you don't have to \"^Mb-l \" to produce \"ls -l foo*\" as you\nwould on the BSD's, you just type \" -l\".\n\nI don't see any reason for why we'd force users to migrate to strict\nBSD-like getopt parsing for commands like tag/branch which accept\nthese form of arguments, although one could argue that it's worth it\nfor consistency with the likes of git-log might have better reasons to\nrequire it.\n\nI.e. are there cases where we encounter genuine ambiguities in our\noption parsing because of this that don't involve the usual suspect of\ne.g. pattern that starts with \"-\" needing \" -- \" to\nresolve the ambiguity?\n\nAs for this patch, I don't think accurately documenting an option like\n--list in terms of what it actually does is advertising and\nencouraging this use. All the examples are still of the form \"git tag\n-l \" not \"git tag -l\".\n\nI think the main point of reference documentation like this should be\nto accurately and exhaustively document what something actually does.\nIf we'd like to change it & deprecate some mode of operation in the\nfuture, fine, but surely the first step towards that is to document\nwhat the command does *now*.\n\nYou should be able to look at a git command, then read the\ndocumentation, and without having run the command or inspected the\ncode be confident that you understand what the command will do when\nyou run it.\n\nRight now that isn't the case with \"tag --list\" at all, because it's\ndocumented as taking a pattern as an argument, but that isn't how it\nworks.\n\n> git tag -l --merged X 'v*'\n>\n> and this one\n>\n>> git tag --list 'v*rc*' '*2.8*'\n>\n>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>> index 74fc82a3c0..d36cd51fe2 100755\n>> --- a/t/t7004-tag.sh\n>> +++ b/t/t7004-tag.sh\n>> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '\n>> git tag\n>> '\n>>\n>> +cat >expect <> +mytag\n>> +EOF\n>> +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n>> + git tag -l -l >actual &&\n>> + test_cmp expect actual &&\n>> + git tag --list --list >actual &&\n>> + test_cmp expect actual &&\n>> + git tag --list -l --list >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> OK. I do not care too deeply about this one, but somebody may want\n> to tighten up the command line parsing to detect conflicting or\n> duplicated cmdmode as an error in the future, and at that time this\n> will require updating. I am not sure if we want to promise that\n> giving multiple -l will keep working.\n>\n>> +test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '\n>> + git tag -l \"v1*\" \"v0*\" >actual &&\n>\n> This is good thing to promise that we will keep it working.\n>\n>> + test_cmp expect actual &&\n>> + git tag -l \"v1*\" --list \"v0*\" >actual &&\n>> + test_cmp expect actual &&\n>> + git tag -l \"v1*\" \"v0*\" -l --list >actual &&\n>> + test_cmp expect actual\n>\n> I'd prefer we do *not* promise that it will keep working if you give\n> pattern and then later any dashed-option like -l to the command by\n> having tests like these.\n\nTo continue the above, I don't agree with this take on the issue at\nall. We should as much as possible aim for full coverage on our tests,\njust because something's tested for doesn't implicitly mean that\nthere's a future promise that the functionality will always work that\nway, it's just testing for both intentional & unintentional\nregressions when the code is changed.\n\nThen if we decide to e.g. change to stricter parsing or BSD-style\nparsing we'll hopefully have an exhaustive list of the cases we're\nchanging.\n\nIt might make sense in cases where we're testing for a feature that\nmight get deprecated in the future to have some test prefix for that,\ni.e. similar to \"test_must_fail\" but \"test_might_get_deprecated\" or\nwhatever.\n\nAlthough that might just as likely turn out to be a useless catalog of\nthings we never actually end up changing, see e.g. that TODO test for\nthe exit code of \"git tag -l\" which I removed at the start of this\nseries.\n"},{"id":"314541","messageId":"CACBZZX5y7ZntOO1RsX74AHoYDJb6pUrjFDbNcamXVJ4Yieeviw@mail.gmail.com","threadId":"45419","inReplyTo":"xmqqinn62z74.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 8/8] tag: Change --point-at to default to HEAD","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T19:52:11Z","receivedAt":"2017-03-18T19:52:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 7:54 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the --points-at option to default to HEAD for consistency with\n>> its siblings --contains, --merged etc. which default to HEAD. This\n>> changes behavior added in commit ae7706b9ac (tag: add --points-at list\n>> option, 2012-02-08).\n>\n> Makes a lot of sense to me.\n>\n>> +test_expect_success '--points-at is a synonym for --points-at HEAD' '\n>> + git tag --points-at >actual &&\n>\n> Even if \"expect\" is the same one established earlier, it is easier\n> to read and understand individual tests if you explicitly said what\n> this one expects.\n\nMakes sense. Queued that change in my WIP v2.\n"},{"id":"314542","messageId":"CACBZZX7G+fj+Fr1ex-vAD47yzvpJXma5YDMBrMoffr_KWFFx7w@mail.gmail.com","threadId":"45419","inReplyTo":"xmqqo9wy1j3s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-18T20:00:50Z","receivedAt":"2017-03-18T20:02:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Mar 18, 2017 at 8:27 PM, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> But I thought it was very a very bizarre pattern to set us to cmdmode\n>> = 'l' when we're not in that mode at all just to, as can be seen in\n>> the diff, get around a slightly more verbose one-time if-check.\n>\n> When I wrote my response, I viewed that setting as committing to be\n> in the \"list\" mode, not as a workaround. So checking with !cmdmode\n> to make sure that the command line is not asking to create a tag\n> makes tons of sense; the new test makes it unreadable from that\n> point of view.\n\nMakes sense. Looking at this more carefully there's never any cases\nwhere `create_tag_object && cmdmode == 'l'` is true once we make it\npast `usage_with_options`, which was the bug I introduced locally\nwhich made this whole thing a bit confusing for me.\n\nI'll drop this patch from v2.\n"},{"id":"314543","messageId":"xmqqbmsy1h7k.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"CACBZZX4+HRnb1SV_URqJyxyaeBJ40p_hh_kGEoRj089xJGARHQ@mail.gmail.com","subject":"Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for \"area: change...\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-18T20:07:59Z","receivedAt":"2017-03-18T20:08:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> ...it makes this subsequent example more succinct and clear, because\n> e.g. \"githooks.txt\" is shorter than \"git-cherry-pick.txt\", and\n> \"clarify\" is obviously a normal looking word...\n\nAh, that makes sense. Thanks.\n\n"},{"id":"314582","messageId":"0dda5b86-2f2b-8f46-98b7-476b6351442b@gmail.com","threadId":"45419","inReplyTo":"20170318103256.27141-2-avarab@gmail.com","subject":"Re: [PATCH 1/8] tag: Remove a TODO item from the test suite","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2017-03-19T00:48:25Z","receivedAt":"2017-03-19T00:58:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 18.03.2017 o 11:32, Ævar Arnfjörð Bjarmason pisze:\n\n> @@ -136,7 +135,6 @@ test_expect_success \\\n> \t'listing a tag using a matching pattern should output that tag' \\\n> \t'test $(git tag -l mytag) = mytag'\n> \n> -# todo: git tag -l now returns always zero, when fixed, change this test\n> test_expect_success \\\n> \t'listing tags using a non-matching pattern should suceed' \\\n> \t'git tag -l xxx'\n\nCould you fix s/suceed/succeed/ in the test description,\nwhile at it?\n\n-- \nJakub Narębski\n\n"},{"id":"314583","messageId":"CACBZZX4fWHKQUVjzbMXXt9vtEGyi4nAqXjf4fGKBCr3yprcu8g@mail.gmail.com","threadId":"45419","inReplyTo":"0dda5b86-2f2b-8f46-98b7-476b6351442b@gmail.com","subject":"Re: [PATCH 1/8] tag: Remove a TODO item from the test suite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-19T06:43:56Z","receivedAt":"2017-03-19T06:46:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Mar 19, 2017 at 1:48 AM, Jakub Narębski wrote:\n> W dniu 18.03.2017 o 11:32, Ævar Arnfjörð Bjarmason pisze:\n>\n>> @@ -136,7 +135,6 @@ test_expect_success \\\n>> 'listing a tag using a matching pattern should output that tag' \\\n>> 'test $(git tag -l mytag) = mytag'\n>>\n>> -# todo: git tag -l now returns always zero, when fixed, change this test\n>> test_expect_success \\\n>> 'listing tags using a non-matching pattern should suceed' \\\n>> 'git tag -l xxx'\n>\n> Could you fix s/suceed/succeed/ in the test description,\n> while at it?\n\nSure, part of v2 now.\n"},{"id":"314619","messageId":"20170320034443.wq2uf4yabsxh2nji@sigill.intra.peff.net","threadId":"45419","inReplyTo":"xmqqmvci2zoc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T03:44:43Z","receivedAt":"2017-03-20T03:44:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 11:43:47AM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n> > +\tgit tag -l -l >actual &&\n> > +\ttest_cmp expect actual &&\n> > +\tgit tag --list --list >actual &&\n> > +\ttest_cmp expect actual &&\n> > +\tgit tag --list -l --list >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> OK. I do not care too deeply about this one, but somebody may want\n> to tighten up the command line parsing to detect conflicting or\n> duplicated cmdmode as an error in the future, and at that time this\n> will require updating. I am not sure if we want to promise that\n> giving multiple -l will keep working.\n\nI think it's expected to work under the usual last-one-wins option\nparsing. A more subtle case is that:\n\n git tag -l -d foo\n\nwould override \"-l\" with \"-d\". That's reasonable under the same rule as\nlong as the user knows that the two are mode-selectors. I don't think we\nmake that explicit in the documentation, though, so perhaps it isn't\nsomething users should rely on.\n\n-Peff\n"},{"id":"314620","messageId":"20170320035549.smzgu2jkaxrhuxl4@sigill.intra.peff.net","threadId":"45419","inReplyTo":"20170318103256.27141-5-avarab@gmail.com","subject":"Re: [PATCH 4/8] tag: Implicitly supply --list given another list-like option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T03:55:49Z","receivedAt":"2017-03-20T03:56:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 10:32:52AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> With this change errors messages such as \"--contains option is only\n> allowed with -l\" don't make sense anymore, since options like\n> --contain turn on -l. Instead we error out when list-like options such\n> as --contain are used in conjunction with conflicting options such as\n> -d or -v.\n\nYeah, I think this is the right approach.\n\n> This change does not consider \"-n\" a list-like option, even though\n> that might be logical. Permitting it would allow:\n> \n> git tag -n 100\n> \n> As a synonym for:\n> \n> git tag -n --list 100\n> \n> Which, while not technically ambiguous as the option must already be\n> provided as -n rather than -n , would be confusing.\n\nI'm not sure the existing behavior isn't confusing anyway (most optional\narguments are). But I don't mind being conservative and leaving out \"-n\"\nfor now; we can always convert it later if somebody feels strongly about\nit.\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 0bba3fd070..3483636e59 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -454,8 +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> -\tif (argc == 0 && !cmdmode && !create_tag_object)\n> -\t\tcmdmode = 'l';\n> +\tif (!cmdmode && !create_tag_object) {\n> +\t\tif (argc == 0)\n> +\t\t\tcmdmode = 'l';\n> +\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n> +\t\t\tcmdmode = 'l';\n> +\t}\n\nMakes sense.\n\n> @@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t\tif (column_active(colopts))\n> \t\t\tstop_column_filter();\n> \t\treturn ret;\n> +\t} else {\n> +\t\tif (filter.lines != -1)\n> +\t\t\tdie(_(\"-n option is only allowed in list mode.\"));\n> +\t\tif (filter.with_commit)\n> +\t\t\tdie(_(\"--contains option is only allowed in list mode.\"));\n> +\t\tif (filter.points_at.nr)\n> +\t\t\tdie(_(\"--points-at option is only allowed in list mode.\"));\n> +\t\tif (filter.merge_commit)\n> +\t\t\tdie(_(\"--merged and --no-merged options are only allowed in list mode.\"));\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\nI'm not sure why these go into the \"else\" clause here. The other side of\nthe conditional (i.e., when we are in list mode) always returns. I don't\n_mind_ it, it's just surprising in this patch.\n\nWhile we are re-wording the messages, we may want to drop the periods at\nthe end of the first three (or keep it on the fourth one, but I our\nusual style is to omit it).\n\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n\nThe tests looked reasonable to me.\n\n-Peff\n"},{"id":"314621","messageId":"20170320040250.7kb3sc25fjilhsta@sigill.intra.peff.net","threadId":"45419","inReplyTo":"20170318103256.27141-6-avarab@gmail.com","subject":"Re: [PATCH 5/8] tag: Implicitly supply --list given the -n option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T04:02:51Z","receivedAt":"2017-03-20T04:09:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 10:32:53AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Most of the work for this was done in my preceding \"tag: Implicitly\n> supply --list given another list-like option\", but I've split off this\n> patch since it's more contentious. Now invocations these invocations\n> will be synonymous:\n\ns/invocations these/these/\n\n> git tag -n 100\n> git tag -n --list 100\n> \n> Whereas before the former would die. This doesn't technically\n> introduce any more ambiguity than change to the other list-like\n> options, but it does introduce the possibility for more confusion\n> since instead of the latter of these dying:\n> \n> git tag -n100\n> git tag -n 100\n> \n> It now works entirely differently, i.e. invokes list mode with a\n> filter for \"100\".\n\nI think:\n\n git tag --list -n 100\n\nis similarly confusing now. It's really the optional-argument thing that\nis misleading, though I suppose silently converting the extra argument\ninto a filter does add an extra helping of confusion (most other\nprograms would complain \"100 is not a rev\" or something, but git-tag\njust produces no output).\n\nAs I said in the other patch, I could take or leave this one for now,\nbut I think I'd lean towards \"leave\".\n\n> Documentation/git-tag.txt | 9 +++++----\n> builtin/tag.c | 2 +-\n> t/t7004-tag.sh | 17 ++++++++++++++++-\n> 3 files changed, 22 insertions(+), 6 deletions(-)\n\nThe patch itself looks fine.\n\n-Peff\n"},{"id":"314622","messageId":"20170320042519.srtavoxhm3fln5mw@sigill.intra.peff.net","threadId":"45419","inReplyTo":"20170318103256.27141-7-avarab@gmail.com","subject":"Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T04:25:19Z","receivedAt":"2017-03-20T04:25:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 10:32:54AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the tag, branch & for-each-ref commands to have a --no-contains\n> option in addition to their longstanding --contains options.\n> \n> This allows for finding the last-good rollout tag given a known-bad\n> . Given a hypothetically bad commit cf5c7253e0 the git version\n> revert to can be found with this hacky two-liner:\n\ns/revert to/to &/, I think.\n\n\n> With this new --no-contains the same can be achieved with:\n> [..]\n\nThe goal sounds good to me.\n\n> In addition to those tests, add a test for \"tag\" which asserts that\n> --no-contains won't find tree/blob tags, which is slightly\n> unintuitive, but consistent with how --contains works & is documented.\n\nMakes sense. In theory we could dig into commits to find trees and blobs\nwhen the user gives us one. But I kind of doubt anybody really wants it,\nand it's expensive to compute. For the simple cases, --points-at already\ndoes the right thing.\n\n[more on that below, though...]\n\n> @@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n> \t\tlist = 1;\n> \n> -\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n> +\tif (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n> \t\tlist = 1;\n\nCould we wrap this long conditional?\n\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index df41fa0350..a11542c4fd 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {\n> \tN_(\"git for-each-ref [] []\"),\n> \tN_(\"git for-each-ref [--points-at ]\"),\n> \tN_(\"git for-each-ref [(--merged | --no-merged) []]\"),\n> -\tN_(\"git for-each-ref [--contains []]\"),\n> +\tN_(\"git for-each-ref [(--contains | --no-contains) []]\"),\n> \tNULL\n\nI'm not sure if this presentation implies that the two cannot be used\ntogether. It copies \"--merged/--no-merged\", but I think those two\n_can't_ be used together (it probably wouldn't be hard to make it work,\nbut if nobody cares it may not be worth spending time on).\n\nI also wonder if we need to explicitly document that --contains and\n--no-contains can be used together and don't cancel each other. The\nother option is to pick a new name (\"--omits\" is the most concise one I\ncould think of; maybe that is preferable anyway because it avoids\nnegation).\n\n> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \tif (!cmdmode && !create_tag_object) {\n> \t\tif (argc == 0)\n> \t\t\tcmdmode = 'l';\n> -\t\telse if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n> +\t\telse if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n\nDitto here on the wrapping. There were a few other long lines, but I\nwon't point them all out.\n\n> -\t\t/* We perform the filtering for the '--contains' option */\n> +\t\t/* We perform the filtering for the '--contains' option... */\n> \t\tif (filter->with_commit &&\n> -\t\t !commit_contains(filter, commit, &ref_cbdata->contains_cache))\n> +\t\t !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))\n> +\t\t\treturn 0;\n> +\t\t/* ...or for the `--no-contains' option */\n> +\t\tif (filter->no_commit &&\n> +\t\t commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))\n> \t\t\treturn 0;\n\nThis looks nice and simple. Good.\n\n> +# As the docs say, list tags which contain a specified *commit*. We\n> +# don't recurse down to tags for trees or blobs pointed to by *those*\n> +# commits.\n> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n> +\tcd no-contains &&\n> +\tblob=$(git rev-parse v0.3:v0.3.t) &&\n> +\ttree=$(git rev-parse v0.3^{tree}) &&\n> +\tgit tag tag-blob $blob &&\n> +\tgit tag tag-tree $tree &&\n> +\tgit tag --contains v0.3 >actual &&\n> +\tcat >expected <<-\\EOF &&\n> +\tv0.3\n> +\tv0.4\n> +\tv0.5\n> +\tEOF\n> +\ttest_cmp expected actual &&\n> +\tgit tag --no-contains v0.3 >actual &&\n> +\tcat >expected <<-\\EOF &&\n> +\tv0.1\n> +\tv0.2\n> +\tEOF\n> +\ttest_cmp expected actual\n> +'\n\nThe tests mostly look fine, but this one puzzled me. I guess we're\nchecking that tag-blob does not contain v0.3. But how could it?\n\nThe more interesting test to me is:\n\n git tag --contains $blob\n\nwhich should barf on a non-commit.\n\nFor the --no-contains side, you could argue that the blob-tag doesn't\ncontain the commit, and it should be listed. But it looks like we just\ndrop all non-commit tags completely as soon as we start to do a\ncontains/not-contains traversal.\n\nI think the more relevant comparison is \"--no-merged\", and it behaves\nthe same way as your new --no-contains. I don't think I saw this\nsubtlety in the documentation, though. It might be worth mentioning\n(unless I just missed it).\n\n-Peff\n"},{"id":"314626","messageId":"20170320042657.25x5gmh7uz5zgwzf@sigill.intra.peff.net","threadId":"45419","inReplyTo":"20170318103256.27141-1-avarab@gmail.com","subject":"Re: [PATCH 0/8] Various changes to the \"tag\" command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T04:26:57Z","receivedAt":"2017-03-20T04:33:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 18, 2017 at 10:32:48AM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> This series incorporates and replaces all the \"tag\" patches I have\n> floating around the list, and adds a lot in the mix which discovered\n> while working on the initial two patches, but which made sense as\n> separate patches.\n\nI had a few small comments, and I agree with the points that Junio\nraised. But aside from that, it looks good to me.\n\n-Peff\n"},{"id":"314635","messageId":"CACBZZX41W40PzSdxsPU+=NP==LuYdAUcD2n4Q381opn63DfcqA@mail.gmail.com","threadId":"45419","inReplyTo":"20170320035549.smzgu2jkaxrhuxl4@sigill.intra.peff.net","subject":"Re: [PATCH 4/8] tag: Implicitly supply --list given another list-like option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-20T09:16:49Z","receivedAt":"2017-03-20T09:19:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 20, 2017 at 4:55 AM, Jeff King wrote:\n> On Sat, Mar 18, 2017 at 10:32:52AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> With this change errors messages such as \"--contains option is only\n>> allowed with -l\" don't make sense anymore, since options like\n>> --contain turn on -l. Instead we error out when list-like options such\n>> as --contain are used in conjunction with conflicting options such as\n>> -d or -v.\n>\n> Yeah, I think this is the right approach.\n>\n>> This change does not consider \"-n\" a list-like option, even though\n>> that might be logical. Permitting it would allow:\n>>\n>> git tag -n 100\n>>\n>> As a synonym for:\n>>\n>> git tag -n --list 100\n>>\n>> Which, while not technically ambiguous as the option must already be\n>> provided as -n rather than -n , would be confusing.\n>\n> I'm not sure the existing behavior isn't confusing anyway (most optional\n> arguments are). But I don't mind being conservative and leaving out \"-n\"\n> for now; we can always convert it later if somebody feels strongly about\n> it.\n>\n>> diff --git a/builtin/tag.c b/builtin/tag.c\n>> index 0bba3fd070..3483636e59 100644\n>> --- a/builtin/tag.c\n>> +++ b/builtin/tag.c\n>> @@ -454,8 +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>> - if (argc == 0 && !cmdmode && !create_tag_object)\n>> - cmdmode = 'l';\n>> + if (!cmdmode && !create_tag_object) {\n>> + if (argc == 0)\n>> + cmdmode = 'l';\n>> + else if (filter.with_commit || filter.points_at.nr || filter.merge_commit)\n>> + cmdmode = 'l';\n>> + }\n>\n> Makes sense.\n>\n>> @@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>> if (column_active(colopts))\n>> stop_column_filter();\n>> return ret;\n>> + } else {\n>> + if (filter.lines != -1)\n>> + die(_(\"-n option is only allowed in list mode.\"));\n>> + if (filter.with_commit)\n>> + die(_(\"--contains option is only allowed in list mode.\"));\n>> + if (filter.points_at.nr)\n>> + die(_(\"--points-at option is only allowed in list mode.\"));\n>> + if (filter.merge_commit)\n>> + die(_(\"--merged and --no-merged options are only allowed in list mode.\"));\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> I'm not sure why these go into the \"else\" clause here. The other side of\n> the conditional (i.e., when we are in list mode) always returns. I don't\n> _mind_ it, it's just surprising in this patch.\n\nChanged it. It was a wart left over from an earlier version of this\nthat didn't make it on-list where the else mattered.\n\n> While we are re-wording the messages, we may want to drop the periods at\n> the end of the first three (or keep it on the fourth one, but I our\n> usual style is to omit it).\n\nDone.\n\n>> --- a/t/t7004-tag.sh\n>> +++ b/t/t7004-tag.sh\n>\n> The tests looked reasonable to me.\n>\n> -Peff\n"},{"id":"314646","messageId":"CACBZZX6FWjG1bXrk+ee8y=T5=ovxxybfrGzkkDxjskwDzhKPuA@mail.gmail.com","threadId":"45419","inReplyTo":"20170320042519.srtavoxhm3fln5mw@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-20T09:32:47Z","receivedAt":"2017-03-20T12:45:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 20, 2017 at 5:25 AM, Jeff King wrote:\n> On Sat, Mar 18, 2017 at 10:32:54AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the tag, branch & for-each-ref commands to have a --no-contains\n>> option in addition to their longstanding --contains options.\n>>\n>> This allows for finding the last-good rollout tag given a known-bad\n>> . Given a hypothetically bad commit cf5c7253e0 the git version\n>> revert to can be found with this hacky two-liner:\n>\n> s/revert to/to &/, I think.\nDone.\n>\n>> With this new --no-contains the same can be achieved with:\n>> [..]\n>\n> The goal sounds good to me.\n>\n>> In addition to those tests, add a test for \"tag\" which asserts that\n>> --no-contains won't find tree/blob tags, which is slightly\n>> unintuitive, but consistent with how --contains works & is documented.\n>\n> Makes sense. In theory we could dig into commits to find trees and blobs\n> when the user gives us one. But I kind of doubt anybody really wants it,\n> and it's expensive to compute. For the simple cases, --points-at already\n> does the right thing.\n>\n> [more on that below, though...]\n>\n>> @@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n>> list = 1;\n>>\n>> - if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n>> + if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n>> list = 1;\n>\n> Could we wrap this long conditional?\n\nDone. Will also go through the series as a whole & find other such occurances.\n\n>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n>> index df41fa0350..a11542c4fd 100644\n>> --- a/builtin/for-each-ref.c\n>> +++ b/builtin/for-each-ref.c\n>> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {\n>> N_(\"git for-each-ref [] []\"),\n>> N_(\"git for-each-ref [--points-at ]\"),\n>> N_(\"git for-each-ref [(--merged | --no-merged) []]\"),\n>> - N_(\"git for-each-ref [--contains []]\"),\n>> + N_(\"git for-each-ref [(--contains | --no-contains) []]\"),\n>> NULL\n>\n> I'm not sure if this presentation implies that the two cannot be used\n> together. It copies \"--merged/--no-merged\", but I think those two\n> _can't_ be used together (it probably wouldn't be hard to make it work,\n> but if nobody cares it may not be worth spending time on).\n\nYeah this has been bothering me a bit too. I'll change the various\n--help and synopsis entries to split them up, since they're not\nmutually exclusive at all.\n\n> I also wonder if we need to explicitly document that --contains and\n> --no-contains can be used together and don't cancel each other. The\n> other option is to pick a new name (\"--omits\" is the most concise one I\n> could think of; maybe that is preferable anyway because it avoids\n> negation).\n>\n>> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>> if (!cmdmode && !create_tag_object) {\n>> if (argc == 0)\n>> cmdmode = 'l';\n>> - else if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n>> + else if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)\n>\n> Ditto here on the wrapping. There were a few other long lines, but I\n> won't point them all out.\n>\n>> - /* We perform the filtering for the '--contains' option */\n>> + /* We perform the filtering for the '--contains' option... */\n>> if (filter->with_commit &&\n>> - !commit_contains(filter, commit, &ref_cbdata->contains_cache))\n>> + !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))\n>> + return 0;\n>> + /* ...or for the `--no-contains' option */\n>> + if (filter->no_commit &&\n>> + commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))\n>> return 0;\n>\n> This looks nice and simple. Good.\n>\n>> +# As the docs say, list tags which contain a specified *commit*. We\n>> +# don't recurse down to tags for trees or blobs pointed to by *those*\n>> +# commits.\n>> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '\n>> + cd no-contains &&\n>> + blob=$(git rev-parse v0.3:v0.3.t) &&\n>> + tree=$(git rev-parse v0.3^{tree}) &&\n>> + git tag tag-blob $blob &&\n>> + git tag tag-tree $tree &&\n>> + git tag --contains v0.3 >actual &&\n>> + cat >expected <<-\\EOF &&\n>> + v0.3\n>> + v0.4\n>> + v0.5\n>> + EOF\n>> + test_cmp expected actual &&\n>> + git tag --no-contains v0.3 >actual &&\n>> + cat >expected <<-\\EOF &&\n>> + v0.1\n>> + v0.2\n>> + EOF\n>> + test_cmp expected actual\n>> +'\n>\n> The tests mostly look fine, but this one puzzled me. I guess we're\n> checking that tag-blob does not contain v0.3. But how could it?\n\nIt's a very defensive test, but I'd like to leave it in.\n\nIt would be possible, and perhaps efficient in some cases, to\nimplement \"--no-contains \" internally as literally something\nthat just ran \"--contains \", got the list of tags, stashed\nthem into a hash, and then did a \"git tag -l\" and printed out anything\n*not* in the hash.\n\nThis test is asserting that we don't somehow regress to such an implementation.\n\n> The more interesting test to me is:\n>\n> git tag --contains $blob\n>\n> which should barf on a non-commit.\n\nWill make sure that's tested for.\n\n> For the --no-contains side, you could argue that the blob-tag doesn't\n> contain the commit, and it should be listed. But it looks like we just\n> drop all non-commit tags completely as soon as we start to do a\n> contains/not-contains traversal.\n>\n> I think the more relevant comparison is \"--no-merged\", and it behaves\n> the same way as your new --no-contains. I don't think I saw this\n> subtlety in the documentation, though. It might be worth mentioning\n> (unless I just missed it).\n\nFor --contains we explicitly document \"contain the specified commit\",\ni.e. you couldn't expect this to list tree-test, and indeed it\ndoesn't:\n\n $ git tag tree-test master^{tree}\n $ git tag -l --contains master '*test*'\n\nHowever the --[no-]merged option says \"reachable [...] from the\nspecified commit\", which seems to me to be a bit more ambiguous as to\nwhether you could expect it to print tree/blob tags.\n"},{"id":"314655","messageId":"xmqqinn4x7rz.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170320034443.wq2uf4yabsxh2nji@sigill.intra.peff.net","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-20T15:55:12Z","receivedAt":"2017-03-20T15:59:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think it's expected to work under the usual last-one-wins option\n> parsing. A more subtle case is that:\n>\n> git tag -l -d foo\n>\n> would override \"-l\" with \"-d\". That's reasonable under the same rule as\n> long as the user knows that the two are mode-selectors. I don't think we\n> make that explicit in the documentation, though, so perhaps it isn't\n> something users should rely on.\n\nYup, that is reasonable.\n"},{"id":"314656","messageId":"xmqqefxsx7ns.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"20170320042657.25x5gmh7uz5zgwzf@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] Various changes to the \"tag\" command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-20T15:57:43Z","receivedAt":"2017-03-20T16:06:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Mar 18, 2017 at 10:32:48AM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This series incorporates and replaces all the \"tag\" patches I have\n>> floating around the list, and adds a lot in the mix which discovered\n>> while working on the initial two patches, but which made sense as\n>> separate patches.\n>\n> I had a few small comments, and I agree with the points that Junio\n> raised. But aside from that, it looks good to me.\n\nThanks for catching issues I missed, and thank you to both of you to\nworking their solution out quickly. It seems that a final reroll\nwill be rather an uncontroversial one that can be merged to 'next'\nand then to 'master' soonish.\n\nThanks for working on this.\n"},{"id":"314660","messageId":"20170320161134.7g5ljysunwsjiz3l@sigill.intra.peff.net","threadId":"45419","inReplyTo":"CACBZZX5V3fMpwUCFr8vXMsxkXpz353-BOtaANYvnu9Qnz1GSQg@mail.gmail.com","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T16:11:34Z","receivedAt":"2017-03-20T16:14:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2017 at 05:09:02PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I think it's expected to work under the usual last-one-wins option\n> > parsing. A more subtle case is that:\n> >\n> > git tag -l -d foo\n> >\n> > would override \"-l\" with \"-d\". That's reasonable under the same rule as\n> > long as the user knows that the two are mode-selectors. I don't think we\n> > make that explicit in the documentation, though, so perhaps it isn't\n> > something users should rely on.\n> \n> That hasn't been the case since v1.8.5 (v1.8.4-rc0-12-g1158826394).\n> Now supplying multiple CMDMODE invocations will die.\n> \n> It is the case that we still need to manually check any pseudo-cmdmode\n> switches like \"tag -a\" (bool) v.s. \"tag -l\" (cmdmode). We check that\n> particular combination, but we doubtless have bugs like that in other\n> commands.\n\nYeah, you're right. I didn't look past the options[] array to see that\nwe handled this specially. I guess there's nothing to document, then.\n\n-Peff\n"},{"id":"314661","messageId":"CACBZZX5V3fMpwUCFr8vXMsxkXpz353-BOtaANYvnu9Qnz1GSQg@mail.gmail.com","threadId":"45419","inReplyTo":"20170320034443.wq2uf4yabsxh2nji@sigill.intra.peff.net","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-20T16:09:02Z","receivedAt":"2017-03-20T16:18:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 20, 2017 at 4:44 AM, Jeff King wrote:\n> On Sat, Mar 18, 2017 at 11:43:47AM -0700, Junio C Hamano wrote:\n>\n>> > +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n>> > + git tag -l -l >actual &&\n>> > + test_cmp expect actual &&\n>> > + git tag --list --list >actual &&\n>> > + test_cmp expect actual &&\n>> > + git tag --list -l --list >actual &&\n>> > + test_cmp expect actual\n>> > +'\n>>\n>> OK. I do not care too deeply about this one, but somebody may want\n>> to tighten up the command line parsing to detect conflicting or\n>> duplicated cmdmode as an error in the future, and at that time this\n>> will require updating. I am not sure if we want to promise that\n>> giving multiple -l will keep working.\n>\n> I think it's expected to work under the usual last-one-wins option\n> parsing. A more subtle case is that:\n>\n> git tag -l -d foo\n>\n> would override \"-l\" with \"-d\". That's reasonable under the same rule as\n> long as the user knows that the two are mode-selectors. I don't think we\n> make that explicit in the documentation, though, so perhaps it isn't\n> something users should rely on.\n\nThat hasn't been the case since v1.8.5 (v1.8.4-rc0-12-g1158826394).\nNow supplying multiple CMDMODE invocations will die.\n\nIt is the case that we still need to manually check any pseudo-cmdmode\nswitches like \"tag -a\" (bool) v.s. \"tag -l\" (cmdmode). We check that\nparticular combination, but we doubtless have bugs like that in other\ncommands.\n"},{"id":"314685","messageId":"xmqqshm7x4gb.fsf@gitster.mtv.corp.google.com","threadId":"45419","inReplyTo":"xmqqinn4x7rz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/8] tag: Change misleading --list documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-20T17:07:00Z","receivedAt":"2017-03-20T17:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I think it's expected to work under the usual last-one-wins option\n>> parsing. A more subtle case is that:\n>>\n>> git tag -l -d foo\n>>\n>> would override \"-l\" with \"-d\". That's reasonable under the same rule as\n>> long as the user knows that the two are mode-selectors. I don't think we\n>> make that explicit in the documentation, though, so perhaps it isn't\n>> something users should rely on.\n>\n> Yup, that is reasonable.\n\nOops. I meant \"two are mode-selectors and can override with the\nusual last one wins rule\" was reasonable. As Ævar says, this is an\nestablished practice and may already be documented sufficiently.\n\n"},{"id":"314718","messageId":"20170320195209.oyivf2ta3m3kg264@sigill.intra.peff.net","threadId":"45419","inReplyTo":"CACBZZX6FWjG1bXrk+ee8y=T5=ovxxybfrGzkkDxjskwDzhKPuA@mail.gmail.com","subject":"Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-20T19:52:09Z","receivedAt":"2017-03-20T19:53:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > I think the more relevant comparison is \"--no-merged\", and it behaves\n> > the same way as your new --no-contains. I don't think I saw this\n> > subtlety in the documentation, though. It might be worth mentioning\n> > (unless I just missed it).\n> \n> For --contains we explicitly document \"contain the specified commit\",\n> i.e. you couldn't expect this to list tree-test, and indeed it\n> doesn't:\n> \n> $ git tag tree-test master^{tree}\n> $ git tag -l --contains master '*test*'\n\nRight, \"--contains\" cannot have a commit inside a tree, so we were\ncorrect to skip the computation entirely. But does that mean that\n\"--no-contains\" should be the complement of that, or should it only\ninclude tags whose \"contains\" can be computed in the first place?\n\nIOW, I don't think --contains or --merged are interesting here; they\ngive the right answer by skipping the computation. The question is what\nthe \"--no-\" variants should do.\n\n> However the --[no-]merged option says \"reachable [...] from the\n> specified commit\", which seems to me to be a bit more ambiguous as to\n> whether you could expect it to print tree/blob tags.\n\nI suspect that --no-merged behaves the way it does because it originally\ncame from git-branch, where you only have commits in the first place.\nThe other commands only learned about it during the move to ref-filter,\nand nobody thought about this corner case.\n\nSo we could just treat it like a bug and fix it. But I doubt anybody\ncares that much in practice either way, so documenting it as \"any use of\n--contains, --no-contains, --no-merged, or --merged requires that the\nref in question be a commit\" is fine, too.\n\n-Peff\n"},{"id":"314722","messageId":"CACBZZX5GErSBsiCz0Y2SgtvckcPE51ekfUgBpvYjNF1u44piMA@mail.gmail.com","threadId":"45419","inReplyTo":"20170320195209.oyivf2ta3m3kg264@sigill.intra.peff.net","subject":"Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-20T20:04:32Z","receivedAt":"2017-03-20T20:06:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Mar 20, 2017 at 8:52 PM, Jeff King wrote:\n> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I think the more relevant comparison is \"--no-merged\", and it behaves\n>> > the same way as your new --no-contains. I don't think I saw this\n>> > subtlety in the documentation, though. It might be worth mentioning\n>> > (unless I just missed it).\n>>\n>> For --contains we explicitly document \"contain the specified commit\",\n>> i.e. you couldn't expect this to list tree-test, and indeed it\n>> doesn't:\n>>\n>> $ git tag tree-test master^{tree}\n>> $ git tag -l --contains master '*test*'\n>\n> Right, \"--contains\" cannot have a commit inside a tree, so we were\n> correct to skip the computation entirely. But does that mean that\n> \"--no-contains\" should be the complement of that, or should it only\n> include tags whose \"contains\" can be computed in the first place?\n>\n> IOW, I don't think --contains or --merged are interesting here; they\n> give the right answer by skipping the computation. The question is what\n> the \"--no-\" variants should do.\n\nI think both should only ever find commits. I only came up with this\ntree/blob scenario for the purposes of tests, but it would make the\ncommand less useful & way slower in practice. E.g. now you want to\nfind what to revert to and some blob tag shows up.\n\n>> However the --[no-]merged option says \"reachable [...] from the\n>> specified commit\", which seems to me to be a bit more ambiguous as to\n>> whether you could expect it to print tree/blob tags.\n>\n> I suspect that --no-merged behaves the way it does because it originally\n> came from git-branch, where you only have commits in the first place.\n> The other commands only learned about it during the move to ref-filter,\n> and nobody thought about this corner case.\n>\n> So we could just treat it like a bug and fix it. But I doubt anybody\n> cares that much in practice either way, so documenting it as \"any use of\n> --contains, --no-contains, --no-merged, or --merged requires that the\n> ref in question be a commit\" is fine, too.\n\nIt's fixed in my soon-to-be resent series.\n"}]}