threads / patch / 45419

patch, 8 partstag: Remove a TODO item from the test suite

Subject: [PATCH 1/8] tag: Remove a TODO item from the test suite

## tl;dr

41 messages between Mar 18, 2017 and Mar 20, 2017. Diffs are folded; open one to read it.

replies: 40people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · lore

[PATCH 0/8] Various changes to the "tag" command

This series incorporates and replaces all the "tag" patches I have floating around the list, and adds a lot in the mix which discovered while working on the initial two patches, but which made sense as separate patches.

It's based no top of Jeff's gitster/jk/ref-filter-flags-cleanup.

I'm bundling this all together because a lot of these patches touch the exact same code, and in other cases subsequent patches make use of test suite improvements I made earlier in the series. So although some could be split out entirely (e.g. the --point-at patch), I'd like to just present them all for review together & split out any if there's strong objections to basing them on top of each other.

I think this series changes addresses all the points brought up by Junio/Jeff about my previous patches, except there's no extensive discussion of how the filtering mechanism works in general as pointed out by Junio in <xmqqwpbvumrk.fsf@gitster.mtv.corp.google.com>.

I think it makes sense to have that, but in the interest of getting something out the door I'm not working on that for now.

Ævar Arnfjörð Bjarmason (8):
  tag: Remove a TODO item from the test suite
  tag: Refactor the options handling code to be less bizarro
  tag: Change  misleading --list <pattern> documentation
  tag: Implicitly supply --list given another list-like option
  tag: Implicitly supply --list given the -n option
  ref-filter: Add --no-contains option to tag/branch/for-each-ref
  tag: Add tests for --with and --without
  tag: Change --point-at to default to HEAD
 Documentation/git-branch.txt           |  15 ++-
 Documentation/git-for-each-ref.txt     |   6 +-
 Documentation/git-tag.txt              |  44 ++++---
 builtin/branch.c                       |   4 +-
 builtin/for-each-ref.c                 |   3 +-
 builtin/tag.c                          |  36 ++++--
 contrib/completion/git-completion.bash |   4 +-
 parse-options.h                        |   4 +-
 ref-filter.c                           |  19 ++-
 ref-filter.h                           |   1 +
 t/t3201-branch-contains.sh             |  51 +++++++-
 t/t6302-for-each-ref-filter.sh         |  16 +++
 t/t7004-tag.sh                         | 226 +++++++++++++++++++++++++++++++--
 13 files changed, 371 insertions(+), 58 deletions(-)
-- 
2.11.0
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

Change the test for "git tag -l" to not have an associated TODO comment saying that it should return non-zero if there's no tags.

This was added in commit ef5a6fb597 ("Add test-script for git-tag", 2007-06-28) when the tests for "tag" were initially added, but at this point changing this would be inconsistent with how "git tag" is a synonym for "git tag -l", and would needlessly break external code that relies on this porcelain command.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7004-tag.sh | 2 --
 1 file changed, 2 deletions(-)
Show changes to t/t7004-tag.sh +0 −2
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..876ccfc830 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -16,7 +16,6 @@ tag_exists () {
 	git show-ref --quiet --verify refs/tags/"$1"
 }
 
-# todo: git tag -l now returns always zero, when fixed, change this test
 test_expect_success 'listing all tags in an empty tree should succeed' '
 	git tag -l &&
 	git tag
@@ -136,7 +135,6 @@ test_expect_success \
 	'listing a tag using a matching pattern should output that tag' \
 	'test $(git tag -l mytag) = mytag'
 
-# todo: git tag -l now returns always zero, when fixed, change this test
 test_expect_success \
 	'listing tags using a non-matching pattern should suceed' \
 	'git tag -l xxx'
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 18:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/8] tag: Remove a TODO item from the test suite

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 8 quoted lines
> Change the test for "git tag -l" to not have an associated TODO
> comment saying that it should return non-zero if there's no tags.
>
> This was added in commit ef5a6fb597 ("Add test-script for git-tag",
> 2007-06-28) when the tests for "tag" were initially added, but at this
> point changing this would be inconsistent with how "git tag" is a
> synonym for "git tag -l", and would needlessly break external code
> that relies on this porcelain command.
Makes sense.

I'll retitle s/Remove/remove/ so that "git shortlog --no-merges" would look more consistent, though.

Thanks.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 18:42 UTC · re: Junio C Hamano · lore

[PATCH 0/2] doc/SubmittingPatches: A couple of minor improvements

On Sat, Mar 18, 2017 at 7:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I'll retitle s/Remove/remove/ so that "git shortlog --no-merges"
> would look more consistent, though.

I already found a few grammar / phrasing issues with the commit messages, so I'll just change this on my side for a resend.

But I noticed that this casing rule wasn't documented in SubmittingPatches, so here's a patch for that, and while I'm at it another small improvement that I've been meaning to make to it based on a local alias I have.

Ævar Arnfjörð Bjarmason (2):
  doc/SubmittingPatches: clarify the casing convention for "area:
    change..."
  doc/SubmittingPatches: show how to get a CLI commit summary
 Documentation/SubmittingPatches | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
-- 
2.11.0
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 18:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."

Amend the section which describes how the first line of the subject should look like to say that the ":" in "area: " shouldn't be treated like a full stop for the purposes of letter casing.

Change the two subject examples to make this new paragraph clearer, i.e. "unstar" is not a common word, and "git-cherry-pick.txt" is a much longer string than "githooks.txt". Pick two recent commits from git.git that fit better for the description.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/SubmittingPatches | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Show changes to Documentation/SubmittingPatches +7 −2
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 3faf7eb884..9ef624ce38 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -98,12 +98,17 @@ should skip the full stop.  It is also conventional in most cases to
 prefix the first line with "area: " where the area is a filename or
 identifier for the general area of the code being modified, e.g.
 
-  . archive: ustar header checksum is computed unsigned
-  . git-cherry-pick.txt: clarify the use of revision range notation
+  . doc: clarify distinction between sign-off and pgp-signing
+  . githooks.txt: improve the intro section
 
 If in doubt which identifier to use, run "git log --no-merges" on the
 files you are modifying to see the current conventions.
 
+It's customary to start the remainder of the first line after "area: "
+with a lower-case letter. E.g. "doc: clarify...", not "doc:
+Clarify...", or "githooks.txt: improve...", not "githooks.txt:
+Improve...".
+
 The body should provide a meaningful commit message, which:
 
   . explains the problem the change tries to solve, iow, what is wrong
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 19:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 7 quoted lines
>  prefix the first line with "area: " where the area is a filename or
>  identifier for the general area of the code being modified, e.g.
>  
> -  . archive: ustar header checksum is computed unsigned
> -  . git-cherry-pick.txt: clarify the use of revision range notation
> +  . doc: clarify distinction between sign-off and pgp-signing
> +  . githooks.txt: improve the intro section

Sorry, but I fail to spot why this is an improvement (it is not making things worse, either).

Show 7 quoted lines
>  If in doubt which identifier to use, run "git log --no-merges" on the
>  files you are modifying to see the current conventions.
>  
> +It's customary to start the remainder of the first line after "area: "
> +with a lower-case letter. E.g. "doc: clarify...", not "doc:
> +Clarify...", or "githooks.txt: improve...", not "githooks.txt:
> +Improve...".
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 19:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."

On Sat, Mar 18, 2017 at 8:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  prefix the first line with "area: " where the area is a filename or
>>  identifier for the general area of the code being modified, e.g.
>>
>> -  . archive: ustar header checksum is computed unsigned
>> -  . git-cherry-pick.txt: clarify the use of revision range notation
>> +  . doc: clarify distinction between sign-off and pgp-signing
>> +  . githooks.txt: improve the intro section
>
> Sorry, but I fail to spot why this is an improvement (it is not
> making things worse, either).
Because...
Show 7 quoted lines
>>  If in doubt which identifier to use, run "git log --no-merges" on the
>>  files you are modifying to see the current conventions.
>>
>> +It's customary to start the remainder of the first line after "area: "
>> +with a lower-case letter. E.g. "doc: clarify...", not "doc:
>> +Clarify...", or "githooks.txt: improve...", not "githooks.txt:
>> +Improve...".

...it makes this subsequent example more succinct and clear, because e.g. "githooks.txt" is shorter than "git-cherry-pick.txt", and "clarify" is obviously a normal looking word which you'd expect to be capitalized after a full stop, but it might take a couple of readings to understand that "unstar" without a hyphen isn't some jargon.

Junio C Hamano· Mar 18, 2017, 20:07 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/2] doc/SubmittingPatches: clarify the casing convention for "area: change..."

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> ...it makes this subsequent example more succinct and clear, because
> e.g. "githooks.txt" is shorter than "git-cherry-pick.txt", and
> "clarify" is obviously a normal looking word...
Ah, that makes sense.  Thanks.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 18:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/2] doc/SubmittingPatches: show how to get a CLI commit summary

Amend the section which describes how to get a commit summary to show how do to that with "git show", currently the documentation only shows how to do that with gitk.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/SubmittingPatches | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
Show changes to Documentation/SubmittingPatches +10 −1
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 9ef624ce38..78c8e36a4b 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -134,8 +134,17 @@ with the subject enclosed in a pair of double-quotes, like this:
     noticed that ...
 
 The "Copy commit summary" command of gitk can be used to obtain this
-format.
+format, or this invocation of "git show":
 
+    git show -s --date=format:%Y-%m-%d --pretty='commit %h ("%s", %ad)' <commit>
+
+To turn that into a handy alias:
+
+    git config --global alias.git-commit-summary "show -s --date=format:%Y-%m-%d --pretty='commit %h (\"%s\", %ad)'"
+
+And then to get the commit summary:
+
+    git git-commit-summary <commit>
 
 (3) Generate your patch using Git tools out of your commits.
 
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 19:07 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] doc/SubmittingPatches: show how to get a CLI commit summary

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 5 quoted lines
>  The "Copy commit summary" command of gitk can be used to obtain this
> -format.
> +format, or this invocation of "git show":
>  
> +    git show -s --date=format:%Y-%m-%d --pretty='commit %h ("%s", %ad)' <commit>

I've seen (I think I stole it from Peff) this one recommended often on the list, which is shorter:

    $ git show --date=short -s --pretty='format:%h ("%s", %ad)' <commit>
Jakub Narębski· Mar 19, 2017, 00:48 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/8] tag: Remove a TODO item from the test suite

W dniu 18.03.2017 o 11:32, Ævar Arnfjörð Bjarmason pisze:
Show 8 quoted lines
> @@ -136,7 +135,6 @@ test_expect_success \
>  	'listing a tag using a matching pattern should output that tag' \
>  	'test $(git tag -l mytag) = mytag'
>  
> -# todo: git tag -l now returns always zero, when fixed, change this test
>  test_expect_success \
>  	'listing tags using a non-matching pattern should suceed' \
>  	'git tag -l xxx'

Could you fix s/suceed/succeed/ in the test description, while at it?

