[PATCH v2 2/2] tag: prevent nested tags

[Denton Liu <liu.denton@gmail.com>]

Robert Dailey reported confusion on the mailing list about a nested tag
which was most likely created by mistake. Jeff King noted that this
isn't a very common case so, most likely, creating a tag-to-a-tag is a
user-error.

Prevent mistakes by erroring and providing advice on nested tags, unless
"--allow-nested-tag" is specified. Fix tests that fail as a result of
this change.

Add tests to ensure that nested tags are disallowed unless the
"--allow-nested-tag" option is provided.

Reported-by: Robert Dailey <rcdailey.lists@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/config/advice.txt |  2 ++
 Documentation/git-tag.txt       | 16 +++++++++++++++-
 advice.c                        |  2 ++
 advice.h                        |  1 +
 builtin/tag.c                   | 23 ++++++++++++++++++++++-
 t/annotate-tests.sh             |  2 +-
 t/t0410-partial-clone.sh        |  2 +-
 t/t4205-log-pretty-formats.sh   |  2 +-
 t/t5305-include-tag.sh          |  2 +-
 t/t5500-fetch-pack.sh           |  2 +-
 t/t6302-for-each-ref-filter.sh  |  4 ++--
 t/t7004-tag.sh                  | 12 ++++++++++--
 t/t9350-fast-export.sh          |  4 ++--
 13 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 88620429ea..ec4f6ae658 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -90,4 +90,6 @@ advice.*::
 	waitingForEditor::
 		Print a message to the terminal whenever Git is waiting for
 		editor input from the user.
+	nestedTag::
+		Advice shown if a user attempts to recursively tag a tag object.
 --
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index a74e7b926d..e65548b1a0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git tag' [-a | -s | -u <keyid>] [-f] [-m <msg> | -F <file>] [-e]
-	<tagname> [<commit> | <object>]
+	[--allow-nested-tag] <tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]
 	[--points-at <object>] [--column[=<options>] | --no-column]
@@ -193,6 +193,20 @@ This option is only applicable when listing tags without annotation lines.
 	that of linkgit:git-for-each-ref[1].  When unspecified,
 	defaults to `%(refname:strip=2)`.
 
+--allow-nested-tag::
+	Usually nestedly tagging a tag object is a mistake and the
+	command prevents you from making such a tag. This option
+	bypasses the safety and allows this to happen.
++
+Note that there is nothing logically wrong with nesting tags and, in
+fact, there may be some valid use-cases, such as showing a cryptographic
+chain of custody by signing someone else's signed tag. However, in
+practice, this is typically a mistake so we prevent it from happening by
+default unless specifically requested.
++
+Automatically erroring on nested tags was introduced in Git version
+2.22.0.
+
 <tagname>::
 	The name of the tag to create, delete, or describe.
 	The new tag name must pass all checks defined by
diff --git a/advice.c b/advice.c
index 567209aa79..ce5f374ecd 100644
--- a/advice.c
+++ b/advice.c
@@ -26,6 +26,7 @@ int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
 int advice_graft_file_deprecated = 1;
 int advice_checkout_ambiguous_remote_branch_name = 1;
