| Message ID | c371a653b4049256f3427e467b144385ee47ef43.1553586707.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tag: prevent recursive tags | expand |
On Tue, Mar 26, 2019 at 12:53:18AM -0700, Denton Liu wrote: > Robert Dailey reported confusion on the mailing list about a recursive > 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 recursive tags, > unless "--allow-recursive-tag" is specified. Fix tests that fail as a > result of this change. > > 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> > --- > advice.c | 2 ++ > advice.h | 1 + > builtin/tag.c | 30 ++++++++++++++++++++++++++---- > 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 | 4 ++-- > t/t9350-fast-export.sh | 4 ++-- > 11 files changed, 40 insertions(+), 15 deletions(-) > > diff --git a/advice.c b/advice.c > index 567209aa79..f31889e6de 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_recursive_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 }, > + { "recursiveTag", &advice_recursive_tag }, > > /* make this an alias for backward compatibility */ > { "pushNonFastForward", &advice_push_update_rejected } > diff --git a/advice.h b/advice.h > index f875f8cd8d..66aa39757c 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_recursive_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 02f6bd1279..0b44a3cbc1 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -22,10 +22,11 @@ > #include "ref-filter.h" > > 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 [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [--allow-recursive-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\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), > + N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n" > + "\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), > N_("git tag -v [--format=<format>] <tagname>..."), > NULL > }; > @@ -197,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_recursive_tag; > unsigned int sign; > enum { > CLEANUP_NONE, > @@ -205,6 +207,17 @@ struct create_tag_options { > } cleanup_mode; > }; > > +static const char message_advice_recursive_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-recursive-tag %s\n" My bad, left an extra space before the quote. > + "\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) > @@ -215,7 +228,14 @@ static void create_tag(const struct object_id *object, const char *tag, > > type = oid_object_info(the_repository, object, NULL); > if (type <= OBJ_NONE) > - die(_("bad object type.")); > + die(_("bad object type.")); > + > + if (type == OBJ_TAG && !opt->allow_recursive_tag) { > + error(_("refusing to make a recursive tag")); > + if (advice_recursive_tag) > + advise(_(message_advice_recursive_tag), tag, tag, tag); > + exit(1); > + } > > strbuf_addf(&header, > "object %s\n" > @@ -403,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-recursive-tag", &opt.allow_recursive_tag, > + N_("allow recursive 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..841f922e07 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-recursive-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..5f06c2d76f 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-recursive-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..018550f3b2 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-recursive-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..c99850c1c0 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-recursive-tag outer inner && > git tag -d inner > ' > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 49c540b1e1..c549b37aec 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-recursive-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..f7b56ae195 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-recursive-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-recursive-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..7a7c0ccee9 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-recursive-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-recursive-tag \ > annotated-again-v4.0 annotated-v4.0 && > cat >expect <<-\EOF && > annotated-again-v4.0 > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh > index 5690fe2810..b5ed7e119a 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-recursive-tag tree_tag-obj && > + git tag -a tag-obj_tag-obj -m "tagging a tag" --allow-recursive-tag tree_tag-obj > ' > > test_expect_success 'tree_tag' ' > -- > 2.21.0.512.g57bf1b23e1 >
On Tue, Mar 26 2019, Denton Liu wrote: > 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 [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [--allow-recursive-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\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), > + N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n" > + "\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), > N_("git tag -v [--format=<format>] <tagname>..."), > NULL Better to split the refactoring part of this into a leading change. I.e. the latter 2 removed/2 added here, and the former could also start with a wrap to 79 chars... > type = oid_object_info(the_repository, object, NULL); > if (type <= OBJ_NONE) > - die(_("bad object type.")); > + die(_("bad object type.")); Ditto this 4 space -> tab change. > + > + if (type == OBJ_TAG && !opt->allow_recursive_tag) { > + error(_("refusing to make a recursive tag")); > + if (advice_recursive_tag) > + advise(_(message_advice_recursive_tag), tag, tag, tag); > + exit(1); > + } This patch of mine didn't end up making it in, but shows how instead of "tag, tag, tag" here you can just pass it once and use positional printf arguments in the message. It makes things a lot more self-explanatory: https://public-inbox.org/git/20181026192734.9609-8-avarab@gmail.com/ > > strbuf_addf(&header, > "object %s\n" > @@ -403,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-recursive-tag", &opt.allow_recursive_tag, > + N_("allow recursive 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..841f922e07 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-recursive-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..5f06c2d76f 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-recursive-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..018550f3b2 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-recursive-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..c99850c1c0 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-recursive-tag outer inner && > git tag -d inner > ' > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 49c540b1e1..c549b37aec 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-recursive-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..f7b56ae195 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-recursive-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-recursive-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..7a7c0ccee9 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-recursive-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-recursive-tag \ > annotated-again-v4.0 annotated-v4.0 && > cat >expect <<-\EOF && > annotated-again-v4.0 > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh > index 5690fe2810..b5ed7e119a 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-recursive-tag tree_tag-obj && > + git tag -a tag-obj_tag-obj -m "tagging a tag" --allow-recursive-tag tree_tag-obj > ' > > test_expect_success 'tree_tag' ' At least some of these tests should change the existing test line to a "test_must_fail", i.e. assert that without the new option these aren't created, maybe for good measure test --force too.
On Tue, Mar 26, 2019 at 12:56 AM Denton Liu <liu.denton@gmail.com> wrote: > > Robert Dailey reported confusion on the mailing list about a recursive > 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 recursive tags, > unless "--allow-recursive-tag" is specified. Fix tests that fail as a > result of this change. Any chance we could use the term "nested tag" instead of "recursive tag"?
On Wed, Mar 27 2019, Elijah Newren wrote: > On Tue, Mar 26, 2019 at 12:56 AM Denton Liu <liu.denton@gmail.com> wrote: >> >> Robert Dailey reported confusion on the mailing list about a recursive >> 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 recursive tags, >> unless "--allow-recursive-tag" is specified. Fix tests that fail as a >> result of this change. > > Any chance we could use the term "nested tag" instead of "recursive tag"? +1. "Recursive" sounded wrong to me, but I couldn't think of the now-obvious alternative. Some grepping around shows we use "nested submodules" fairly consistently, and in gitrevisions(7) we say the peel syntax will recursively peel tags (but don't call them nested). So makes sense to refer to the object type as nested, and when we're referring to the operation that'll iterate over that nested structure say it'll be done "recursively".
On Wed, Mar 27, 2019 at 5:27 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Mar 27 2019, Elijah Newren wrote: > > > On Tue, Mar 26, 2019 at 12:56 AM Denton Liu <liu.denton@gmail.com> wrote: > >> > >> Robert Dailey reported confusion on the mailing list about a recursive > >> 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 recursive tags, > >> unless "--allow-recursive-tag" is specified. Fix tests that fail as a > >> result of this change. > > > > Any chance we could use the term "nested tag" instead of "recursive tag"? > > +1. "Recursive" sounded wrong to me, but I couldn't think of the > now-obvious alternative. > > Some grepping around shows we use "nested submodules" fairly > consistently, and in gitrevisions(7) we say the peel syntax will > recursively peel tags (but don't call them nested). > > So makes sense to refer to the object type as nested, and when we're > referring to the operation that'll iterate over that nested structure > say it'll be done "recursively". Wow thanks for fixing this, you guys are awesome. I wasn't expecting anyone to fix this since it seemed kinda niche. You're right that I created this nested tag unintentionally. And due to `git-lfs migrate` not handling nested annotated tags, it took days of debugging to eventually figure out that nothing was working because of 1 farkled tag. Just to make sure I understand the change, is a tag pointing to another tag object now going to be forbidden by default? And to allow it, you must do `git tag --allow-nested-tag`? So for example, going forward, if we have this: $ git tag -m 'Tag 1' 1.0 $ git tag -m 'Tag 2' 2.0 This will fail: $ git tag -f 2.0 1.0 Unless I do either: $ git tag --allow-nested-tag -f 2.0 1.0 Or (this is the intended behavior in my case): $ git tag -f 2.0 1.0^{}
diff --git a/advice.c b/advice.c index 567209aa79..f31889e6de 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_recursive_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 }, + { "recursiveTag", &advice_recursive_tag }, /* make this an alias for backward compatibility */ { "pushNonFastForward", &advice_push_update_rejected } diff --git a/advice.h b/advice.h index f875f8cd8d..66aa39757c 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_recursive_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 02f6bd1279..0b44a3cbc1 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -22,10 +22,11 @@ #include "ref-filter.h" 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 [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [--allow-recursive-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\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), + N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n" + "\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"), N_("git tag -v [--format=<format>] <tagname>..."), NULL }; @@ -197,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_recursive_tag; unsigned int sign; enum { CLEANUP_NONE, @@ -205,6 +207,17 @@ struct create_tag_options { } cleanup_mode; }; +static const char message_advice_recursive_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-recursive-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) @@ -215,7 +228,14 @@ static void create_tag(const struct object_id *object, const char *tag, type = oid_object_info(the_repository, object, NULL); if (type <= OBJ_NONE) - die(_("bad object type.")); + die(_("bad object type.")); + + if (type == OBJ_TAG && !opt->allow_recursive_tag) { + error(_("refusing to make a recursive tag")); + if (advice_recursive_tag) + advise(_(message_advice_recursive_tag), tag, tag, tag); + exit(1); + } strbuf_addf(&header, "object %s\n" @@ -403,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-recursive-tag", &opt.allow_recursive_tag, + N_("allow recursive 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..841f922e07 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-recursive-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..5f06c2d76f 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-recursive-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..018550f3b2 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-recursive-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..c99850c1c0 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-recursive-tag outer inner && git tag -d inner ' diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 49c540b1e1..c549b37aec 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-recursive-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..f7b56ae195 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-recursive-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-recursive-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..7a7c0ccee9 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-recursive-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-recursive-tag \ annotated-again-v4.0 annotated-v4.0 && cat >expect <<-\EOF && annotated-again-v4.0 diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 5690fe2810..b5ed7e119a 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-recursive-tag tree_tag-obj && + git tag -a tag-obj_tag-obj -m "tagging a tag" --allow-recursive-tag tree_tag-obj ' test_expect_success 'tree_tag' '
Robert Dailey reported confusion on the mailing list about a recursive 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 recursive tags, unless "--allow-recursive-tag" is specified. Fix tests that fail as a result of this change. 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> --- advice.c | 2 ++ advice.h | 1 + builtin/tag.c | 30 ++++++++++++++++++++++++++---- 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 | 4 ++-- t/t9350-fast-export.sh | 4 ++-- 11 files changed, 40 insertions(+), 15 deletions(-)