-- 
Jakub Narębski
Ævar Arnfjörð Bjarmason· Mar 19, 2017, 06:43 UTC · re: Jakub Narębski · lore

Re: [PATCH 1/8] tag: Remove a TODO item from the test suite

On Sun, Mar 19, 2017 at 1:48 AM, Jakub Narębski <jnareb@gmail.com> wrote:
Show 13 quoted lines
> W dniu 18.03.2017 o 11:32, Ævar Arnfjörð Bjarmason pisze:
>
>> @@ -136,7 +135,6 @@ test_expect_success \
>>       'listing a tag using a matching pattern should output that tag' \
>>       'test $(git tag -l mytag) = mytag'
>>
>> -# todo: git tag -l now returns always zero, when fixed, change this test
>>  test_expect_success \
>>       'listing tags using a non-matching pattern should suceed' \
>>       'git tag -l xxx'
>
> Could you fix s/suceed/succeed/ in the test description,
> while at it?
Sure, part of v2 now.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 5/8] tag: Implicitly supply --list given the -n option

Change the "tag" command to treat the "-n" invocation as a list-like option in addition to --contains, --points-at etc.

Most of the work for this was done in my preceding "tag: Implicitly supply --list given another list-like option", but I've split off this patch since it's more contentious. Now invocations these invocations will be synonymous:

    git tag -n 100
    git tag -n --list 100

Whereas before the former would die. This doesn't technically introduce any more ambiguity than change to the other list-like options, but it does introduce the possibility for more confusion since instead of the latter of these dying:

    git tag -n100
    git tag -n 100

It now works entirely differently, i.e. invokes list mode with a filter for "100".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-tag.txt |  9 +++++----
 builtin/tag.c             |  2 +-
 t/t7004-tag.sh            | 17 ++++++++++++++++-
 3 files changed, 22 insertions(+), 6 deletions(-)
Show changes to 3 files +22 −6

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 2acd3b6beb..e7793afad1 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -82,10 +82,11 @@ OPTIONS
 
 -n<num>::
 	<num> specifies how many lines from the annotation, if any,
-	are printed when using -l.
-	The default is not to print any annotation lines.
-	If no number is given to `-n`, only the first line is printed.
-	If the tag is not annotated, the commit message is displayed instead.
+	are printed when using -l. Implies `--list`.
++
+The default is not to print any annotation lines.
+If no number is given to `-n`, only the first line is printed.
+If the tag is not annotated, the commit message is displayed instead.
 
 -l::
 --list::
diff --git a/builtin/tag.c b/builtin/tag.c
index 3483636e59..2da28a5ce6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -457,7 +457,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!cmdmode && !create_tag_object) {
 		if (argc == 0)
 			cmdmode = 'l';
-		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
+		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 5c94932f0f..ba1ab1f21c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -639,6 +639,11 @@ test_expect_success \
 	git tag -n0 -l tag-one-line >actual &&
 	test_cmp expect actual &&
 
+	git tag -n0 | grep "^tag-one-line" >actual &&
+	test_cmp expect actual &&
+	git tag -n0 tag-one-line >actual &&
+	test_cmp expect actual &&
+
 	echo "tag-one-line    A msg" >expect &&
 	git tag -n1 -l | grep "^tag-one-line" >actual &&
 	test_cmp expect actual &&
@@ -652,6 +657,17 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+test_expect_success 'The -n 100 invocation means -n --list 100, not -n100' '
+	>expect &&
+	git tag -n 100 >actual &&
+	test_cmp expect actual &&
+
+	git tag -m "A msg" 100 &&
+	echo "100             A msg" >expect &&
+	git tag -n 100 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success \
 	'listing the zero-lines message of a non-signed tag should succeed' '
 	git tag -m "" tag-zero-lines &&
@@ -1495,7 +1511,6 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -a -s -m -F &&
 	test_must_fail git tag -a -s -m -F -l &&
 	test_must_fail git tag -l -v &&
-	test_must_fail git tag -n 100 &&
 	test_must_fail git tag -n 100 -v &&
 	test_must_fail git tag -l -m msg &&
 	test_must_fail git tag -l -F some file &&
-- 
2.11.0
Jeff King· Mar 20, 2017, 04:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 5/8] tag: Implicitly supply --list given the -n option

On Sat, Mar 18, 2017 at 10:32:53AM +0000, Ævar Arnfjörð Bjarmason wrote:
> Most of the work for this was done in my preceding "tag: Implicitly
> supply --list given another list-like option", but I've split off this
> patch since it's more contentious. Now invocations these invocations
> will be synonymous:
s/invocations these/these/
Show 13 quoted lines
>     git tag -n 100
>     git tag -n --list 100
> 
> Whereas before the former would die. This doesn't technically
> introduce any more ambiguity than change to the other list-like
> options, but it does introduce the possibility for more confusion
> since instead of the latter of these dying:
> 
>     git tag -n100
>     git tag -n 100
> 
> It now works entirely differently, i.e. invokes list mode with a
> filter for "100".
I think:
  git tag --list -n 100

is similarly confusing now. It's really the optional-argument thing that is misleading, though I suppose silently converting the extra argument into a filter does add an extra helping of confusion (most other programs would complain "100 is not a rev" or something, but git-tag just produces no output).

As I said in the other patch, I could take or leave this one for now, but I think I'd lean towards "leave".

>  Documentation/git-tag.txt |  9 +++++----
>  builtin/tag.c             |  2 +-
>  t/t7004-tag.sh            | 17 ++++++++++++++++-
>  3 files changed, 22 insertions(+), 6 deletions(-)
The patch itself looks fine.
-Peff
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/8] tag: Refactor the options handling code to be less bizarro

Refactor the options handling code so that "cmdmode = 'l'" isn't set on the likes of "tag -a". This change introduces no functional changes, but makes subsequent patches easier to reason about, as both "annotate = 1" "cmdmode = 'l'" won't be unexpectedly set if "tag" is merely invoked as "tag -a", "tag -s" etc.

Before this the cmdmode variable would be set to 'l' indicating that "tag" was operating in list mode, this was then used a couple of lines later by e.g. the codepath for "tag -a" where the command would only bail out with usage_with_options() if the likes of "-a" were set, *and* some cmdmode (i.e. 'l' in this case) had been set.

This behavior was an emergent property of other earlier changes, starting with commit 6fa8342b12 (tag: Check that options are only allowed in the appropriate mode, 2008-11-05), to its present form in commit e6b722db09 ("tag: use OPT_CMDMODE", 2013-07-30).

Change the test suite to more exhaustively assert that already existing behavior related to this option parsing is kept.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/tag.c  |  4 ++--
 t/t7004-tag.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)
Show changes to 2 files +15 −2

builtin/tag.c, t/t7004-tag.sh

diff --git a/builtin/tag.c b/builtin/tag.c
index ad29be6923..0bba3fd070 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
 
-	if (argc == 0 && !cmdmode)
+	if (argc == 0 && !cmdmode && !create_tag_object)
 		cmdmode = 'l';
 
-	if ((create_tag_object || force) && (cmdmode != 0))
+	if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 876ccfc830..74fc82a3c0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1455,6 +1455,19 @@ test_expect_success 'checking that initial commit is in all tags' "
 
 test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -a &&
+	test_must_fail git tag -a -l &&
+	test_must_fail git tag -s &&
+	test_must_fail git tag -s -l &&
+	test_must_fail git tag -m &&
+	test_must_fail git tag -m -l &&
+	test_must_fail git tag -m "hlagh" &&
+	test_must_fail git tag -m "hlagh" -l &&
+	test_must_fail git tag -F &&
+	test_must_fail git tag -F -l &&
+	test_must_fail git tag -f &&
+	test_must_fail git tag -f -l &&
+	test_must_fail git tag -a -s -m -F &&
+	test_must_fail git tag -a -s -m -F -l &&
 	test_must_fail git tag -l -v &&
 	test_must_fail git tag -n 100 &&
 	test_must_fail git tag -l -m msg &&
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 18:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 11 quoted lines
> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..0bba3fd070 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>  
> -	if (argc == 0 && !cmdmode)
> +	if (argc == 0 && !cmdmode && !create_tag_object)
>  		cmdmode = 'l';

So with this change, if we cannot infer that we are creating a tag object from other options, we leave cmdmode to its original 0.

> -	if ((create_tag_object || force) && (cmdmode != 0))
> +	if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
>  		usage_with_options(git_tag_usage, options);

And then immediately after that, we complain by detecting that we know we are creating a tag and a non-zero cmdmode is in effect (i.e. 'l', 'd' or 'v', none of which is about creating a tag). The way we used to detect that we are doing something other than tag creation was by seeing cmdmode is set to anything. Because of your earlier change, that no longer is true. You need to separately check (!cmdmode && !argc).

By following the logic that way, I can see how this change at this step is a no-op, but I have to say that the code with this patch looks much more bizarre than the original.

I am not sure why you want to do the first change at this step in the first place. Is it because you'd want to take over (!cmdmode && !argc) condtion to default to 'list'? With the change in 4/8 and 5/8, you are ensuring that cmdmode is set to 'l' for these new cases before the code hits the check to call usage_with_options(). And at that point, you can use the original "are we creating and !cmdmode says we are not? That's contradiction" logic without making it more bizarre with this patch, no?

Ævar Arnfjörð Bjarmason· Mar 18, 2017, 19:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro

On Sat, Mar 18, 2017 at 7:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 41 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index ad29be6923..0bba3fd070 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       }
>>       create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>>
>> -     if (argc == 0 && !cmdmode)
>> +     if (argc == 0 && !cmdmode && !create_tag_object)
>>               cmdmode = 'l';
>
> So with this change, if we cannot infer that we are creating a tag
> object from other options, we leave cmdmode to its original 0.
>
>> -     if ((create_tag_object || force) && (cmdmode != 0))
>> +     if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
>>               usage_with_options(git_tag_usage, options);
>
> And then immediately after that, we complain by detecting that we
> know we are creating a tag and a non-zero cmdmode is in effect
> (i.e. 'l', 'd' or 'v', none of which is about creating a tag).  The
> way we used to detect that we are doing something other than tag
> creation was by seeing cmdmode is set to anything.  Because of your
> earlier change, that no longer is true.  You need to separately
> check (!cmdmode && !argc).
>
> By following the logic that way, I can see how this change at this
> step is a no-op, but I have to say that the code with this patch
> looks much more bizarre than the original.
>
> I am not sure why you want to do the first change at this step in
> the first place.  Is it because you'd want to take over (!cmdmode &&
> !argc) condtion to default to 'list'?  With the change in 4/8 and
> 5/8, you are ensuring that cmdmode is set to 'l' for these new cases
> before the code hits the check to call usage_with_options().  And at
> that point, you can use the original "are we creating and !cmdmode
> says we are not?  That's contradiction" logic without making it more
> bizarre with this patch, no?

Nothing about this patch is needed for the rest of the series. I just tried rebasing it out now and all tests pass & everything works as expected.

