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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 18, 2017 at 8:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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...

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

[PATCH v2 0/2] A couple of minor improvements

[Ævar Arnfjörð Bjarmason]

Unchanged from v1 in my <20170318184203.16890-1-avarab@gmail.com>,
except 2/2 changes the "show" command to use --date=short as Junio
suggested in <xmqq37ea2ykh.fsf@gitster.mtv.corp.google.com>.

Æ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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 9ef624ce38..d8c88153c1 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=short --pretty='format:%h ("%s", %ad)' <commit>
+
+To turn that into a handy alias:
+
+    git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%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

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

[SZEDER Gábor]

> 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(-)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 9ef624ce38..d8c88153c1 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=short --pretty='format:%h ("%s", %ad)' <commit>
> +
> +To turn that into a handy alias:
> +
> +    git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
> +
> +And then to get the commit summary:
> +
> +    git git-commit-summary <commit>

- 'tformat:' is a better fit than 'format:' in this case, because it
  adds a trailing newline.

- I actually have a pretty format alias exactly for this purpose:

    pretty.commitref=tformat:%h (%s, %as)

  and a shorter 'git log -1 --pretty=commitref <commit>' does the
  trick.  Perhaps it would be worth to provide this as a builtin
  pretty format.
  (Of course this relies on '%as' being interpreted as short author
  date to make the format work on its own, without the additional
  '--date=short'.  Alas, git doesn't support this, see [1].
  If only there were a pretty format specifier for suppressing diff
  output, then the '-s' could go away as well.)

- I find that the two subsequent 'git's in 'git git-<whatever>' look
  strange.  However, to make this point moot right away:

- I don't think SubmittingPatches is the right place to show how to
  create and use a command alias.


[1] - http://public-inbox.org/git/1444235305-8718-1-git-send-email-szeder@ira.uka.de/T/#u

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>>  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=short --pretty='format:%h ("%s", %ad)' <commit>
>> +
>> +To turn that into a handy alias:
>> +
>> +    git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>> +
>> +And then to get the commit summary:
>> +
>> +    git git-commit-summary <commit>
>
> - 'tformat:' is a better fit than 'format:' in this case, because it
>   adds a trailing newline.

That depends on what you use it for.  I most often use mine to
insert the reference that flows in a sentence, not as a separate
displayed material, e.g.

    1f6b1afe ("Git 2.12.1", 2017-03-20)

so for that purpose, not adding a trailing newline is a feature.

> - I find that the two subsequent 'git's in 'git git-<whatever>' look
>   strange.  However, to make this point moot right away:
>
> - I don't think SubmittingPatches is the right place to show how to
>   create and use a command alias.

These two I do agree with.

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

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 21, 2017 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>>  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=short --pretty='format:%h ("%s", %ad)' <commit>
>>> +
>>> +To turn that into a handy alias:
>>> +
>>> +    git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>>> +
>>> +And then to get the commit summary:
>>> +
>>> +    git git-commit-summary <commit>
>>
>> - 'tformat:' is a better fit than 'format:' in this case, because it
>>   adds a trailing newline.
>
> That depends on what you use it for.  I most often use mine to
> insert the reference that flows in a sentence, not as a separate
> displayed material, e.g.
>
>     1f6b1afe ("Git 2.12.1", 2017-03-20)
>
> so for that purpose, not adding a trailing newline is a feature.

I agree with tformat. I didn't notice this because I've been screwing
around with my pager settings and my configuration was implicitly
adding a newline. Do you mind fixing that up Junio, or should I
re-send it?

>> - I find that the two subsequent 'git's in 'git git-<whatever>' look
>>   strange.  However, to make this point moot right away:
>>
>> - I don't think SubmittingPatches is the right place to show how to
>>   create and use a command alias.
>
> These two I do agree with.