+int advice_nested_tag = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -81,6 +82,7 @@ static struct {
 	{ "waitingForEditor", &advice_waiting_for_editor },
 	{ "graftFileDeprecated", &advice_graft_file_deprecated },
 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
+	{ "nestedTag", &advice_nested_tag },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index f875f8cd8d..cb5d361614 100644
--- a/advice.h
+++ b/advice.h
@@ -26,6 +26,7 @@ extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
 extern int advice_graft_file_deprecated;
 extern int advice_checkout_ambiguous_remote_branch_name;
+extern int advice_nested_tag;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/builtin/tag.c b/builtin/tag.c
index faae364e0f..66da4775b1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -22,7 +22,7 @@
 #include "ref-filter.h"
 
 static const char * const git_tag_usage[] = {
-	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
+	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [--allow-nested-tag]\n"
 		"\t\t<tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
@@ -198,6 +198,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu
 struct create_tag_options {
 	unsigned int message_given:1;
 	unsigned int use_editor:1;
+	unsigned int allow_nested_tag;
 	unsigned int sign;
 	enum {
 		CLEANUP_NONE,
@@ -206,6 +207,17 @@ struct create_tag_options {
 	} cleanup_mode;
 };
 
+static const char message_advice_nested_tag[] =
+	N_("The object '%s' referred to by your new tag is already a tag.\n"
+	   "\n"
+	   "If you meant to create a tag of a tag, use:\n"
+	   "\n"
+	   "\tgit tag --allow-nested-tag %s\n"
+	   "\n"
+	   "If you meant to tag the object that it points to, use:\n"
+	   "\n"
+	   "\tgit tag %s^{}");
+
 static void create_tag(const struct object_id *object, const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
 		       struct object_id *prev, struct object_id *result)
@@ -218,6 +230,13 @@ static void create_tag(const struct object_id *object, const char *tag,
 	if (type <= OBJ_NONE)
 		die(_("bad object type."));
 
+	if (type == OBJ_TAG && !opt->allow_nested_tag) {
+		error(_("refusing to make a nested tag"));
+		if (advice_nested_tag)
+			advise(_(message_advice_nested_tag), tag, tag, tag);
+		exit(1);
+	}
+
 	strbuf_addf(&header,
 		    "object %s\n"
 		    "type %s\n"
@@ -404,6 +423,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 					N_("use another key to sign the tag")),
 		OPT__FORCE(&force, N_("replace the tag if exists"), 0),
 		OPT_BOOL(0, "create-reflog", &create_reflog, N_("create a reflog")),
+		OPT_BOOL(0, "allow-nested-tag", &opt.allow_nested_tag,
+					N_("allow nested tags to be made")),
 
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 6da48a2e0a..9849ee30ea 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -70,7 +70,7 @@ test_expect_success 'blame 1 author' '
 
 test_expect_success 'blame by tag objects' '
 	git tag -m "test tag" testTag &&
-	git tag -m "test tag #2" testTag2 testTag &&
+	git tag -m "test tag #2" --allow-nested-tag testTag2 testTag &&
 	check_count -h testTag A 2 &&
 	check_count -h testTag2 A 2
 '
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index bce02788e6..00922d4649 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -16,7 +16,7 @@ pack_as_from_promisor () {
 
 promise_and_delete () {
 	HASH=$(git -C repo rev-parse "$1") &&
-	git -C repo tag -a -m message my_annotated_tag "$HASH" &&
+	git -C repo tag -a -m message my_annotated_tag --allow-nested-tag "$HASH" &&
 	git -C repo rev-parse my_annotated_tag | pack_as_from_promisor &&
 	# tag -d prints a message to stdout, so redirect it
 	git -C repo tag -d my_annotated_tag >/dev/null &&
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f42a69faa2..039f652418 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -511,7 +511,7 @@ test_expect_success 'set up log decoration tests' '
 
 test_expect_success 'log decoration properly follows tag chain' '
 	git tag -a tag1 -m tag1 &&
-	git tag -a tag2 -m tag2 tag1 &&
+	git tag -a tag2 -m tag2 --allow-nested-tag tag1 &&
 	git tag -d tag1 &&
 	git commit --amend -m shorter &&
 	git log --no-walk --tags --pretty="%H %d" --decorate=full >actual &&
diff --git a/t/t5305-include-tag.sh b/t/t5305-include-tag.sh
index a5eca210b8..be17bfa9b4 100755
--- a/t/t5305-include-tag.sh
+++ b/t/t5305-include-tag.sh
@@ -68,7 +68,7 @@ test_expect_success 'check unpacked result (have commit, have tag)' '
 test_expect_success 'create hidden inner tag' '
 	test_commit commit &&
 	git tag -m inner inner HEAD &&
-	git tag -m outer outer inner &&
+	git tag -m outer --allow-nested-tag outer inner &&
 	git tag -d inner
 '
 
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 49c540b1e1..a71ac97a61 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -562,7 +562,7 @@ test_expect_success 'test --all wrt tag to non-commits' '
 		hello tag
 	EOF
 	) &&
-	git tag -a -m "tag -> tag" tag-to-tag $tag &&
+	git tag -a -m "tag -> tag" --allow-nested-tag tag-to-tag $tag &&
 
 	# `fetch-pack --all` should succeed fetching all those objects.
 	mkdir fetchall &&
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fc067ed672..5eed5da6d2 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -12,7 +12,7 @@ test_expect_success 'setup some history and refs' '
 	git checkout -b side &&
 	test_commit four &&
 	git tag -m "An annotated tag" annotated-tag &&
-	git tag -m "Annonated doubly" doubly-annotated-tag annotated-tag &&
+	git tag -m "Annonated doubly" --allow-nested-tag doubly-annotated-tag annotated-tag &&
 
 	# Note that these "signed" tags might not actually be signed.
 	# Tests which care about the distinction should be marked
@@ -24,7 +24,7 @@ test_expect_success 'setup some history and refs' '
 		sign=
 	fi &&
 	git tag $sign -m "A signed tag" signed-tag &&
-	git tag $sign -m "Signed doubly" doubly-signed-tag signed-tag &&
+	git tag $sign -m "Signed doubly" --allow-nested-tag doubly-signed-tag signed-tag &&
 
 	git checkout master &&
 	git update-ref refs/odd/spot master
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0b01862c23..d5e705fa1d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1265,7 +1265,7 @@ echo "A message for another tag" >>expect
 echo '-----BEGIN PGP SIGNATURE-----' >>expect
 test_expect_success GPG \
 	'creating a signed tag pointing to another tag should succeed' '
-	git tag -s -m "A message for another tag" tag-signed-tag signed-tag &&
+	git tag -s -m "A message for another tag" --allow-nested-tag tag-signed-tag signed-tag &&
 	get_tag_msg tag-signed-tag >actual &&
 	test_cmp expect actual
 '
@@ -1690,7 +1690,7 @@ test_expect_success '--points-at finds annotated tags of commits' '
 '
 
 test_expect_success '--points-at finds annotated tags of tags' '
-	git tag -m "describing the v4.0 tag object" \
+	git tag -m "describing the v4.0 tag object" --allow-nested-tag \
 		annotated-again-v4.0 annotated-v4.0 &&
 	cat >expect <<-\EOF &&
 	annotated-again-v4.0
@@ -1700,6 +1700,14 @@ test_expect_success '--points-at finds annotated tags of tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'recursive tagging should fail without --allow-nested-tag' '
+	test_must_fail git tag -m nested nested annotated-v4.0
+'
+
+test_expect_success 'recursive tagging should pass with --allow-nested-tag' '
+	git tag --allow-nested-tag -m nested nested annotated-v4.0
+'
+
 test_expect_success 'multiple --points-at are OR-ed together' '
 	cat >expect <<-\EOF &&
 	v2.0
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 5690fe2810..3f48d60d7f 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -441,8 +441,8 @@ test_expect_success 'set-up a few more tags for tag export tests' '
 	HEAD_TREE=$(git show -s --pretty=raw HEAD | grep tree | sed "s/tree //") &&
 	git tag    tree_tag        -m "tagging a tree" $HEAD_TREE &&
 	git tag -a tree_tag-obj    -m "tagging a tree" $HEAD_TREE &&
-	git tag    tag-obj_tag     -m "tagging a tag" tree_tag-obj &&
-	git tag -a tag-obj_tag-obj -m "tagging a tag" tree_tag-obj
+	git tag    tag-obj_tag     -m "tagging a tag" --allow-nested-tag tree_tag-obj &&
+	git tag -a tag-obj_tag-obj -m "tagging a tag" --allow-nested-tag tree_tag-obj
 '
 
 test_expect_success 'tree_tag'        '
-- 
2.21.0.741.g2c528c8f87