The reason I'm submitting it is because while this works *now* and there's no cases I can see currently where cmdmode is 'l' after the current `((create_tag_object || force) && (cmdmode != 0))`, during a lengthy debugging session when I was hacking on a subsequent patch in this series it took me a long time to track down a segfault later in the file because surely it was impossible that I was in cmdmode = 'l' with only "git tag -a".

Partly that was late night hacking session blindness after having read the !argc and assuming that the -a would be counted towards argc.

But I thought it was very a very bizarre pattern to set us to cmdmode = 'l' when we're not in that mode at all just to, as can be seen in the diff, get around a slightly more verbose one-time if-check.

So I think it's worth it to include this for less confusion in any subsequent patches to tag.c.

Junio C Hamano· Mar 18, 2017, 19:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> But I thought it was very a very bizarre pattern to set us to cmdmode
> = 'l' when we're not in that mode at all just to, as can be seen in
> the diff, get around a slightly more verbose one-time if-check.

When I wrote my response, I viewed that setting as committing to be in the "list" mode, not as a workaround. So checking with !cmdmode to make sure that the command line is not asking to create a tag makes tons of sense; the new test makes it unreadable from that point of view.

Ævar Arnfjörð Bjarmason· Mar 18, 2017, 20:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro

On Sat, Mar 18, 2017 at 8:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> But I thought it was very a very bizarre pattern to set us to cmdmode
>> = 'l' when we're not in that mode at all just to, as can be seen in
>> the diff, get around a slightly more verbose one-time if-check.
>
> When I wrote my response, I viewed that setting as committing to be
> in the "list" mode, not as a workaround.  So checking with !cmdmode
> to make sure that the command line is not asking to create a tag
> makes tons of sense; the new test makes it unreadable from that
> point of view.

Makes sense. Looking at this more carefully there's never any cases where `create_tag_object && cmdmode == 'l'` is true once we make it past `usage_with_options`, which was the bug I introduced locally which made this whole thing a bit confusing for me.

I'll drop this patch from v2.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 7/8] tag: Add tests for --with and --without

Change the test suite to test for these options. Before this change there were no tests for this at all. This change doesn't exhaustively test for them as well as their --contains and --no-contains aliases, but at least it's something.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7004-tag.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7004-tag.sh +1 −1
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 428e21c369..f7ff4e034b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1592,7 +1592,7 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag --contains --no-contains
 '
 
-for option in --contains --no-contains --merged --no-merged --points-at
+for option in --contains --with --no-contains --without --merged --no-merged --points-at
 do
 	test_expect_success "mixing incompatible modes with $option is forbidden" "
 		test_must_fail git tag -d $option HEAD &&
-- 
2.11.0
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 8/8] tag: Change --point-at to default to HEAD

Change the --points-at option to default to HEAD for consistency with its siblings --contains, --merged etc. which default to HEAD. This changes behavior added in commit ae7706b9ac (tag: add --points-at list option, 2012-02-08).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-tag.txt | 3 ++-
 builtin/tag.c             | 3 ++-
 t/t7004-tag.sh            | 8 +++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)
Show changes to 3 files +11 −3

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index d0b506f120..1d66348b6a 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -136,7 +136,8 @@ This option is only applicable when listing tags without annotation lines.
 	not specified). Implies `--list`.
 
 --points-at <object>::
-	Only list tags of the given object. Implies `--list`.
+	Only list tags of the given object (HEAD if not
+	specified). Implies `--list`.
 
 -m <msg>::
 --message=<msg>::
diff --git a/builtin/tag.c b/builtin/tag.c
index f91ae171b7..300f831fb1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -433,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
-			N_("print only tags of the object"), 0, parse_opt_object_name
+			N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
+			parse_opt_object_name, (intptr_t) "HEAD"
 		},
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f7ff4e034b..112d96b4ce 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1601,7 +1601,8 @@ do
 	"
 	test_expect_success "Doing 'git tag --list-like $option <commit> <pattern> is permitted" "
 		git tag -n $option HEAD HEAD &&
-		git tag $option HEAD HEAD
+		git tag $option HEAD HEAD &&
+		git tag $option
 	"
 done
 
@@ -1613,6 +1614,11 @@ test_expect_success '--points-at can be used in non-list mode' '
 	test_cmp expect actual
 '
 
+test_expect_success '--points-at is a synonym for --points-at HEAD' '
+	git tag --points-at >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--points-at finds lightweight tags' '
 	echo v4.0 >expect &&
 	git tag --points-at v4.0 >actual &&
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 18:54 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 8/8] tag: Change --point-at to default to HEAD

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> Change the --points-at option to default to HEAD for consistency with
> its siblings --contains, --merged etc. which default to HEAD. This
> changes behavior added in commit ae7706b9ac (tag: add --points-at list
> option, 2012-02-08).
Makes a lot of sense to me.
> +test_expect_success '--points-at is a synonym for --points-at HEAD' '
> +	git tag --points-at >actual &&

Even if "expect" is the same one established earlier, it is easier to read and understand individual tests if you explicitly said what this one expects.

Thanks.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 19:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 8/8] tag: Change --point-at to default to HEAD

On Sat, Mar 18, 2017 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the --points-at option to default to HEAD for consistency with
>> its siblings --contains, --merged etc. which default to HEAD. This
>> changes behavior added in commit ae7706b9ac (tag: add --points-at list
>> option, 2012-02-08).
>
> Makes a lot of sense to me.
>
>> +test_expect_success '--points-at is a synonym for --points-at HEAD' '
>> +     git tag --points-at >actual &&
>
> Even if "expect" is the same one established earlier, it is easier
> to read and understand individual tests if you explicitly said what
> this one expects.
Makes sense. Queued that change in my WIP v2.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 3/8] tag: Change misleading --list <pattern> documentation

Change the documentation for --list so that it's described as a toggle, not as an option that takes a <pattern> as an argument.

Junio initially documented this in b867c7c23a ("git-tag: -l to list tags (usability).", 2006-02-17), but later Jeff King changed "tag" to accept multiple patterns in 588d0e834b ("tag: accept multiple patterns for --list", 2011-06-20).

However, documenting this as "-l <pattern>" was never correct, as these both worked before Jeff's change:

    git tag -l 'v*'
    git tag 'v*' -l

One would expect an option that was documented like that to only accept:

    git tag --list
    git tag --list 'v*rc*'
And after Jeff's change, one that took multiple patterns:
    git tag --list 'v*rc*' --list '*2.8*'

But since it's actually a toggle all of these work as well, and produce identical output to the last example above:

    git tag --list 'v*rc*' '*2.8*'
    git tag --list 'v*rc*' '*2.8*' --list --list --list
    git tag --list 'v*rc*' '*2.8*' --list -l --list -l --list

Now the documentation is more in tune with how the "branch" command describes its --list option since commit cddd127b9a ("branch: introduce --list option", 2011-08-28).

Change the test suite to assert that these invocations work for the cases that weren't already being tested for.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-tag.txt | 16 +++++++++-------
 t/t7004-tag.sh            | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 7 deletions(-)
Show changes to 2 files +30 −6

Documentation/git-tag.txt, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 525737a5d8..848e8c1b73 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -87,13 +87,15 @@ OPTIONS
 	If no number is given to `-n`, only the first line is printed.
 	If the tag is not annotated, the commit message is displayed instead.
 
--l <pattern>::
---list <pattern>::
-	List tags with names that match the given pattern (or all if no
-	pattern is given).  Running "git tag" without arguments also
-	lists all tags. The pattern is a shell wildcard (i.e., matched
-	using fnmatch(3)).  Multiple patterns may be given; if any of
-	them matches, the tag is shown.
+-l::
+--list::
+	Activate the list mode. `git tag <pattern>` would try to create a
+	tag, use `git tag --list <pattern>` to list matching branches, (or
+	all if no pattern is given).
++
+Running "git tag" without arguments also lists all tags. The pattern
+is a shell wildcard (i.e., matched using fnmatch(3)). Multiple
+patterns may be given; if any of them matches, the tag is shown.
 
 --sort=<key>::
 	Sort based on the key given.  Prefix `-` to sort in
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 74fc82a3c0..d36cd51fe2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '
 	git tag
 '
 
+cat >expect <<EOF
+mytag
+EOF
+test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
+	git tag -l -l >actual &&
+	test_cmp expect actual &&
+	git tag --list --list >actual &&
+	test_cmp expect actual &&
+	git tag --list -l --list >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'listing all tags if one exists should output that tag' '
 	test $(git tag -l) = mytag &&
 	test $(git tag) = mytag
@@ -336,6 +348,15 @@ test_expect_success 'tag -l can accept multiple patterns' '
 	test_cmp expect actual
 '
 
+test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '
+	git tag -l "v1*" "v0*" >actual &&
+	test_cmp expect actual &&
+	git tag -l "v1*" --list "v0*" >actual &&
+	test_cmp expect actual &&
+	git tag -l "v1*" "v0*" -l --list >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'listing tags in column' '
 	COLUMNS=40 git tag -l --column=row >actual &&
 	cat >expected <<\EOF &&
-- 
2.11.0
Junio C Hamano· Mar 18, 2017, 18:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 5 quoted lines
> However, documenting this as "-l <pattern>" was never correct, as
> these both worked before Jeff's change:
>
>     git tag -l 'v*'
>     git tag 'v*' -l

Actually, we do not particularly care about the latter, and quite honestly, I'd prefer we do not advertise and encourage the latter. Most Git commands take dashed options first and then non-dashed arguments, and so should "git tag". A more important example to show why "-l <pattern>" that pretends <pattern> is an argument to the option is wrong is this:

	git tag -l --merged X 'v*'
and this one
>     git tag --list 'v*rc*' '*2.8*'
Show 19 quoted lines
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 74fc82a3c0..d36cd51fe2 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '
>  	git tag
>  '
>  
> +cat >expect <<EOF
> +mytag
> +EOF
> +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
> +	git tag -l -l >actual &&
> +	test_cmp expect actual &&
> +	git tag --list --list >actual &&
> +	test_cmp expect actual &&
> +	git tag --list -l --list >actual &&
> +	test_cmp expect actual
> +'

OK. I do not care too deeply about this one, but somebody may want to tighten up the command line parsing to detect conflicting or duplicated cmdmode as an error in the future, and at that time this will require updating. I am not sure if we want to promise that giving multiple -l will keep working.

> +test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '
> +	git tag -l "v1*" "v0*" >actual &&
This is good thing to promise that we will keep it working.
Show 5 quoted lines
> +	test_cmp expect actual &&
> +	git tag -l "v1*" --list "v0*" >actual &&
> +	test_cmp expect actual &&
> +	git tag -l "v1*" "v0*" -l --list >actual &&
> +	test_cmp expect actual

I'd prefer we do *not* promise that it will keep working if you give pattern and then later any dashed-option like -l to the command by having tests like these.

Thanks.
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 19:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> However, documenting this as "-l <pattern>" was never correct, as
>> these both worked before Jeff's change:
>>
>>     git tag -l 'v*'
>>     git tag 'v*' -l
>
> Actually, we do not particularly care about the latter, and quite
> honestly, I'd prefer we do not advertise and encourage the latter.
> Most Git commands take dashed options first and then non-dashed
> arguments, and so should "git tag".  A more important example to
> show why "-l <pattern>" that pretends <pattern> is an argument to
> the option is wrong is this:

I for one do care about the latter in my CLI use. I.e. I'm fairly used to GNU-style getopt parsing where if you type "ls foo*" and forget the -l you don't have to "^Mb-l <RET>" to produce "ls -l foo*" as you would on the BSD's, you just type " -l<RET>".

I don't see any reason for why we'd force users to migrate to strict BSD-like getopt parsing for commands like tag/branch which accept these form of arguments, although one could argue that it's worth it for consistency with the likes of git-log might have better reasons to require it.

I.e. are there cases where we encounter genuine ambiguities in our option parsing because of this that don't involve the usual suspect of e.g. pattern that starts with "-" needing "<opts> -- <pattern>" to resolve the ambiguity?

As for this patch, I don't think accurately documenting an option like --list in terms of what it actually does is advertising and encouraging this use. All the examples are still of the form "git tag -l <pattern>" not "git tag <pattern> -l".

I think the main point of reference documentation like this should be to accurately and exhaustively document what something actually does. If we'd like to change it & deprecate some mode of operation in the future, fine, but surely the first step towards that is to document what the command does *now*.

You should be able to look at a git command, then read the documentation, and without having run the command or inspected the code be confident that you understand what the command will do when you run it.

Right now that isn't the case with "tag --list" at all, because it's documented as taking a pattern as an argument, but that isn't how it works.

Show 46 quoted lines
>         git tag -l --merged X 'v*'
>
> and this one
>
>>     git tag --list 'v*rc*' '*2.8*'
>
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index 74fc82a3c0..d36cd51fe2 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should succeed' '
>>       git tag
>>  '
>>
>> +cat >expect <<EOF
>> +mytag
>> +EOF
>> +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
>> +     git tag -l -l >actual &&
>> +     test_cmp expect actual &&
>> +     git tag --list --list >actual &&
>> +     test_cmp expect actual &&
>> +     git tag --list -l --list >actual &&
>> +     test_cmp expect actual
>> +'
>
> OK.  I do not care too deeply about this one, but somebody may want
> to tighten up the command line parsing to detect conflicting or
> duplicated cmdmode as an error in the future, and at that time this
> will require updating.  I am not sure if we want to promise that
> giving multiple -l will keep working.
>
>> +test_expect_success 'tag -l can accept multiple patterns interleaved with -l or --list options' '
>> +     git tag -l "v1*" "v0*" >actual &&
>
> This is good thing to promise that we will keep it working.
>
>> +     test_cmp expect actual &&
>> +     git tag -l "v1*" --list "v0*" >actual &&
>> +     test_cmp expect actual &&
>> +     git tag -l "v1*" "v0*" -l --list >actual &&
>> +     test_cmp expect actual
>
> I'd prefer we do *not* promise that it will keep working if you give
> pattern and then later any dashed-option like -l to the command by
> having tests like these.

To continue the above, I don't agree with this take on the issue at all. We should as much as possible aim for full coverage on our tests, just because something's tested for doesn't implicitly mean that there's a future promise that the functionality will always work that way, it's just testing for both intentional & unintentional regressions when the code is changed.

Then if we decide to e.g. change to stricter parsing or BSD-style parsing we'll hopefully have an exhaustive list of the cases we're changing.

It might make sense in cases where we're testing for a feature that might get deprecated in the future to have some test prefix for that, i.e. similar to "test_must_fail" but "test_might_get_deprecated" or whatever.

Although that might just as likely turn out to be a useless catalog of things we never actually end up changing, see e.g. that TODO test for the exit code of "git tag -l" which I removed at the start of this series.

Jeff King· Mar 20, 2017, 03:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

On Sat, Mar 18, 2017 at 11:43:47AM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> > +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
> > +	git tag -l -l >actual &&
> > +	test_cmp expect actual &&
> > +	git tag --list --list >actual &&
> > +	test_cmp expect actual &&
> > +	git tag --list -l --list >actual &&
> > +	test_cmp expect actual
> > +'
> 
> OK.  I do not care too deeply about this one, but somebody may want
> to tighten up the command line parsing to detect conflicting or
> duplicated cmdmode as an error in the future, and at that time this
> will require updating.  I am not sure if we want to promise that
> giving multiple -l will keep working.

I think it's expected to work under the usual last-one-wins option parsing. A more subtle case is that:

  git tag -l -d foo

would override "-l" with "-d". That's reasonable under the same rule as long as the user knows that the two are mode-selectors. I don't think we make that explicit in the documentation, though, so perhaps it isn't something users should rely on.

-Peff
Junio C Hamano· Mar 20, 2017, 15:55 UTC · re: Jeff King · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> I think it's expected to work under the usual last-one-wins option
> parsing. A more subtle case is that:
>
>   git tag -l -d foo
>
> would override "-l" with "-d". That's reasonable under the same rule as
> long as the user knows that the two are mode-selectors. I don't think we
> make that explicit in the documentation, though, so perhaps it isn't
> something users should rely on.
Yup, that is reasonable.
Junio C Hamano· Mar 20, 2017, 17:07 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> I think it's expected to work under the usual last-one-wins option
>> parsing. A more subtle case is that:
>>
>>   git tag -l -d foo
>>
>> would override "-l" with "-d". That's reasonable under the same rule as
>> long as the user knows that the two are mode-selectors. I don't think we
>> make that explicit in the documentation, though, so perhaps it isn't
>> something users should rely on.
>
> Yup, that is reasonable.

Oops. I meant "two are mode-selectors and can override with the usual last one wins rule" was reasonable. As Ævar says, this is an established practice and may already be documented sufficiently.

Ævar Arnfjörð Bjarmason· Mar 20, 2017, 16:09 UTC · re: Jeff King · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

On Mon, Mar 20, 2017 at 4:44 AM, Jeff King <peff@peff.net> wrote:
Show 26 quoted lines
> On Sat, Mar 18, 2017 at 11:43:47AM -0700, Junio C Hamano wrote:
>
>> > +test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '
>> > +   git tag -l -l >actual &&
>> > +   test_cmp expect actual &&
>> > +   git tag --list --list >actual &&
>> > +   test_cmp expect actual &&
>> > +   git tag --list -l --list >actual &&
>> > +   test_cmp expect actual
>> > +'
>>
>> OK.  I do not care too deeply about this one, but somebody may want
>> to tighten up the command line parsing to detect conflicting or
>> duplicated cmdmode as an error in the future, and at that time this
>> will require updating.  I am not sure if we want to promise that
>> giving multiple -l will keep working.
>
> I think it's expected to work under the usual last-one-wins option
> parsing. A more subtle case is that:
>
>   git tag -l -d foo
>
> would override "-l" with "-d". That's reasonable under the same rule as
> long as the user knows that the two are mode-selectors. I don't think we
> make that explicit in the documentation, though, so perhaps it isn't
> something users should rely on.

That hasn't been the case since v1.8.5 (v1.8.4-rc0-12-g1158826394). Now supplying multiple CMDMODE invocations will die.

It is the case that we still need to manually check any pseudo-cmdmode switches like "tag -a" (bool) v.s. "tag -l" (cmdmode). We check that particular combination, but we doubtless have bugs like that in other commands.

Jeff King· Mar 20, 2017, 16:11 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/8] tag: Change misleading --list <pattern> documentation

On Mon, Mar 20, 2017 at 05:09:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 17 quoted lines
> > I think it's expected to work under the usual last-one-wins option
> > parsing. A more subtle case is that:
> >
> >   git tag -l -d foo
> >
> > would override "-l" with "-d". That's reasonable under the same rule as
> > long as the user knows that the two are mode-selectors. I don't think we
> > make that explicit in the documentation, though, so perhaps it isn't
> > something users should rely on.
> 
> That hasn't been the case since v1.8.5 (v1.8.4-rc0-12-g1158826394).
> Now supplying multiple CMDMODE invocations will die.
> 
> It is the case that we still need to manually check any pseudo-cmdmode
> switches like "tag -a" (bool) v.s. "tag -l" (cmdmode). We check that
> particular combination, but we doubtless have bugs like that in other
> commands.

Yeah, you're right. I didn't look past the options[] array to see that we handled this specially. I guess there's nothing to document, then.

-Peff
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Change the "tag" command to implicitly turn on its --list mode when provided with a list-like option such as --contains, --points-at etc.

This is for consistency with how "branch" works. When "branch" is given a list-like option, such as --contains, it implicitly provides --list. Before this change "tag" would error out on those sorts of invocations. I.e. while both of these worked for "branch":

    git branch --contains v2.8.0 <pattern>
    git branch --list --contains v2.8.0 <pattern>
Only the latter form worked for "tag":
    git tag --contains v2.8.0 '*rc*'
    git tag --list --contains v2.8.0 '*rc*'

Now "tag", like "branch" will implicitly supply --list in when a list-like option is provided, and no other conflicting non-list options (such as -d) are present on the command-line.

Spelunking through the history via:
    git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'

Reveals that there was no good reason for not allowing this in the first place. The --contains option added in 32c35cfb1e ("git-tag: Add --contains option", 2009-01-26) made this an error, and all the other subsequent list-like options that were added copied its pattern of making this usage an error.

The only tests that break as a result of this change are tests that were explicitly checking that this "branch-like" usage wasn't permitted. Change those failing tests to check that this invocation mode is permitted, add extra tests for the list-like options we weren't testing, and add tests to ensure that e.g. we don't toggle the --list in the presence of other conflicting non-list options.

With this change errors messages such as "--contains option is only allowed with -l" don't make sense anymore, since options like --contain turn on -l. Instead we error out when list-like options such as --contain are used in conjunction with conflicting options such as -d or -v.

This change does not consider "-n" a list-like option, even though that might be logical. Permitting it would allow:

    git tag -n 100
As a synonym for:
    git tag -n --list 100

Which, while not technically ambiguous as the option must already be provided as -n<num> rather than -n <num>, would be confusing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-tag.txt | 10 +++++++---
 builtin/tag.c             | 25 +++++++++++++++----------
 t/t7004-tag.sh            | 39 +++++++++++++++++++++++++++++++++++----
 3 files changed, 57 insertions(+), 17 deletions(-)
Show changes to 3 files +57 −17

Documentation/git-tag.txt, builtin/tag.c, t/t7004-tag.sh

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 848e8c1b73..2acd3b6beb 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -96,6 +96,10 @@ OPTIONS
 Running "git tag" without arguments also lists all tags. The pattern
 is a shell wildcard (i.e., matched using fnmatch(3)). Multiple
 patterns may be given; if any of them matches, the tag is shown.
++
+This option is implicitly supplied if any other list-like option such
+as `--contains` is provided. See the documentation for each of those
+options for details.
 
 --sort=<key>::
 	Sort based on the key given.  Prefix `-` to sort in
