git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Lars Hjemli <hjemli@gmail.com>, Jeff King <peff@peff.net>,
	Christian Couder <christian.couder@gmail.com>,
	Carlos Rica <jasampler@gmail.com>,
	Samuel Tardieu <sam@rfc1149.net>,
	Tom Grennan <tmgrennan@gmail.com>,
	Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
Subject: [PATCH 4/8] tag: Implicitly supply --list given another list-like option
Date: Sat, 18 Mar 2017 10:32:52 +0000
Message-ID: <20170318103256.27141-5-avarab@gmail.com> (raw)
In-Reply-To: <20170318103256.27141-1-avarab@gmail.com>

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


  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-18 10:32 [PATCH 0/8] Various changes to the "tag" command Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 1/8] tag: Remove a TODO item from the test suite Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 2/8] tag: Refactor the options handling code to be less bizarro Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 3/8] tag: Change misleading --list <pattern> documentation Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` Ævar Arnfjörð Bjarmason [this message]
2017-03-20  3:55   ` [PATCH 4/8] tag: Implicitly supply --list given another list-like option Jeff King
2017-03-20  9:16     ` Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 5/8] tag: Implicitly supply --list given the -n option Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 6/8] ref-filter: Add --no-contains option to tag/branch/for-each-ref Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 7/8] tag: Add tests for --with and --without Ævar Arnfjörð Bjarmason
2017-03-18 10:32 ` [PATCH 8/8] tag: Change --point-at to default to HEAD Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170318103256.27141-5-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=hjemli@gmail.com \
    --cc=jasampler@gmail.com \
    --cc=peff@peff.net \
    --cc=sam@rfc1149.net \
    --cc=tmgrennan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox