All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Jake Goulding" <goulding@vivisimo.com>,
	"Jeff King" <peff@peff.net>, "Tom Grennan" <tmgrennan@gmail.com>,
	"Karthik Nayak" <karthik.188@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] tag: Implicitly supply --list given another list-like option
Date: Sat, 11 Mar 2017 12:08:55 +0000	[thread overview]
Message-ID: <20170311120855.16756-1-avarab@gmail.com> (raw)
In-Reply-To: <20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net>

Change these invocations which currently error out without the -l, to
behave as if though -l was provided:

    git tag -l [--contains|--points-at|--[no-]merged] <commit>

I ran into what turned out to be not-a-bug in "branch" where it,
unlike "tag" before this patch, accepts input like:

    git branch --contains v2.8.0 <pattern>

Jeff King pointed out in
<20170310124247.jvrmmcz2pbv4qf3o@sigill.intra.peff.net> in reply to
that::

    The difference between "branch" and "tag" here is that "branch
    --contains" implies "--list" (and the argument becomes a pattern).
    Whereas "tag --contains" just detects the situation and complains.

    If anything, I'd think we would consider teaching "tag" to behave
    more like "branch".

I agree. This change does that, the only tests that broke as a result
of this were tests that were explicitly checking that this
"branch-like" usage wasn't permitted, i.e. no actual breakages
occurred, and I can't imagine an invocation that would negatively
impact backwards compatibility, i.e. these invocations all just
errored out before.

Spelunking through the history via:

    git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'

Reveals that there was no good reason for this in the first place. The
--contains option added in v1.6.1.1-243-g32c35cfb1e made this an
error, and all the other subsequent options I'm changing here just
copy/pasted its pattern.

I've changed the failing tests to check that this invocation mode is
permitted instead, and added extra tests for the list-like options we
weren't testing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Junio: This will merge conflict with my in-flight --no-contains
patch. I can re-send either one depending on which you want to accept
first, this patch will need an additional test for --no-contains. I
just wanted to get this on the ML for review before the --no-contains
patch hit "master".

 Documentation/git-tag.txt |  3 +++
 builtin/tag.c             | 12 ++++++------
 t/t7004-tag.sh            | 25 +++++++++++++++++++++----
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 525737a5d8..c80d9e11ba 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -94,6 +94,9 @@ OPTIONS
 	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.
++
+We supply this option implicitly if any other list-like option is
+provided. E.g. `--contains`, `--points-at` etc.
 
 --sort=<key>::
 	Sort based on the key given.  Prefix `-` to sort in
diff --git a/builtin/tag.c b/builtin/tag.c
index ad29be6923..6ab65bcf6b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -454,6 +454,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
 
+	/* We implicitly supply --list with --contains, --points-at,
+	   --merged and --no-merged, just like git-branch */
+	if (filter.with_commit || filter.points_at.nr || filter.merge_commit)
+		cmdmode = 'l';
+
+	/* Just plain "git tag" is like "git tag --list" */
 	if (argc == 0 && !cmdmode)
 		cmdmode = 'l';
 
@@ -486,12 +492,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	}
 	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 b4698ab5f5..e0306ee5a8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1453,6 +1453,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' '
@@ -1466,8 +1471,10 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
 
 # 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' '
@@ -1744,8 +1751,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' '
@@ -1765,6 +1777,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	other threads:[~2017-03-11 12:09 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-10 10:43 BUG: "git branch --contains <commit> <name>" does nothing, silently fails Ævar Arnfjörð Bjarmason
2017-03-10 12:42 ` Jeff King
2017-03-10 13:23   ` Ævar Arnfjörð Bjarmason
2017-03-11 12:08   ` Ævar Arnfjörð Bjarmason [this message]
2017-03-11 12:14     ` [PATCH] tag: Implicitly supply --list given another list-like option Ævar Arnfjörð Bjarmason
2017-03-12  2:51       ` Junio C Hamano
2017-03-12  3:00         ` Junio C Hamano
2017-03-12  9:15         ` Ævar Arnfjörð Bjarmason
2017-03-12  3:19     ` Junio C Hamano
2017-03-12  9:15       ` Ævar Arnfjörð Bjarmason
2017-03-12 11:19     ` Jeff King

Reply instructions:

You may reply publicly 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

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

  git send-email \
    --in-reply-to=20170311120855.16756-1-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=goulding@vivisimo.com \
    --cc=karthik.188@gmail.com \
    --cc=peff@peff.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.