@@ -124,10 +128,10 @@ This option is only applicable when listing tags without annotation lines.
 
 --contains [<commit>]::
 	Only list tags which contain the specified commit (HEAD if not
-	specified).
+	specified). Implies `--list`.
 
 --points-at <object>::
-	Only list tags of the given object.
+	Only list tags of the given object. Implies `--list`.
 
 -m <msg>::
 --message=<msg>::
@@ -178,7 +182,7 @@ This option is only applicable when listing tags without annotation lines.
 --[no-]merged [<commit>]::
 	Only list tags whose tips are reachable, or not reachable
 	if `--no-merged` is used, from the specified commit (`HEAD`
-	if not specified).
+	if not specified). Implies `--list`.
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 0bba3fd070..3483636e59 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -454,8 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
 
-	if (argc == 0 && !cmdmode && !create_tag_object)
-		cmdmode = 'l';
+	if (!cmdmode && !create_tag_object) {
+		if (argc == 0)
+			cmdmode = 'l';
+		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
+			cmdmode = 'l';
+	}
 
 	if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
 		usage_with_options(git_tag_usage, options);
@@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
+	} else {
+		if (filter.lines != -1)
+			die(_("-n option is only allowed in list mode."));
+		if (filter.with_commit)
+			die(_("--contains option is only allowed in list mode."));
+		if (filter.points_at.nr)
+			die(_("--points-at option is only allowed in list mode."));
+		if (filter.merge_commit)
+			die(_("--merged and --no-merged options are only allowed in list mode."));
 	}
-	if (filter.lines != -1)
-		die(_("-n option is only allowed with -l."));
-	if (filter.with_commit)
-		die(_("--contains option is only allowed with -l."));
-	if (filter.points_at.nr)
-		die(_("--points-at option is only allowed with -l."));
-	if (filter.merge_commit)
-		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
 	if (cmdmode == 'v') {
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index d36cd51fe2..5c94932f0f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1472,6 +1472,11 @@ test_expect_success 'checking that initial commit is in all tags' "
 	test_cmp expected actual
 "
 
+test_expect_success 'checking that --contains can be used in non-list mode' '
+	git tag --contains $hash1 v* >actual &&
+	test_cmp expected actual
+'
+
 # mixing modes and options:
 
 test_expect_success 'mixing incompatibles modes and options is forbidden' '
@@ -1491,15 +1496,31 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -a -s -m -F -l &&
 	test_must_fail git tag -l -v &&
 	test_must_fail git tag -n 100 &&
+	test_must_fail git tag -n 100 -v &&
 	test_must_fail git tag -l -m msg &&
 	test_must_fail git tag -l -F some file &&
 	test_must_fail git tag -v -s
 '
 
+for option in --contains --merged --no-merged --points-at
+do
+	test_expect_success "mixing incompatible modes with $option is forbidden" "
+		test_must_fail git tag -d $option HEAD &&
+		test_must_fail git tag -d $option HEAD some-tag &&
+		test_must_fail git tag -v $option HEAD
+	"
+	test_expect_success "Doing 'git tag --list-like $option <commit> <pattern> is permitted" "
+		git tag -n $option HEAD HEAD &&
+		git tag $option HEAD HEAD
+	"
+done
+
 # check points-at
 
-test_expect_success '--points-at cannot be used in non-list mode' '
-	test_must_fail git tag --points-at=v4.0 foo
+test_expect_success '--points-at can be used in non-list mode' '
+	echo v4.0 >expect &&
+	git tag --points-at=v4.0 "v*" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--points-at finds lightweight tags' '
@@ -1776,8 +1797,13 @@ test_expect_success 'setup --merged test tags' '
 	git tag mergetest-3 HEAD
 '
 
-test_expect_success '--merged cannot be used in non-list mode' '
-	test_must_fail git tag --merged=mergetest-2 foo
+test_expect_success '--merged can be used in non-list mode' '
+	cat >expect <<-\EOF &&
+	mergetest-1
+	mergetest-2
+	EOF
+	git tag --merged=mergetest-2 "mergetest*" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--merged shows merged tags' '
@@ -1797,6 +1823,11 @@ test_expect_success '--no-merged show unmerged tags' '
 	test_cmp expect actual
 '
 
+test_expect_success '--no-merged can be used in non-list mode' '
+	git tag --no-merged=mergetest-2 mergetest-* >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ambiguous branch/tags not marked' '
 	git tag ambiguous &&
 	git branch ambiguous &&
-- 
2.11.0
Jeff King· Mar 20, 2017, 03:55 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Sat, Mar 18, 2017 at 10:32:52AM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> With this change errors messages such as "--contains option is only
> allowed with -l" don't make sense anymore, since options like
> --contain turn on -l. Instead we error out when list-like options such
> as --contain are used in conjunction with conflicting options such as
> -d or -v.
Yeah, I think this is the right approach.
Show 11 quoted lines
> This change does not consider "-n" a list-like option, even though
> that might be logical. Permitting it would allow:
> 
>     git tag -n 100
> 
> As a synonym for:
> 
>     git tag -n --list 100
> 
> Which, while not technically ambiguous as the option must already be
> provided as -n<num> rather than -n <num>, would be confusing.

I'm not sure the existing behavior isn't confusing anyway (most optional arguments are). But I don't mind being conservative and leaving out "-n" for now; we can always convert it later if somebody feels strongly about it.

Show 16 quoted lines
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 0bba3fd070..3483636e59 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,8 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>  
> -	if (argc == 0 && !cmdmode && !create_tag_object)
> -		cmdmode = 'l';
> +	if (!cmdmode && !create_tag_object) {
> +		if (argc == 0)
> +			cmdmode = 'l';
> +		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
> +			cmdmode = 'l';
> +	}
Makes sense.
Show 22 quoted lines
> @@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		if (column_active(colopts))
>  			stop_column_filter();
>  		return ret;
> +	} else {
> +		if (filter.lines != -1)
> +			die(_("-n option is only allowed in list mode."));
> +		if (filter.with_commit)
> +			die(_("--contains option is only allowed in list mode."));
> +		if (filter.points_at.nr)
> +			die(_("--points-at option is only allowed in list mode."));
> +		if (filter.merge_commit)
> +			die(_("--merged and --no-merged options are only allowed in list mode."));
>  	}
> -	if (filter.lines != -1)
> -		die(_("-n option is only allowed with -l."));
> -	if (filter.with_commit)
> -		die(_("--contains option is only allowed with -l."));
> -	if (filter.points_at.nr)
> -		die(_("--points-at option is only allowed with -l."));
> -	if (filter.merge_commit)
> -		die(_("--merged and --no-merged option are only allowed with -l"));

I'm not sure why these go into the "else" clause here. The other side of the conditional (i.e., when we are in list mode) always returns. I don't _mind_ it, it's just surprising in this patch.

While we are re-wording the messages, we may want to drop the periods at the end of the first three (or keep it on the fourth one, but I our usual style is to omit it).

> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
The tests looked reasonable to me.
-Peff
Ævar Arnfjörð Bjarmason· Mar 20, 2017, 09:16 UTC · re: Jeff King · lore

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

On Mon, Mar 20, 2017 at 4:55 AM, Jeff King <peff@peff.net> wrote:
Show 72 quoted lines
> On Sat, Mar 18, 2017 at 10:32:52AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> With this change errors messages such as "--contains option is only
>> allowed with -l" don't make sense anymore, since options like
>> --contain turn on -l. Instead we error out when list-like options such
>> as --contain are used in conjunction with conflicting options such as
>> -d or -v.
>
> Yeah, I think this is the right approach.
>
>> This change does not consider "-n" a list-like option, even though
>> that might be logical. Permitting it would allow:
>>
>>     git tag -n 100
>>
>> As a synonym for:
>>
>>     git tag -n --list 100
>>
>> Which, while not technically ambiguous as the option must already be
>> provided as -n<num> rather than -n <num>, would be confusing.
>
> I'm not sure the existing behavior isn't confusing anyway (most optional
> arguments are). But I don't mind being conservative and leaving out "-n"
> for now; we can always convert it later if somebody feels strongly about
> it.
>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 0bba3fd070..3483636e59 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -454,8 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       }
>>       create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>>
>> -     if (argc == 0 && !cmdmode && !create_tag_object)
>> -             cmdmode = 'l';
>> +     if (!cmdmode && !create_tag_object) {
>> +             if (argc == 0)
>> +                     cmdmode = 'l';
>> +             else if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
>> +                     cmdmode = 'l';
>> +     }
>
> Makes sense.
>
>> @@ -483,15 +487,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>               if (column_active(colopts))
>>                       stop_column_filter();
>>               return ret;
>> +     } else {
>> +             if (filter.lines != -1)
>> +                     die(_("-n option is only allowed in list mode."));
>> +             if (filter.with_commit)
>> +                     die(_("--contains option is only allowed in list mode."));
>> +             if (filter.points_at.nr)
>> +                     die(_("--points-at option is only allowed in list mode."));
>> +             if (filter.merge_commit)
>> +                     die(_("--merged and --no-merged options are only allowed in list mode."));
>>       }
>> -     if (filter.lines != -1)
>> -             die(_("-n option is only allowed with -l."));
>> -     if (filter.with_commit)
>> -             die(_("--contains option is only allowed with -l."));
>> -     if (filter.points_at.nr)
>> -             die(_("--points-at option is only allowed with -l."));
>> -     if (filter.merge_commit)
>> -             die(_("--merged and --no-merged option are only allowed with -l"));
>
> I'm not sure why these go into the "else" clause here. The other side of
> the conditional (i.e., when we are in list mode) always returns. I don't
> _mind_ it, it's just surprising in this patch.

Changed it. It was a wart left over from an earlier version of this that didn't make it on-list where the else mattered.

> While we are re-wording the messages, we may want to drop the periods at
> the end of the first three (or keep it on the fourth one, but I our
> usual style is to omit it).
Done.
Show 6 quoted lines
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>
> The tests looked reasonable to me.
>
> -Peff
Ævar Arnfjörð Bjarmason· Mar 18, 2017, 10:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

Change the tag, branch & for-each-ref commands to have a --no-contains option in addition to their longstanding --contains options.

This allows for finding the last-good rollout tag given a known-bad <commit>. Given a hypothetically bad commit cf5c7253e0 the git version revert to can be found with this hacky two-liner:

    (git tag -l 'v[0-9]*'; git tag -l --contains cf5c7253e0 'v[0-9]*') |
        sort | uniq -c | grep -E '^ *1 ' | awk '{print $2}' | tail -n 10
With this new --no-contains the same can be achieved with:
    git tag -l --no-contains cf5c7253e0 'v[0-9]*' | sort | tail -n 10

As the filtering machinery is shared between the tag, branch & for-each-ref commands, implement this for those commands too. A practical use for this with "branch" is e.g. finding branches which diverged between v2.8 & v2.10:

    git branch --contains v2.8.0 --no-contains v2.10.0