I don't think it disturbs the flow of the document, and since someone
going through SubmittingPatches is likely about to submit a patch,
providing that one-liner is handy, not as some "here's how to add
aliases" tutorial, but so you don't need to go and copy/paste it, add
\'s for the "'s etc.

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

[SZEDER Gábor]

On Tue, Mar 21, 2017 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>>  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=short --pretty='format:%h ("%s", %ad)' <commit>
>>> +
>>> +To turn that into a handy alias:
>>> +
>>> +    git config --global alias.git-commit-summary "show -s --date=short --pretty='format:%h (\"%s\", %ad)'"
>>> +
>>> +And then to get the commit summary:
>>> +
>>> +    git git-commit-summary <commit>
>>
>> - 'tformat:' is a better fit than 'format:' in this case, because it
>>   adds a trailing newline.
>
> That depends on what you use it for.  I most often use mine to
> insert the reference that flows in a sentence, not as a separate
> displayed material, e.g.
>
>     1f6b1afe ("Git 2.12.1", 2017-03-20)
>
> so for that purpose, not adding a trailing newline is a feature.

Perhaps we are running it differently.

I use its output that way, too, usually running the command in a
terminal and copy-pasting its output into an editor.  For this I find
'tformat:' clearly better, because the reference ends up on its own
line: it's separate from the next prompt and easy to copy the whole
thing with a triple-click anywhere on that line.

With 'format:' the subsequent shell prompt is the direct continuation
of the reference:

  ~/src/git (master %)$ git show -s --date=short --pretty='format:%h
("%s", %ad)' master
  c0f9c7058 ("Sync with 2.12.1", 2017-03-20)~/src/git (master %)$

I don't like this, because it looks ugly, is a bit more difficult to
copy, and if I press the up key for shell history, then for some
reason it gets messed up like this:

  c0f9c7058 ("Sync with 2.12.1", 2017-03-20)~/src/git (master %)$ git
show -s --da
  masterrt --pretty='format:%h ("%s", %ad)' m

with the cursor blinking on the last line after 'master'.

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> That depends on what you use it for.  I most often use mine to
>> insert the reference that flows in a sentence, not as a separate
>> displayed material, e.g.
>>
>>     1f6b1afe ("Git 2.12.1", 2017-03-20)
>>
>> so for that purpose, not adding a trailing newline is a feature.
>
> Perhaps we are running it differently.
>
> I use its output that way, too, usually running the command in a
> terminal and copy-pasting its output into an editor.

I do \C-u\M-!git one<ENTER> from Emacs in the middle of typing a
sentence (where "git one" is aliased to that --format thing), and
for this I obviously do not want the terminating newline.

Of course --pretty=format:... has the opposite effect and in a
terminal to make it easier to cut&paste you do want to have its
output separated from the prompt string.

So as I said, "That depends on what you use it for."

As this is a mere example, we should just shoot for brevity instead?
Both "--pretty=format:" and "--pretty=tformat:" are much longer than
"--format=", so we can just mention

    git show --date=short -s --format='%h ("%s", %ad)'

and let the users to customize it for their needs?

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

[Jakub Narębski]

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?

-- 
Jakub Narębski

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 19, 2017 at 1:48 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> 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.

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 18, 2017 at 7:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 18, 2017 at 8:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

	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.

Thanks.

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 18, 2017 at 7:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

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

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 4:44 AM, Jeff King <peff@peff.net> wrote:
> 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.

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

[Jeff King]

On Mon, Mar 20, 2017 at 05:09:02PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Jeff King]

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.

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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 4:55 AM, Jeff King <peff@peff.net> wrote:
> 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.

>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>
> The tests looked reasonable to me.
>
> -Peff

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Jeff King]

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/

>     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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Jeff King]

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.


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

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

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

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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 5:25 AM, Jeff King <peff@peff.net> wrote:
> 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.
>
>> 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.

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

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

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

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

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

[Jeff King]

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.

> 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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 20, 2017 at 8:52 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

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

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 18, 2017 at 7:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Æ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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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