git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Denton Liu <liu.denton@gmail.com>
Date
Apr 2, 2019, 23:03 UTC
Message-ID
<20190402230345.GA5004@dev-l>
In-Reply-To
<1bd9ee28bc8726490ec0a93286056beeb147fc49.1554183429.git.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.

This is the first use of the '%n$<fmt>' style of printf format in the *.[ch] files in our codebase, but it's supported by POSIX[1] and there are existing uses for it in po/*.po files, so hopefully it won't cause any trouble. It's more obvious for translators than having to repeat the argument three times because each use of '%s' could potentially have a different string but this makes it obvious that they're the same.

[1]: http://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html
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>
---
Hi all,

I forgot to incorporate Ævar's recommendation of using the '%n$<fmt>' style of printf format. This patch now includes that.

Thanks,
Denton
 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..8df31e13f0 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 '%1$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 %1$s\n"
+	   "\n"
+	   "If you meant to tag the object that it points to, use:\n"
+	   "\n"
+	   "\tgit tag %1$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);
+		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.832.gd5ec0d3bee
Previous: Denton LiuNext: Junio C Hamano
Message 27 of 48 in “Strange annotated tag issue”
  1. Robert DaileyMar 21, 2019
  2. Bryan TurnerMar 21, 2019
  3. Jeff KingMar 21, 2019
  4. Jeff KingMar 21, 2019
  5. Robert DaileyMar 25, 2019
  6. Jeff KingMar 25, 2019
  7. Robert DaileyMar 25, 2019
  8. Bryan TurnerMar 25, 2019
  9. Jeff KingMar 25, 2019
  10. Ævar Arnfjörð BjarmasonMar 25, 2019
  11. Jeff KingMar 25, 2019
  12. 0/3 tag: prevent recursive tagsDenton Liu, Mar 26, 2019
  13. 1/3 tag: prevent recursive tagsDenton Liu, Mar 26, 2019
  14. Denton LiuMar 26, 2019
  15. Ævar Arnfjörð BjarmasonMar 26, 2019
  16. Elijah NewrenMar 27, 2019
  17. Ævar Arnfjörð BjarmasonMar 27, 2019
  18. Robert DaileyMar 28, 2019
  19. 2/3 t7004: ensure recursive tag behavior is workingDenton Liu, Mar 26, 2019
  20. Ævar Arnfjörð BjarmasonMar 26, 2019
  21. 3/3 git-tag.txt: document --allow-recursive-tag optionDenton Liu, Mar 26, 2019
  22. Ævar Arnfjörð BjarmasonMar 26, 2019
  23. Jeff KingMar 26, 2019
  24. 0/2 tag: prevent nested tagsDenton Liu, Apr 2, 2019
  25. 1/2 tag: fix formattingDenton Liu, Apr 2, 2019
  26. 2/2 tag: prevent nested tagsDenton Liu, Apr 2, 2019
  27. 2/2 tag: prevent nested tagsDenton Liu, Apr 2, 2019
  28. Junio C HamanoApr 3, 2019
  29. Junio C HamanoApr 3, 2019
  30. Robert DaileyApr 3, 2019
  31. Junio C HamanoApr 4, 2019
  32. Robert DaileyApr 4, 2019
  33. Junio C HamanoApr 4, 2019
  34. Robert DaileyApr 5, 2019
  35. Johannes SixtApr 3, 2019
  36. Denton LiuApr 3, 2019
  37. Jeff KingApr 4, 2019
  38. Junio C HamanoApr 4, 2019
  39. Jeff KingApr 4, 2019
  40. Junio C HamanoApr 4, 2019
  41. Jeff KingApr 4, 2019
  42. Eckhard MaaßApr 11, 2019
  43. Junio C HamanoApr 12, 2019
  44. Elijah NewrenApr 5, 2019
  45. Junio C HamanoApr 5, 2019
  46. 0/2 tag: advise on recursive taggingDenton Liu, Apr 4, 2019
  47. 1/2 tag: fix formattingDenton Liu, Apr 4, 2019
  48. 2/2 tag: advise on nested tagsDenton Liu, Apr 4, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.