The "describe" command also has a --contains option, but its semantics are unrelated to what tag/branch/for-each-ref use --contains for. A --no-contains option for "describe" wouldn't make any sense, other than being exactly equivalent to not supplying --contains at all, which would be confusing at best.

Add a --without option to "tag" as an alias for --no-contains, for consistency with --with and --contains. The --with option is undocumented, and possibly the only user of it is Junio (<xmqqefy71iej.fsf@gitster.mtv.corp.google.com>). But it's trivial to support, so let's do that.

The additions to the the test suite are inverse copies of the corresponding --contains tests. With this change --no-contains for tag, branch & for-each-ref is just as well tested as the existing --contains option.

In addition to those tests, add a test for "tag" which asserts that --no-contains won't find tree/blob tags, which is slightly unintuitive, but consistent with how --contains works & is documented.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-branch.txt           |  15 ++--
 Documentation/git-for-each-ref.txt     |   6 +-
 Documentation/git-tag.txt              |   6 +-
 builtin/branch.c                       |   4 +-
 builtin/for-each-ref.c                 |   3 +-
 builtin/tag.c                          |   8 ++-
 contrib/completion/git-completion.bash |   4 +-
 parse-options.h                        |   4 +-
 ref-filter.c                           |  19 +++--
 ref-filter.h                           |   1 +
 t/t3201-branch-contains.sh             |  51 ++++++++++++-
 t/t6302-for-each-ref-filter.sh         |  16 +++++
 t/t7004-tag.sh                         | 128 +++++++++++++++++++++++++++++++--
 13 files changed, 240 insertions(+), 25 deletions(-)
Show changes to 13 files +240 −25

Documentation/git-branch.txt, Documentation/git-for-each-ref.txt, Documentation/git-tag.txt, builtin/branch.c, builtin/for-each-ref.c, builtin/tag.c, contrib/completion/git-completion.bash, parse-options.h, ref-filter.c, ref-filter.h, t/t3201-branch-contains.sh, t/t6302-for-each-ref-filter.sh, t/t7004-tag.sh

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 092f1bcf9f..5e52fc9b91 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
-	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
+	[(--merged | --no-merged | --contains | --no-contains) [<commit>]] [--sort=<key>]
 	[--points-at <object>] [--format=<format>] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
@@ -35,7 +35,7 @@ as branch creation.
 
 With `--contains`, shows only the branches that contain the named commit
 (in other words, the branches whose tip commits are descendants of the
-named commit).  With `--merged`, only branches merged into the named
+named commit), `--no-contains` inverts it. With `--merged`, only branches merged into the named
 commit (i.e. the branches whose tip commits are reachable from the named
 commit) will be listed.  With `--no-merged` only branches not merged into
 the named commit will be listed.  If the <commit> argument is missing it
@@ -213,6 +213,10 @@ start-point is either a local or remote-tracking branch.
 	Only list branches which contain the specified commit (HEAD
 	if not specified). Implies `--list`.
 
+--no-contains [<commit>]::
+	Only list branches which don't contain the specified commit
+	(HEAD if not specified). Implies `--list`.
+
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
 	specified commit (HEAD if not specified). Implies `--list`.
@@ -296,13 +300,16 @@ If you are creating a branch that you want to checkout immediately, it is
 easier to use the git checkout command with its `-b` option to create
 a branch and check it out with a single command.
 
-The options `--contains`, `--merged` and `--no-merged` serve three related
-but different purposes:
+The options `--contains`, `--no-contains`, `--merged` and `--no-merged`
+serve four related but different purposes:
 
 - `--contains <commit>` is used to find all branches which will need
   special attention if <commit> were to be rebased or amended, since those
   branches contain the specified <commit>.
 
+- `--no-contains <commit>` is the inverse of that, i.e. branches that don't
+  contain the specified <commit>.
+
 - `--merged` is used to find all branches which can be safely deleted,
   since those branches are fully contained by HEAD.
 
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 111e1be6f5..83b93c75a8 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
 		   [--points-at <object>] [(--merged | --no-merged) [<object>]]
-		   [--contains [<object>]]
+		   [(--contains | --no-contains) [<object>]]
 
 DESCRIPTION
 -----------
@@ -79,6 +79,10 @@ OPTIONS
 	Only list refs which contain the specified commit (HEAD if not
 	specified).
 
+--no-contains [<object>]::
+	Only list refs which don't contain the specified commit (HEAD
+	if not specified).
+
 --ignore-case::
 	Sorting and filtering refs are case insensitive.
 
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index e7793afad1..d0b506f120 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <keyid>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
+'git tag' [-n[<num>]] -l [--[no-]contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
 	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
 'git tag' -v [--format=<format>] <tagname>...
@@ -131,6 +131,10 @@ This option is only applicable when listing tags without annotation lines.
 	Only list tags which contain the specified commit (HEAD if not
 	specified). Implies `--list`.
 
+--no-contains [<commit>]::
+	Only list tags which don't contain the specified commit (HEAD if
+	not specified). Implies `--list`.
+
 --points-at <object>::
 	Only list tags of the given object. Implies `--list`.
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 94f7de7fa5..dd96319726 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -548,7 +548,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT('r', "remotes",     &filter.kind, N_("act on remote-tracking branches"),
 			FILTER_REFS_REMOTES),
 		OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit, N_("print only branches that don't contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT_WITHOUT(&filter.no_commit, N_("print only branches that don't contain the commit")),
 		OPT__ABBREV(&filter.abbrev),
 
 		OPT_GROUP(N_("Specific git-branch actions:")),
@@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
+	if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!new_upstream +
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index df41fa0350..a11542c4fd 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
 	N_("git for-each-ref [--points-at <object>]"),
 	N_("git for-each-ref [(--merged | --no-merged) [<object>]]"),
-	N_("git for-each-ref [--contains [<object>]]"),
+	N_("git for-each-ref [(--contains | --no-contains) [<object>]]"),
 	NULL
 };
 
@@ -43,6 +43,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only refs that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only refs that are not merged")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only refs which contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
 		OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_END(),
 	};
diff --git a/builtin/tag.c b/builtin/tag.c
index 2da28a5ce6..f91ae171b7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -22,7 +22,7 @@
 static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
-	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
+	N_("git tag -l [-n[<num>]] [--[no-]contains <commit>] [--points-at <object>]"
 		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
@@ -424,7 +424,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_NO_CONTAINS(&filter.no_commit, N_("print only tags that don't contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
 		OPT_MERGED(&filter, N_("print only tags that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
@@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!cmdmode && !create_tag_object) {
 		if (argc == 0)
 			cmdmode = 'l';
-		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
+		else if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
@@ -492,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			die(_("-n option is only allowed in list mode."));
 		if (filter.with_commit)
 			die(_("--contains option is only allowed in list mode."));
+		if (filter.no_commit)
+			die(_("--no-contains option is only allowed in list mode."));
 		if (filter.points_at.nr)
 			die(_("--points-at option is only allowed in list mode."));
 		if (filter.merge_commit)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fc32286a43..ec8fce5820 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1093,7 +1093,7 @@ _git_branch ()
 	--*)
 		__gitcomp "
 			--color --no-color --verbose --abbrev= --no-abbrev
-			--track --no-track --contains --merged --no-merged
+			--track --no-track --contains --no-contains --merged --no-merged
 			--set-upstream-to= --edit-description --list
 			--unset-upstream --delete --move --remotes
 			--column --no-column --sort= --points-at
@@ -2862,7 +2862,7 @@ _git_tag ()
 		__gitcomp "
 			--list --delete --verify --annotate --message --file
 			--sign --cleanup --local-user --force --column --sort=
-			--contains --points-at --merged --no-merged --create-reflog
+			--contains --no-contains --points-at --merged --no-merged --create-reflog
 			"
 		;;
 	esac
diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..0eac90b510 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -258,7 +258,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
 	  PARSE_OPT_LASTARG_DEFAULT | flag, \
 	  parse_opt_commits, (intptr_t) "HEAD" \
 	}
-#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
+#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
+#define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
 #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN)
+#define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH("without", v, h, PARSE_OPT_HIDDEN)
 
 #endif
diff --git a/ref-filter.c b/ref-filter.c
index 7eeecc608f..092331fd81 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1487,6 +1487,7 @@ struct ref_filter_cbdata {
 	struct ref_array *array;
 	struct ref_filter *filter;
 	struct contains_cache contains_cache;
+	struct contains_cache no_contains_cache;
 };
 
 /*
@@ -1586,11 +1587,11 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
 }
 
 static int commit_contains(struct ref_filter *filter, struct commit *commit,
-			   struct contains_cache *cache)
+			   struct commit_list *list, struct contains_cache *cache)
 {
 	if (filter->with_commit_tag_algo)
-		return contains_tag_algo(commit, filter->with_commit, cache) == CONTAINS_YES;
-	return is_descendant_of(commit, filter->with_commit);
+		return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
+	return is_descendant_of(commit, list);
 }
 
 /*
@@ -1780,13 +1781,17 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit || filter->verbose) {
+	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
-		/* We perform the filtering for the '--contains' option */
+		/* We perform the filtering for the '--contains' option... */
 		if (filter->with_commit &&
-		    !commit_contains(filter, commit, &ref_cbdata->contains_cache))
+		    !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
+			return 0;
+		/* ...or for the `--no-contains' option */
+		if (filter->no_commit &&
+		    commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
 			return 0;
 	}
 
@@ -1887,6 +1892,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	filter->kind = type & FILTER_REFS_KIND_MASK;
 
 	init_contains_cache(&ref_cbdata.contains_cache);
+	init_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Simple per-ref filtering */
 	if (!filter->kind)
@@ -1911,6 +1917,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	}
 
 	clear_contains_cache(&ref_cbdata.contains_cache);
+	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
 	if (filter->merge_commit)
diff --git a/ref-filter.h b/ref-filter.h
index e738c5dfd3..dde40f6849 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -53,6 +53,7 @@ struct ref_filter {
 	const char **name_patterns;
 	struct sha1_array points_at;
 	struct commit_list *with_commit;
+	struct commit_list *no_commit;
 
 	enum {
 		REF_FILTER_MERGED_NONE = 0,
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 7f3ec47241..506415dbd3 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='branch --contains <commit>, --merged, and --no-merged'
+test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged'
 
 . ./test-lib.sh
 
@@ -45,6 +45,22 @@ test_expect_success 'branch --contains master' '
 
 '
 
+test_expect_success 'branch --no-contains=master' '
+
+	git branch --no-contains=master >actual &&
+	>expect &&
+	test_cmp expect actual
+
+'
+
+test_expect_success 'branch --no-contains master' '
+
+	git branch --no-contains master >actual &&
+	>expect &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'branch --contains=side' '
 
 	git branch --contains=side >actual &&
@@ -55,6 +71,16 @@ test_expect_success 'branch --contains=side' '
 
 '
 
+test_expect_success 'branch --no-contains=side' '
+
+	git branch --no-contains=side >actual &&
+	{
+		echo "  master"
+	} >expect &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'branch --contains with pattern implies --list' '
 
 	git branch --contains=master master >actual &&
@@ -65,6 +91,14 @@ test_expect_success 'branch --contains with pattern implies --list' '
 
 '
 
+test_expect_success 'branch --no-contains with pattern implies --list' '
+
+	git branch --no-contains=master master >actual &&
+	>expect &&
+	test_cmp expect actual
+
+'
+
 test_expect_success 'side: branch --merged' '
 
 	git branch --merged >actual &&
@@ -126,7 +160,9 @@ test_expect_success 'branch --no-merged with pattern implies --list' '
 test_expect_success 'implicit --list conflicts with modification options' '
 
 	test_must_fail git branch --contains=master -d &&
-	test_must_fail git branch --contains=master -m foo
+	test_must_fail git branch --contains=master -m foo &&
+	test_must_fail git branch --no-contains=master -d &&
+	test_must_fail git branch --no-contains=master -m foo
 
 '
 
@@ -159,4 +195,15 @@ test_expect_success 'branch --merged with --verbose' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'branch --contains combined with --no-contains' '
+	git branch --contains zzz --no-contains topic >actual &&
+	cat >expect <<-\EOF &&
+	  master
+	  side
+	  zzz
+	EOF
+	test_cmp expect actual
+
+'
+
 test_done
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index a09a1a46ef..4902ba5f16 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -93,6 +93,22 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'filtering with --no-contains' '
+	cat >expect <<-\EOF &&
+	refs/tags/one
+	EOF
+	git for-each-ref --format="%(refname)" --no-contains=two >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'filtering with --contains and --no-contains' '
+	cat >expect <<-\EOF &&
+	refs/tags/two
+	EOF
+	git for-each-ref --format="%(refname)" --contains=two --no-contains=three >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(color) must fail' '
 	test_must_fail git for-each-ref --format="%(color)%(refname)"
 '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index ba1ab1f21c..428e21c369 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1420,6 +1420,23 @@ test_expect_success 'checking that first commit is in all tags (relative)' "
 	test_cmp expected actual
 "
 
+# All the --contains tests above, but with --no-contains
+test_expect_success 'checking that first commit is not listed in any tag with --no-contains  (hash)' "
+	>expected &&
+	git tag -l --no-contains $hash1 v* >actual &&
+	test_cmp expected actual
+"
+
+test_expect_success 'checking that first commit is in all tags (tag)' "
+	git tag -l --no-contains v1.0 v* >actual &&
+	test_cmp expected actual
+"
+
+test_expect_success 'checking that first commit is in all tags (relative)' "
+	git tag -l --no-contains HEAD~2 v* >actual &&
+	test_cmp expected actual
+"
+
 cat > expected <<EOF
 v2.0
 EOF
@@ -1429,6 +1446,17 @@ test_expect_success 'checking that second commit only has one tag' "
 	test_cmp expected actual
 "
 
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+EOF
+
+test_expect_success 'inverse of the last test, with --no-contains' "
+	git tag -l --no-contains $hash2 v* >actual &&
+	test_cmp expected actual
+"
 
 cat > expected <<EOF
 EOF
@@ -1438,6 +1466,19 @@ test_expect_success 'checking that third commit has no tags' "
 	test_cmp expected actual
 "
 
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+EOF
+
+test_expect_success 'conversely --no-contains on the third commit lists all tags' "
+	git tag -l --no-contains $hash3 v* >actual &&
+	test_cmp expected actual
+"
+
 # how about a simple merge?
 
 test_expect_success 'creating simple branch' '
@@ -1459,6 +1500,19 @@ test_expect_success 'checking that branch head only has one tag' "
 	test_cmp expected actual
 "
 
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+EOF
+
+test_expect_success 'checking that branch head with --no-contains lists all but one tag' "
+	git tag -l --no-contains $hash4 v* >actual &&
+	test_cmp expected actual
+"
+
 test_expect_success 'merging original branch into this branch' '
 	git merge --strategy=ours master &&
         git tag v4.0
@@ -1480,6 +1534,20 @@ v1.0.1
 v1.1.3
 v2.0
 v3.0
+EOF
+
+test_expect_success 'checking that original branch head with --no-contains lists all but one tag now' "
+	git tag -l --no-contains $hash3 v* >actual &&
+	test_cmp expected actual
+"
+
+cat > expected <<EOF
+v0.2.1
+v1.0
+v1.0.1
+v1.1.3
+v2.0
+v3.0
 v4.0
 EOF
 
@@ -1493,6 +1561,12 @@ test_expect_success 'checking that --contains can be used in non-list mode' '
 	test_cmp expected actual
 '
 
+test_expect_success 'checking that initial commit is in all tags with --no-contains' "
+	>expected &&
+	git tag -l --no-contains $hash1 v* >actual &&
+	test_cmp expected actual
+"
+
 # mixing modes and options:
 
 test_expect_success 'mixing incompatibles modes and options is forbidden' '
@@ -1514,10 +1588,11 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 	test_must_fail git tag -n 100 -v &&
 	test_must_fail git tag -l -m msg &&
 	test_must_fail git tag -l -F some file &&
-	test_must_fail git tag -v -s
+	test_must_fail git tag -v -s &&
+	test_must_fail git tag --contains --no-contains
 '
 
-for option in --contains --merged --no-merged --points-at
+for option in --contains --no-contains --merged --no-merged --points-at
 do
 	test_expect_success "mixing incompatible modes with $option is forbidden" "
 		test_must_fail git tag -d $option HEAD &&
@@ -1777,7 +1852,7 @@ run_with_limited_stack () {
 test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
 
 # we require ulimit, this excludes Windows
-test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a deep repo' '
 	>expect &&
 	i=1 &&
 	while test $i -lt 8000
@@ -1793,7 +1868,9 @@ EOF"
 	git checkout master &&
 	git tag far-far-away HEAD^ &&
 	run_with_limited_stack git tag --contains HEAD >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	run_with_limited_stack git tag --no-contains HEAD >actual &&
+	test_line_count ">" 10 actual
 '
 
 test_expect_success '--format should list tags as per format given' '
@@ -1851,4 +1928,47 @@ test_expect_success 'ambiguous branch/tags not marked' '
 	test_cmp expect actual
 '
 
+test_expect_success '--contains combined with --no-contains' '
+	(
+		git init no-contains &&
+		cd no-contains &&
+		test_commit v0.1 &&
+		test_commit v0.2 &&
+		test_commit v0.3 &&
+		test_commit v0.4 &&
+		test_commit v0.5 &&
+		cat >expected <<-\EOF &&
+		v0.2
+		v0.3
+		v0.4
+		EOF
+		git tag --contains v0.2 --no-contains v0.5 >actual &&
+		test_cmp expected actual
+	)
+'
+
+# As the docs say, list tags which contain a specified *commit*. We
+# don't recurse down to tags for trees or blobs pointed to by *those*
+# commits.
+test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
+	cd no-contains &&
+	blob=$(git rev-parse v0.3:v0.3.t) &&
+	tree=$(git rev-parse v0.3^{tree}) &&
+	git tag tag-blob $blob &&
+	git tag tag-tree $tree &&
+	git tag --contains v0.3 >actual &&
+	cat >expected <<-\EOF &&
+	v0.3
+	v0.4
+	v0.5
+	EOF
+	test_cmp expected actual &&
+	git tag --no-contains v0.3 >actual &&
+	cat >expected <<-\EOF &&
+	v0.1
+	v0.2
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.11.0
Jeff King· Mar 20, 2017, 04:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

On Sat, Mar 18, 2017 at 10:32:54AM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> Change the tag, branch & for-each-ref commands to have a --no-contains
> option in addition to their longstanding --contains options.
> 
> This allows for finding the last-good rollout tag given a known-bad
> <commit>. Given a hypothetically bad commit cf5c7253e0 the git version
> revert to can be found with this hacky two-liner:
s/revert to/to &/, I think.
> With this new --no-contains the same can be achieved with:
> [..]
The goal sounds good to me.
> In addition to those tests, add a test for "tag" which asserts that
> --no-contains won't find tree/blob tags, which is slightly
> unintuitive, but consistent with how --contains works & is documented.

Makes sense. In theory we could dig into commits to find trees and blobs when the user gives us one. But I kind of doubt anybody really wants it, and it's expensive to compute. For the simple cases, --points-at already does the right thing.

[more on that below, though...]
Show 7 quoted lines
> @@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
>  		list = 1;
>  
> -	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
> +	if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>  		list = 1;
Could we wrap this long conditional?
Show 11 quoted lines
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index df41fa0350..a11542c4fd 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {
>  	N_("git for-each-ref [<options>] [<pattern>]"),
>  	N_("git for-each-ref [--points-at <object>]"),
>  	N_("git for-each-ref [(--merged | --no-merged) [<object>]]"),
> -	N_("git for-each-ref [--contains [<object>]]"),
> +	N_("git for-each-ref [(--contains | --no-contains) [<object>]]"),
>  	NULL

I'm not sure if this presentation implies that the two cannot be used together. It copies "--merged/--no-merged", but I think those two _can't_ be used together (it probably wouldn't be hard to make it work, but if nobody cares it may not be worth spending time on).

I also wonder if we need to explicitly document that --contains and --no-contains can be used together and don't cancel each other. The other option is to pick a new name ("--omits" is the most concise one I could think of; maybe that is preferable anyway because it avoids negation).

Show 6 quoted lines
> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (!cmdmode && !create_tag_object) {
>  		if (argc == 0)
>  			cmdmode = 'l';
> -		else if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
> +		else if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)

Ditto here on the wrapping. There were a few other long lines, but I won't point them all out.

Show 10 quoted lines
> -		/* We perform the filtering for the '--contains' option */
> +		/* We perform the filtering for the '--contains' option... */
>  		if (filter->with_commit &&
> -		    !commit_contains(filter, commit, &ref_cbdata->contains_cache))
> +		    !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
> +			return 0;
> +		/* ...or for the `--no-contains' option */
> +		if (filter->no_commit &&
> +		    commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
>  			return 0;
This looks nice and simple. Good.
Show 23 quoted lines
> +# As the docs say, list tags which contain a specified *commit*. We
> +# don't recurse down to tags for trees or blobs pointed to by *those*
> +# commits.
> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
> +	cd no-contains &&
> +	blob=$(git rev-parse v0.3:v0.3.t) &&
> +	tree=$(git rev-parse v0.3^{tree}) &&
> +	git tag tag-blob $blob &&
> +	git tag tag-tree $tree &&
> +	git tag --contains v0.3 >actual &&
> +	cat >expected <<-\EOF &&
> +	v0.3
> +	v0.4
> +	v0.5
> +	EOF
> +	test_cmp expected actual &&
> +	git tag --no-contains v0.3 >actual &&
> +	cat >expected <<-\EOF &&
> +	v0.1
> +	v0.2
> +	EOF
> +	test_cmp expected actual
> +'

The tests mostly look fine, but this one puzzled me. I guess we're checking that tag-blob does not contain v0.3. But how could it?

The more interesting test to me is:
  git tag --contains $blob
which should barf on a non-commit.

For the --no-contains side, you could argue that the blob-tag doesn't contain the commit, and it should be listed. But it looks like we just drop all non-commit tags completely as soon as we start to do a contains/not-contains traversal.

I think the more relevant comparison is "--no-merged", and it behaves the same way as your new --no-contains. I don't think I saw this subtlety in the documentation, though. It might be worth mentioning (unless I just missed it).

-Peff
Ævar Arnfjörð Bjarmason· Mar 20, 2017, 09:32 UTC · re: Jeff King · lore

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

On Mon, Mar 20, 2017 at 5:25 AM, Jeff King <peff@peff.net> wrote:
Show 10 quoted lines
> On Sat, Mar 18, 2017 at 10:32:54AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the tag, branch & for-each-ref commands to have a --no-contains
>> option in addition to their longstanding --contains options.
>>
>> This allows for finding the last-good rollout tag given a known-bad
>> <commit>. Given a hypothetically bad commit cf5c7253e0 the git version
>> revert to can be found with this hacky two-liner:
>
> s/revert to/to &/, I think.
Done.
Show 26 quoted lines
>
>> With this new --no-contains the same can be achieved with:
>> [..]
>
> The goal sounds good to me.
>
>> In addition to those tests, add a test for "tag" which asserts that
>> --no-contains won't find tree/blob tags, which is slightly
>> unintuitive, but consistent with how --contains works & is documented.
>
> Makes sense. In theory we could dig into commits to find trees and blobs
> when the user gives us one. But I kind of doubt anybody really wants it,
> and it's expensive to compute. For the simple cases, --points-at already
> does the right thing.
>
> [more on that below, though...]
>
>> @@ -604,7 +606,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>       if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
>>               list = 1;
>>
>> -     if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>> +     if (filter.with_commit || filter.no_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>>               list = 1;
>
> Could we wrap this long conditional?
Done. Will also go through the series as a whole & find other such occurances.
Show 16 quoted lines
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index df41fa0350..a11542c4fd 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -9,7 +9,7 @@ static char const * const for_each_ref_usage[] = {
>>       N_("git for-each-ref [<options>] [<pattern>]"),
>>       N_("git for-each-ref [--points-at <object>]"),
>>       N_("git for-each-ref [(--merged | --no-merged) [<object>]]"),
>> -     N_("git for-each-ref [--contains [<object>]]"),
>> +     N_("git for-each-ref [(--contains | --no-contains) [<object>]]"),
>>       NULL
>
> I'm not sure if this presentation implies that the two cannot be used
> together. It copies "--merged/--no-merged", but I think those two
> _can't_ be used together (it probably wouldn't be hard to make it work,
> but if nobody cares it may not be worth spending time on).

Yeah this has been bothering me a bit too. I'll change the various --help and synopsis entries to split them up, since they're not mutually exclusive at all.

Show 55 quoted lines
> I also wonder if we need to explicitly document that --contains and
> --no-contains can be used together and don't cancel each other. The
> other option is to pick a new name ("--omits" is the most concise one I
> could think of; maybe that is preferable anyway because it avoids
> negation).
>
>> @@ -457,7 +459,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (!cmdmode && !create_tag_object) {
>>               if (argc == 0)
>>                       cmdmode = 'l';
>> -             else if (filter.with_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
>> +             else if (filter.with_commit || filter.no_commit || filter.points_at.nr || filter.merge_commit || filter.lines != -1)
>
> Ditto here on the wrapping. There were a few other long lines, but I
> won't point them all out.
>
>> -             /* We perform the filtering for the '--contains' option */
>> +             /* We perform the filtering for the '--contains' option... */
>>               if (filter->with_commit &&
>> -                 !commit_contains(filter, commit, &ref_cbdata->contains_cache))
>> +                 !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
>> +                     return 0;
>> +             /* ...or for the `--no-contains' option */
>> +             if (filter->no_commit &&
>> +                 commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
>>                       return 0;
>
> This looks nice and simple. Good.
>
>> +# As the docs say, list tags which contain a specified *commit*. We
>> +# don't recurse down to tags for trees or blobs pointed to by *those*
>> +# commits.
>> +test_expect_success 'Does --[no-]contains stop at commits? Yes!' '
>> +     cd no-contains &&
>> +     blob=$(git rev-parse v0.3:v0.3.t) &&
>> +     tree=$(git rev-parse v0.3^{tree}) &&
>> +     git tag tag-blob $blob &&
>> +     git tag tag-tree $tree &&
>> +     git tag --contains v0.3 >actual &&
>> +     cat >expected <<-\EOF &&
>> +     v0.3
>> +     v0.4
>> +     v0.5
>> +     EOF
>> +     test_cmp expected actual &&
>> +     git tag --no-contains v0.3 >actual &&
>> +     cat >expected <<-\EOF &&
>> +     v0.1
>> +     v0.2
>> +     EOF
>> +     test_cmp expected actual
>> +'
>
> The tests mostly look fine, but this one puzzled me. I guess we're
> checking that tag-blob does not contain v0.3. But how could it?
It's a very defensive test, but I'd like to leave it in.

It would be possible, and perhaps efficient in some cases, to implement "--no-contains <commit>" internally as literally something that just ran "--contains <commit>", got the list of tags, stashed them into a hash, and then did a "git tag -l" and printed out anything *not* in the hash.

This test is asserting that we don't somehow regress to such an implementation.
Show 5 quoted lines
> The more interesting test to me is:
>
>   git tag --contains $blob
>
> which should barf on a non-commit.
Will make sure that's tested for.
Show 9 quoted lines
> For the --no-contains side, you could argue that the blob-tag doesn't
> contain the commit, and it should be listed. But it looks like we just
> drop all non-commit tags completely as soon as we start to do a
> contains/not-contains traversal.
>
> I think the more relevant comparison is "--no-merged", and it behaves
> the same way as your new --no-contains. I don't think I saw this
> subtlety in the documentation, though. It might be worth mentioning
> (unless I just missed it).

For --contains we explicitly document "contain the specified commit", i.e. you couldn't expect this to list tree-test, and indeed it doesn't:

    $ git tag tree-test master^{tree}
    $ git tag -l --contains master '*test*'

However the --[no-]merged option says "reachable [...] from the specified commit", which seems to me to be a bit more ambiguous as to whether you could expect it to print tree/blob tags.

Jeff King· Mar 20, 2017, 19:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 11 quoted lines
> > I think the more relevant comparison is "--no-merged", and it behaves
> > the same way as your new --no-contains. I don't think I saw this
> > subtlety in the documentation, though. It might be worth mentioning
> > (unless I just missed it).
> 
> For --contains we explicitly document "contain the specified commit",
> i.e. you couldn't expect this to list tree-test, and indeed it
> doesn't:
> 
>     $ git tag tree-test master^{tree}
>     $ git tag -l --contains master '*test*'

Right, "--contains" cannot have a commit inside a tree, so we were correct to skip the computation entirely. But does that mean that "--no-contains" should be the complement of that, or should it only include tags whose "contains" can be computed in the first place?

IOW, I don't think --contains or --merged are interesting here; they give the right answer by skipping the computation. The question is what the "--no-" variants should do.

> However the --[no-]merged option says "reachable [...] from the
> specified commit", which seems to me to be a bit more ambiguous as to
> whether you could expect it to print tree/blob tags.

I suspect that --no-merged behaves the way it does because it originally came from git-branch, where you only have commits in the first place. The other commands only learned about it during the move to ref-filter, and nobody thought about this corner case.

So we could just treat it like a bug and fix it. But I doubt anybody cares that much in practice either way, so documenting it as "any use of --contains, --no-contains, --no-merged, or --merged requires that the ref in question be a commit" is fine, too.

-Peff
Ævar Arnfjörð Bjarmason· Mar 20, 2017, 20:04 UTC · re: Jeff King · lore

Re: [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref

On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Mon, Mar 20, 2017 at 10:32:47AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I think the more relevant comparison is "--no-merged", and it behaves
>> > the same way as your new --no-contains. I don't think I saw this
>> > subtlety in the documentation, though. It might be worth mentioning
>> > (unless I just missed it).
>>
>> For --contains we explicitly document "contain the specified commit",
>> i.e. you couldn't expect this to list tree-test, and indeed it
>> doesn't:
>>
>>     $ git tag tree-test master^{tree}
>>     $ git tag -l --contains master '*test*'
>
> Right, "--contains" cannot have a commit inside a tree, so we were
> correct to skip the computation entirely. But does that mean that
> "--no-contains" should be the complement of that, or should it only
> include tags whose "contains" can be computed in the first place?
>
> IOW, I don't think --contains or --merged are interesting here; they
> give the right answer by skipping the computation. The question is what
> the "--no-" variants should do.

I think both should only ever find commits. I only came up with this tree/blob scenario for the purposes of tests, but it would make the command less useful & way slower in practice. E.g. now you want to find what to revert to and some blob tag shows up.

Show 13 quoted lines
>> However the --[no-]merged option says "reachable [...] from the
>> specified commit", which seems to me to be a bit more ambiguous as to
>> whether you could expect it to print tree/blob tags.
>
> I suspect that --no-merged behaves the way it does because it originally
> came from git-branch, where you only have commits in the first place.
> The other commands only learned about it during the move to ref-filter,
> and nobody thought about this corner case.
>
> So we could just treat it like a bug and fix it.  But I doubt anybody
> cares that much in practice either way, so documenting it as "any use of
> --contains, --no-contains, --no-merged, or --merged requires that the
> ref in question be a commit" is fine, too.
It's fixed in my soon-to-be resent series.
Jeff King· Mar 20, 2017, 04:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 0/8] Various changes to the "tag" command

On Sat, Mar 18, 2017 at 10:32:48AM +0000, Ævar Arnfjörð Bjarmason wrote:
> This series incorporates and replaces all the "tag" patches I have
> floating around the list, and adds a lot in the mix which discovered
> while working on the initial two patches, but which made sense as
> separate patches.

I had a few small comments, and I agree with the points that Junio raised. But aside from that, it looks good to me.

-Peff
Junio C Hamano· Mar 20, 2017, 15:57 UTC · re: Jeff King · lore

Re: [PATCH 0/8] Various changes to the "tag" command

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> On Sat, Mar 18, 2017 at 10:32:48AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> This series incorporates and replaces all the "tag" patches I have
>> floating around the list, and adds a lot in the mix which discovered
>> while working on the initial two patches, but which made sense as
>> separate patches.
>
> I had a few small comments, and I agree with the points that Junio
> raised. But aside from that, it looks good to me.

Thanks for catching issues I missed, and thank you to both of you to working their solution out quickly. It seems that a final reroll will be rather an uncontroversial one that can be merged to 'next' and then to 'master' soonish.

Thanks for working on this.

← back to recent threads