Strange annotated tag issue

Strange annotated tag issue

[Robert Dailey]

I have a particular tag in my repo that shows 2 annotated
descriptions, which is very confusing.

The command I ran:

```
git show --format=fuller 4.2.0.1900
```

And the output:

```
tag 4.2.0/1900
Tagger:     John Doe <john.doe@domain.com>
TaggerDate: Fri Jul 18 10:46:30 2014 -0500

QA/Internal Release for 4.2.0.19

tag 4.2.0/1900
Tagger:     John Doe <john.doe@domain.com>
TaggerDate: Fri Jul 18 10:46:15 2014 -0500

QA/Internal Release

commit 2fcfd00ef84572fb88852be55315914f37e91e11 (tag: 4.2.0.1900)
Author:     John Doe <john.doe@domain.com>
AuthorDate: Thu Jul 17 11:20:17 2014 -0500
Commit:     John Doe <john.doe@domain.com>
CommitDate: Thu Jul 17 11:20:17 2014 -0500

    Commit description
```

Why does it show two entries? In my `packed-refs` file, it also shows
a strange revision for the tag (I expect to see just 1 SHA1). Not sure
if it is related:

```
66c41d67da887025c4e22e9891f5cd261f82eb31 refs/tags/4.2.0.1900
^2fcfd00ef84572fb88852be55315914f37e91e11
```

Note I'm checking all of this on a bare clone (used `git clone
--mirror`). Can someone help me understand what is going on here? I
found this issue because I'm trying to do `git lfs migrate import`,
and it isn't processing my tag because of this.

Re: Strange annotated tag issue

[Bryan Turner]

On Thu, Mar 21, 2019 at 9:59 AM Robert Dailey <rcdailey.lists@gmail.com> wrote:
>
> I have a particular tag in my repo that shows 2 annotated
> descriptions, which is very confusing.
>
> The command I ran:
>
> ```
> git show --format=fuller 4.2.0.1900
> ```
>
> And the output:
>
> ```
> tag 4.2.0/1900
> Tagger:     John Doe <john.doe@domain.com>
> TaggerDate: Fri Jul 18 10:46:30 2014 -0500
>
> QA/Internal Release for 4.2.0.19
>
> tag 4.2.0/1900
> Tagger:     John Doe <john.doe@domain.com>
> TaggerDate: Fri Jul 18 10:46:15 2014 -0500
>
> QA/Internal Release

Not sure about this part, though I notice that the two rows have
different "TaggerDate" values by ~15 seconds.

>
> commit 2fcfd00ef84572fb88852be55315914f37e91e11 (tag: 4.2.0.1900)
> Author:     John Doe <john.doe@domain.com>
> AuthorDate: Thu Jul 17 11:20:17 2014 -0500
> Commit:     John Doe <john.doe@domain.com>
> CommitDate: Thu Jul 17 11:20:17 2014 -0500
>
>     Commit description
> ```
>
> Why does it show two entries? In my `packed-refs` file, it also shows
> a strange revision for the tag (I expect to see just 1 SHA1). Not sure
> if it is related:
>
> ```
> 66c41d67da887025c4e22e9891f5cd261f82eb31 refs/tags/4.2.0.1900
> ^2fcfd00ef84572fb88852be55315914f37e91e11
> ```

This part, though, is normal for "packed-refs". The first line shows
the annotated tag object's hash ("66c41d67da8") and the tagged
object's hash ("2fcfd00ef8"). You can see that "2fcfd00ef8" matches
the tagged commit output by "git show". The leading "^" on the second
line is how Git knows the line identifies a peeled tag's target rather
than the start of a new ref. If your "packed-refs" starts with
"peeled" (and maybe "fully-peeled") then every annotated (or signed)
tag in the file should have a second line prefixed by "^".

Hopefully this at least resolves part of your question!

Bryan

>
> Note I'm checking all of this on a bare clone (used `git clone
> --mirror`). Can someone help me understand what is going on here? I
> found this issue because I'm trying to do `git lfs migrate import`,
> and it isn't processing my tag because of this.

Re: Strange annotated tag issue

[Jeff King]

On Thu, Mar 21, 2019 at 12:04:22PM -0700, Bryan Turner wrote:

> > Why does it show two entries? In my `packed-refs` file, it also shows
> > a strange revision for the tag (I expect to see just 1 SHA1). Not sure
> > if it is related:
> >
> > ```
> > 66c41d67da887025c4e22e9891f5cd261f82eb31 refs/tags/4.2.0.1900
> > ^2fcfd00ef84572fb88852be55315914f37e91e11
> > ```
> 
> This part, though, is normal for "packed-refs". The first line shows
> the annotated tag object's hash ("66c41d67da8") and the tagged
> object's hash ("2fcfd00ef8"). You can see that "2fcfd00ef8" matches
> the tagged commit output by "git show". The leading "^" on the second
> line is how Git knows the line identifies a peeled tag's target rather
> than the start of a new ref. If your "packed-refs" starts with
> "peeled" (and maybe "fully-peeled") then every annotated (or signed)
> tag in the file should have a second line prefixed by "^".

Nicely explained.

I think there's one other interesting bit of trivia, since we seem to be
dealing with a tag-of-a-tag here. We store only a single peeled value
for each ref, whatever is at the bottom (which is always a non-tag). So
in this case of a tag that points to a tag that points to a commit, the
peeled value is the commit, and the tag in the middle isn't mentioned.

Likewise for upload-pack output, which mentions the peeled objects (so
that clients can auto-fetch tags). It only gives the full peeling.

In theory we could store and advertise the intermediate objects, but I
don't think anybody has ever cared enough about this case to explore
that (and this is mostly an optimization; it should all work correctly,
and I recall fixing some bugs over the years).

-Peff

Re: Strange annotated tag issue

[Jeff King]

On Thu, Mar 21, 2019 at 11:59:42AM -0500, Robert Dailey wrote:

> I have a particular tag in my repo that shows 2 annotated
> descriptions, which is very confusing.
> 
> The command I ran:
> 
> ```
> git show --format=fuller 4.2.0.1900
> ```
> 
> And the output:
> 
> ```
> tag 4.2.0/1900
> Tagger:     John Doe <john.doe@domain.com>
> TaggerDate: Fri Jul 18 10:46:30 2014 -0500
> 
> QA/Internal Release for 4.2.0.19
> 
> tag 4.2.0/1900
> Tagger:     John Doe <john.doe@domain.com>
> TaggerDate: Fri Jul 18 10:46:15 2014 -0500
> 
> QA/Internal Release
> 
> commit 2fcfd00ef84572fb88852be55315914f37e91e11 (tag: 4.2.0.1900)

Tags can point to any object, including another tag. It looks like
somebody made an annotated tag of an annotated tag (probably by
mistake, given that they have the same tag-name).

Try this:

  git init
  git commit -m commit --allow-empty
  git tag -m inner mytag HEAD
  git tag -f -m outer mytag mytag

  git show mytag

which produces similar output. You can walk the chain yourself with "git
at-file tag 4.2.0.1900". That will have a "type" and "object" field
which presumably point to the second commit.

My guess is that somebody was trying to amend the tag commit message,
but used the tag name to create the second one, rather than the original
commit. I.e,. any of these would have worked for the second command to
replace the old tag:

  git tag -f -m 'new message' mytag HEAD

  git tag -f -m 'new message' 2fcfd00ef84572fb88852be55315914f37e91e11

  git tag -f -m 'new message' mytag mytag^{commit}

If the original tag isn't signed, you could rewrite it at this point
using one of the above commands, coupled with GIT_COMMITTER_* to munge
the author and date.  But note that fetch doesn't update modified tags
by default, so it may just cause confusion if you have a lot of people
using the repository.

-Peff

Re: Strange annotated tag issue

[Robert Dailey]

On Thu, Mar 21, 2019 at 2:29 PM Jeff King <peff@peff.net> wrote:
> Tags can point to any object, including another tag. It looks like
> somebody made an annotated tag of an annotated tag (probably by
> mistake, given that they have the same tag-name).
>
> Try this:
>
>   git init
>   git commit -m commit --allow-empty
>   git tag -m inner mytag HEAD
>   git tag -f -m outer mytag mytag
>
>   git show mytag
>
> which produces similar output. You can walk the chain yourself with "git
> at-file tag 4.2.0.1900". That will have a "type" and "object" field
> which presumably point to the second commit.
>
> My guess is that somebody was trying to amend the tag commit message,
> but used the tag name to create the second one, rather than the original
> commit. I.e,. any of these would have worked for the second command to
> replace the old tag:
>
>   git tag -f -m 'new message' mytag HEAD
>
>   git tag -f -m 'new message' 2fcfd00ef84572fb88852be55315914f37e91e11
>
>   git tag -f -m 'new message' mytag mytag^{commit}
>
> If the original tag isn't signed, you could rewrite it at this point
> using one of the above commands, coupled with GIT_COMMITTER_* to munge
> the author and date.  But note that fetch doesn't update modified tags
> by default, so it may just cause confusion if you have a lot of people
> using the repository.

Thanks for explaining. This is very helpful. Am I naive to think that
this should be an error? I haven't seen a valid _pragmatic_ use for
tags pointing to tags. In 100% of cases (including this one), it is
done out of error. As per your example, users try to "correct" an
annotated tag pointing at a wrong tag or commit. What they expect is
the tag to point to the other tag's commit, but that's not what they
get.

From a high-level, pragmatic perspective, doesn't it make more sense
to change the git behavior so that annotated tags may only point to
commit objects? And in the `git tag -f -m outer mytag mytag` case in
your example, this would automatically perform `mytag^{}` to ensure
that the behavior the user expects is the behavior they get?

Re: Strange annotated tag issue

[Jeff King]

On Mon, Mar 25, 2019 at 08:50:14AM -0500, Robert Dailey wrote:

> On Thu, Mar 21, 2019 at 2:29 PM Jeff King <peff@peff.net> wrote:
> > Tags can point to any object, including another tag. It looks like
> > somebody made an annotated tag of an annotated tag (probably by
> > mistake, given that they have the same tag-name).
> [..]
> Thanks for explaining. This is very helpful. Am I naive to think that
> this should be an error? I haven't seen a valid _pragmatic_ use for
> tags pointing to tags. In 100% of cases (including this one), it is
> done out of error. As per your example, users try to "correct" an
> annotated tag pointing at a wrong tag or commit. What they expect is
> the tag to point to the other tag's commit, but that's not what they
> get.

I don't think I've ever seen a tag-to-a-tag in the wild, but I wouldn't
be surprised if somebody has found a use for it. For example, because
tags can be signed, I can make a signature of your signature, showing a
cryptographic chain of custody.

And at any rate, it has been allowed in the data model for almost 15
years, so I think disallowing it now would be a bad idea. It might be
acceptable to introduce a safety valve into the porcelain, though.

> From a high-level, pragmatic perspective, doesn't it make more sense
> to change the git behavior so that annotated tags may only point to
> commit objects? And in the `git tag -f -m outer mytag mytag` case in
> your example, this would automatically perform `mytag^{}` to ensure
> that the behavior the user expects is the behavior they get?

I think "just commits" is too restrictive. linux.git contains a tag of a
tree, for example (we also have tags pointing to blobs in git.git, but
they are not annotated).

However, I could see an argument for the git-tag porcelain to notice a
tag-of-tag and complain. Probably peeling the tag automatically is a bad
idea, just because it behaved differently for so long. But something
like might be OK:

  $ git tag -a mytag
  error: refusing to make a recursive tag
  hint: The object 'mytag' referred to by your new tag is already a tag.
  hint:
  hint: If you meant to create a tag of a tag, use:
  hint:
  hint:  git tag -a -f mytag
  hint:
  hint: If you meant to tag the object that it points to, use:
  hint:
  hint:  git tag -a mytag^{}

It would be a minor annoyance to somebody who frequently makes
tags-of-tags, but it leaves them with an escape hatch.

-Peff

Re: Strange annotated tag issue

[Robert Dailey]

On Mon, Mar 25, 2019 at 9:49 AM Jeff King <peff@peff.net> wrote:
> I think "just commits" is too restrictive. linux.git contains a tag of a
> tree, for example (we also have tags pointing to blobs in git.git, but
> they are not annotated).
>
> However, I could see an argument for the git-tag porcelain to notice a
> tag-of-tag and complain. Probably peeling the tag automatically is a bad
> idea, just because it behaved differently for so long. But something
> like might be OK:
>
>   $ git tag -a mytag
>   error: refusing to make a recursive tag
>   hint: The object 'mytag' referred to by your new tag is already a tag.
>   hint:
>   hint: If you meant to create a tag of a tag, use:
>   hint:
>   hint:  git tag -a -f mytag
>   hint:
>   hint: If you meant to tag the object that it points to, use:
>   hint:
>   hint:  git tag -a mytag^{}
>
> It would be a minor annoyance to somebody who frequently makes
> tags-of-tags, but it leaves them with an escape hatch.

I think a warning/error would be perfect. Again, if I had realized the
consequences of my actions years ago when I made this tag, it would
have changed a lot down the line for me. If I had seen that message
before, I think it would have helped a lot. You might even add an
educational bit in there and say "Warning: You are creating an
annotated tag that points to another annotated tag. Annotated tags may
point to more than just commits by design. Refspec tag^{} may be used
to peel the source tag back to the commit it points to". Then follow
with the instructional commands.

I agree with your approach though. Thanks again.

Re: Strange annotated tag issue

[Bryan Turner]

On Mon, Mar 25, 2019 at 7:49 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Mar 25, 2019 at 08:50:14AM -0500, Robert Dailey wrote:
>
> > On Thu, Mar 21, 2019 at 2:29 PM Jeff King <peff@peff.net> wrote:
> > > Tags can point to any object, including another tag. It looks like
> > > somebody made an annotated tag of an annotated tag (probably by
> > > mistake, given that they have the same tag-name).
> > [..]
> > Thanks for explaining. This is very helpful. Am I naive to think that
> > this should be an error? I haven't seen a valid _pragmatic_ use for
> > tags pointing to tags. In 100% of cases (including this one), it is
> > done out of error. As per your example, users try to "correct" an
> > annotated tag pointing at a wrong tag or commit. What they expect is
> > the tag to point to the other tag's commit, but that's not what they
> > get.
>
> I don't think I've ever seen a tag-to-a-tag in the wild, but I wouldn't
> be surprised if somebody has found a use for it. For example, because
> tags can be signed, I can make a signature of your signature, showing a
> cryptographic chain of custody.

For a while the Atlassian Bamboo team followed a workflow where they
would do a build in CI, tag that build and then deploy it to a sandbox
environment for smoke testing. If it passed the smoke tests, it would
get "promoted" from the sandbox environment to internal instances used
by the various teams to do their builds. When a sandbox build was
"promoted", they'd create a tag of the sandbox build's tag to have
traceability between the two environments.

I'm not advocating for or judging that workflow one way or another,
and the Bamboo team has since moved on to a different workflow. I just
thought I'd share it as a tag-of-tag workflow that I've seen a real
team using. (There was one place in Bitbucket Server's code where we
didn't handle recursive tags correctly, so their workflow caused some
errors that I needed to make some adjustments for. As a result,
Bitbucket Server's test suite now includes tests that cover tag-of-tag
behaviors.)

Bryan

Re: Strange annotated tag issue

[Jeff King]

On Mon, Mar 25, 2019 at 11:43:52AM -0700, Bryan Turner wrote:

> > I don't think I've ever seen a tag-to-a-tag in the wild, but I wouldn't
> > be surprised if somebody has found a use for it. For example, because
> > tags can be signed, I can make a signature of your signature, showing a
> > cryptographic chain of custody.
> 
> For a while the Atlassian Bamboo team followed a workflow where they
> would do a build in CI, tag that build and then deploy it to a sandbox
> environment for smoke testing. If it passed the smoke tests, it would
> get "promoted" from the sandbox environment to internal instances used
> by the various teams to do their builds. When a sandbox build was
> "promoted", they'd create a tag of the sandbox build's tag to have
> traceability between the two environments.
> 
> I'm not advocating for or judging that workflow one way or another,
> and the Bamboo team has since moved on to a different workflow. I just
> thought I'd share it as a tag-of-tag workflow that I've seen a real
> team using. (There was one place in Bitbucket Server's code where we
> didn't handle recursive tags correctly, so their workflow caused some
> errors that I needed to make some adjustments for. As a result,
> Bitbucket Server's test suite now includes tests that cover tag-of-tag
> behaviors.)

Thanks, I always like hearing these kinds of data points. If nothing
else, it's a good reminder that if Git has behaved some way for many
years, then _somebody_ is likely to have taken advantage of it, whether
we considered it a possibility or not. :)

-Peff

Re: Strange annotated tag issue

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 25 2019, Jeff King wrote:

> On Mon, Mar 25, 2019 at 08:50:14AM -0500, Robert Dailey wrote:
>
>> On Thu, Mar 21, 2019 at 2:29 PM Jeff King <peff@peff.net> wrote:
>> > Tags can point to any object, including another tag. It looks like
>> > somebody made an annotated tag of an annotated tag (probably by
>> > mistake, given that they have the same tag-name).
>> [..]
>> Thanks for explaining. This is very helpful. Am I naive to think that
>> this should be an error? I haven't seen a valid _pragmatic_ use for
>> tags pointing to tags. In 100% of cases (including this one), it is
>> done out of error. As per your example, users try to "correct" an
>> annotated tag pointing at a wrong tag or commit. What they expect is
>> the tag to point to the other tag's commit, but that's not what they
>> get.
>
> I don't think I've ever seen a tag-to-a-tag in the wild, but I wouldn't
> be surprised if somebody has found a use for it. For example, because
> tags can be signed, I can make a signature of your signature, showing a
> cryptographic chain of custody.
>
> And at any rate, it has been allowed in the data model for almost 15
> years, so I think disallowing it now would be a bad idea. It might be
> acceptable to introduce a safety valve into the porcelain, though.
>
>> From a high-level, pragmatic perspective, doesn't it make more sense
>> to change the git behavior so that annotated tags may only point to
>> commit objects? And in the `git tag -f -m outer mytag mytag` case in
>> your example, this would automatically perform `mytag^{}` to ensure
>> that the behavior the user expects is the behavior they get?
>
> I think "just commits" is too restrictive. linux.git contains a tag of a
> tree, for example (we also have tags pointing to blobs in git.git, but
> they are not annotated).
>
> However, I could see an argument for the git-tag porcelain to notice a
> tag-of-tag and complain. Probably peeling the tag automatically is a bad
> idea, just because it behaved differently for so long. But something
> like might be OK:

Sounds good!

>   $ git tag -a mytag
>   error: refusing to make a recursive tag
>   hint: The object 'mytag' referred to by your new tag is already a tag.
>   hint:
>   hint: If you meant to create a tag of a tag, use:
>   hint:
>   hint:  git tag -a -f mytag
>   hint:
>   hint: If you meant to tag the object that it points to, use:
>   hint:
>   hint:  git tag -a mytag^{}
>
> It would be a minor annoyance to somebody who frequently makes
> tags-of-tags, but it leaves them with an escape hatch.

Let's call that something like --allow-recursive-tag (inspired by
'merge' --allow-unrelated-histories) so we don't confuse the desire to
create such a tag with clobbering an existing tag (which -f is
documented to do).

I was going to say "let's make the 'error:' part self-explanatory
without the 'hint:'" part, in case the advice was disabled. But looking
we only allow turning advice off on a per-variable basis, so I suspect
if someone bothered to do that they know about this already. Our
--allow-recursive-tag message is also fairly cryptic (and should, but
doesn't have, advice).

Re: Strange annotated tag issue

[Jeff King]

On Mon, Mar 25, 2019 at 08:19:25PM +0100, Ævar Arnfjörð Bjarmason wrote:

> >   $ git tag -a mytag
> >   error: refusing to make a recursive tag
> >   hint: The object 'mytag' referred to by your new tag is already a tag.
> >   hint:
> >   hint: If you meant to create a tag of a tag, use:
> >   hint:
> >   hint:  git tag -a -f mytag
> >   hint:
> >   hint: If you meant to tag the object that it points to, use:
> >   hint:
> >   hint:  git tag -a mytag^{}
> >
> > It would be a minor annoyance to somebody who frequently makes
> > tags-of-tags, but it leaves them with an escape hatch.
> 
> Let's call that something like --allow-recursive-tag (inspired by
> 'merge' --allow-unrelated-histories) so we don't confuse the desire to
> create such a tag with clobbering an existing tag (which -f is
> documented to do).

Yeah, that's probably a good idea. Now we just need somebody to write
the patch...

-Peff

[PATCH 0/3] tag: prevent recursive tags

[Denton Liu]

Peff said:
> Yeah, that's probably a good idea. Now we just need somebody to write
> the patch...
> 
> -Peff

Hey would you look at that, somebody wrote the patch!

---

Earlier in the mailing list[1], Robert Dailey reported confusion over
some recursive tags.

Peff noted that he hasn't seen a tag-to-a-tag in the wild so in most
cases, it'd probably be a mistake on the part of a user. He also
suggested we error out on a recursive tag unless "--allow-recursive-tag"
is provided.

This patchset implements those suggestions.

[1]: https://public-inbox.org/git/CAHd499BM91tf7f8=phR4Az8vMsHAHUGYsSb1x9as=WukUVZHJw@mail.gmail.com/
[2]: https://public-inbox.org/git/20190325144930.GA19929@sigill.intra.peff.net/


Denton Liu (3):
  tag: prevent recursive tags
  t7004: ensure recursive tag behavior is working
  git-tag.txt: document --allow-recursive-tag option

 Documentation/git-tag.txt      |  7 ++++++-
 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                 | 12 ++++++++++--
 t/t9350-fast-export.sh         |  4 ++--
 12 files changed, 54 insertions(+), 16 deletions(-)

-- 
2.21.0.512.g57bf1b23e1

[PATCH 1/3] tag: prevent recursive tags

[Denton Liu]

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"
+	   "\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

Re: [PATCH 1/3] tag: prevent recursive tags

[Denton Liu]

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
> 

Re: [PATCH 1/3] tag: prevent recursive tags

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 1/3] tag: prevent recursive tags

[Elijah Newren]

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"?

Re: [PATCH 1/3] tag: prevent recursive tags

[Ævar Arnfjörð Bjarmason]

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".

Re: [PATCH 1/3] tag: prevent recursive tags

[Robert Dailey]

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^{}

[PATCH 2/3] t7004: ensure recursive tag behavior is working

[Denton Liu]

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7004-tag.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 7a7c0ccee9..5297da952d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -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-recursive-tag' '
+	test_must_fail git tag -m recursive recursive annotated-v4.0
+'
+
+test_expect_success 'recursive tagging should pass with --allow-recursive-tag' '
+	git tag --allow-recursive-tag -m recursive recursive annotated-v4.0
+'
+
 test_expect_success 'multiple --points-at are OR-ed together' '
 	cat >expect <<-\EOF &&
 	v2.0
-- 
2.21.0.512.g57bf1b23e1

Re: [PATCH 2/3] t7004: ensure recursive tag behavior is working

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 26 2019, Denton Liu wrote:

> Add tests to ensure that recursive tags are disallowed unless the
> "--allow-recursive-tag" option is provided.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t7004-tag.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 7a7c0ccee9..5297da952d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -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-recursive-tag' '
> +	test_must_fail git tag -m recursive recursive annotated-v4.0
> +'
> +
> +test_expect_success 'recursive tagging should pass with --allow-recursive-tag' '
> +	git tag --allow-recursive-tag -m recursive recursive annotated-v4.0
> +'
> +
>  test_expect_success 'multiple --points-at are OR-ed together' '
>  	cat >expect <<-\EOF &&
>  	v2.0

Mmm, I see part of my feedback on 1/3 is addressed here. IMO better just
to squash this into that, or at least say in the commit message "this
commit leaves us with a test blind spot for XYZ reason, but it will be
added back...".

[PATCH 3/3] git-tag.txt: document --allow-recursive-tag option

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-tag.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index a74e7b926d..7e7eb9a7e9 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-recursive-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,11 @@ 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-recursive-tag::
+	Usually recursively 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.
+
 <tagname>::
 	The name of the tag to create, delete, or describe.
 	The new tag name must pass all checks defined by
-- 
2.21.0.512.g57bf1b23e1

Re: [PATCH 3/3] git-tag.txt: document --allow-recursive-tag option

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 26 2019, Denton Liu wrote:

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Documentation/git-tag.txt | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index a74e7b926d..7e7eb9a7e9 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-recursive-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,11 @@ 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-recursive-tag::
> +	Usually recursively 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.
> +
>  <tagname>::
>  	The name of the tag to create, delete, or describe.
>  	The new tag name must pass all checks defined by

Like 1/3 I this should just be squashed into the one patch. No reason to
leave the doc change in another change.

I see the existing --allow-unrelated-histories uses a similar tone, but
maybe something closer to noting that there's nothing wrong with doing
this, and maybe even point out the signed "chain of custody" tagging as
one use-case, then finally noting that since people usually don't really
want this it's disabled by default.

Finally, since "git tag" gets used by scripts a lot in practice, maybe
note that this behavior changed in "Git version 2.22.0".

Re: [PATCH 0/3] tag: prevent recursive tags

[Jeff King]

On Tue, Mar 26, 2019 at 12:53:14AM -0700, Denton Liu wrote:

> Peff said:
> > Yeah, that's probably a good idea. Now we just need somebody to write
> > the patch...
> 
> Hey would you look at that, somebody wrote the patch!

The system works. :)

> Earlier in the mailing list[1], Robert Dailey reported confusion over
> some recursive tags.
> 
> Peff noted that he hasn't seen a tag-to-a-tag in the wild so in most
> cases, it'd probably be a mistake on the part of a user. He also
> suggested we error out on a recursive tag unless "--allow-recursive-tag"
> is provided.
> 
> This patchset implements those suggestions.

Thanks. I agree with all of the comments Ævar left, but other than that
this looks pretty good to me.

The only hesitation I'd have is that turning this case into a hard error
(rather than just an informative warning) may be too sudden for some
people's tastes. I'm on the fence myself; I'll be curious what Junio
thinks when he gets back.

-Peff

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

[Denton Liu]

Earlier in the mailing list, Robert Dailey reported confusion over some
nested tags. [1]

Peff noted that he hasn't seen a tag-to-a-tag in the wild so in most cases,
it'd probably be a mistake on the part of a user. He also suggested we error
out on a nested tag unless "--allow-nested-tag" is provided. [2]

This patchset implements those suggestions.

Changes since v1:

* Squashed patches into two patches, one to clean up tag.c, one to do the rest
* s/recursive/nested/g (so the new option is --allow-nested-tag)
* Add more details to --allow-nested-tag documentation

[1]: https://public-inbox.org/git/CAHd499BM91tf7f8=phR4Az8vMsHAHUGYsSb1x9as=WukUVZHJw@mail.gmail.com/
[2]: https://public-inbox.org/git/20190325144930.GA19929@sigill.intra.peff.net/


Denton Liu (2):
  tag: fix formatting
  tag: prevent nested tags

 Documentation/config/advice.txt |  2 ++
 Documentation/git-tag.txt       | 16 +++++++++++++++-
 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                  | 12 ++++++++++--
 t/t9350-fast-export.sh          |  4 ++--
 13 files changed, 65 insertions(+), 16 deletions(-)

-- 
2.21.0.741.g2c528c8f87

[PATCH v2 1/2] tag: fix formatting

[Denton Liu]

Wrap usage line at '<tagname>'. Also, wrap strings with '\n' at the end
of string fragments instead of at the beginning of the next string
fragment.

Convert a space-indent into a tab-indent for style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/tag.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279..faae364e0f 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>]\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
 };
@@ -215,7 +216,7 @@ 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."));
 
 	strbuf_addf(&header,
 		    "object %s\n"
-- 
2.21.0.741.g2c528c8f87

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

[Denton Liu]

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

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

[Denton Liu]

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

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> 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

The latter is a stronger indication that this should be OK than the
former ;-)  Thanks for digging and noting.

> 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.
>  --

In addition to 'advice', we may have to add a configuration to help
projects that wants to tag tag objects regularly so that they do not
have to keep typing "--allow-nested-tag".  But that can wait until a
participant of such a project comes forward and makes a case for
their workflow.

> +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.

I am not sure if this is so bad, actually.  Why do we need to treat
it as a mistake?  When a command that wants a commit is fed a tag
(either a tag that directly refers to a commit, or a tag that tags
another tag that refers to a commit), the command knows how to peel
so it's not like the user is forced to say "git log T^{commit}".

And if something that *MUST* take a commit refuses to (or more
likely, forges to) peel a tag down to a commit and yields an error,
I think that is what needs fixing, not the command that creates a
tag.

So, I am fairly negative on this change---unless it is made much
more clear in the doc and/or in the proposed log message what
practical downside there are to the end users if we do not stop this
"mistake", that is.

> +Automatically erroring on nested tags was introduced in Git version
> +2.22.0.

And please do not write something like this.  A feature gets in a
release when it is ready, and we may not ship this in 2.22.

"git tag --help" the user is running may or may not have the
paragraph about "nested tag", depending on the existence of the
feature in the version of Git the user is running, so there is no
need to say something like that.

And no, I do not buy arguments like "random web servers serve
different versions of documentation without identifying which
version of Git they describe".

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not sure if this is so bad, actually.  Why do we need to treat
> it as a mistake?  When a command that wants a commit is fed a tag
> (either a tag that directly refers to a commit, or a tag that tags
> another tag that refers to a commit), the command knows how to peel
> so it's not like the user is forced to say "git log T^{commit}".
>
> And if something that *MUST* take a commit refuses to (or more
> likely, forges to) peel a tag down to a commit and yields an error,
> I think that is what needs fixing, not the command that creates a
> tag.
>
> So, I am fairly negative on this change---unless it is made much
> more clear in the doc and/or in the proposed log message what
> practical downside there are to the end users if we do not stop this
> "mistake", that is.

Having said all that, I can sort-of see that it may make sense to
forbid tagging anything but a commit-ish, either by default, or a
"git tag --forbid-no-committish" that can be turned on with a
configuration.

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

[Robert Dailey]

On Wed, Apr 3, 2019 at 3:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am not sure if this is so bad, actually.  Why do we need to treat
> > it as a mistake?  When a command that wants a commit is fed a tag
> > (either a tag that directly refers to a commit, or a tag that tags
> > another tag that refers to a commit), the command knows how to peel
> > so it's not like the user is forced to say "git log T^{commit}".
> >
> > And if something that *MUST* take a commit refuses to (or more
> > likely, forges to) peel a tag down to a commit and yields an error,
> > I think that is what needs fixing, not the command that creates a
> > tag.
> >
> > So, I am fairly negative on this change---unless it is made much
> > more clear in the doc and/or in the proposed log message what
> > practical downside there are to the end users if we do not stop this
> > "mistake", that is.
>
> Having said all that, I can sort-of see that it may make sense to
> forbid tagging anything but a commit-ish, either by default, or a
> "git tag --forbid-no-committish" that can be turned on with a
> configuration.

I don't really understand what part of the change is a negative for
you. Are you saying that `git tag` should peel the tags for you? Or
are you saying you don't like nested tagging to be opt-in and an error
otherwise?

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

[Junio C Hamano]

Robert Dailey <rcdailey.lists@gmail.com> writes:

>> > more clear in the doc and/or in the proposed log message what
>> > practical downside there are to the end users if we do not stop this
>> > "mistake", that is.
>>
>> Having said all that, I can sort-of see that it may make sense to
>> forbid tagging anything but a commit-ish, either by default, or a
>> "git tag --forbid-no-committish" that can be turned on with a
>> configuration.
>
> I don't really understand what part of the change is a negative for
> you. Are you saying that `git tag` should peel the tags for you? Or
> are you saying you don't like nested tagging to be opt-in and an error
> otherwise?

No, no and no.  I am saying "git tag -a -m 'message' tag anothertag"
that creates a tag that points at another tag is perfectly fine.

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

[Robert Dailey]

On Thu, Apr 4, 2019 at 4:32 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Robert Dailey <rcdailey.lists@gmail.com> writes:
>
> >> > more clear in the doc and/or in the proposed log message what
> >> > practical downside there are to the end users if we do not stop this
> >> > "mistake", that is.
> >>
> >> Having said all that, I can sort-of see that it may make sense to
> >> forbid tagging anything but a commit-ish, either by default, or a
> >> "git tag --forbid-no-committish" that can be turned on with a
> >> configuration.
> >
> > I don't really understand what part of the change is a negative for
> > you. Are you saying that `git tag` should peel the tags for you? Or
> > are you saying you don't like nested tagging to be opt-in and an error
> > otherwise?
>
> No, no and no.  I am saying "git tag -a -m 'message' tag anothertag"
> that creates a tag that points at another tag is perfectly fine.

It might be fine within the realm of git itself, because git knows how
to deal with them by peeling, as you say, but there are 3 reasons I
dislike that this is allowed:

1. The intent by the user was to create a tag pointing to the commit
that another tag points to. So the peeling was expected to occur when
the tag was *created*, not when it is used. Again, the use case is
that I create a new annotated tag, then realize later I pointed it to
the wrong commit. So sometimes I correct it by pointing it at another
tag, but my intent was for both tags to point at the same commit, not
for one tag to point to a commit and the other to point to the tag.

2. When users on my team do a `git show tag`, they see 2 descriptions
and 2 tags. This creates a LOT of confusion. I wasted hours trying to
find out what this meant. It was very obscure and indirect. Most users
do not have your expert level of understanding of the internals of
Git. So I think there's a disconnect between how you like how the
machinery works internally with tags, and what users expect from the
porcelain interface. I think we should go with what's pragmatic (tags
that point to commits, not other tags) and design the interfaces for
the majority use case. Tags pointing to tags is a minority use case,
or an edge case. Given that, I think it should be opt-in.

3. Even if Git internally handles peeling tags, external 3rd party
tooling may not. As I mentioned in another thread, `git lfs migrate`
was not programmed to peel tags. I reported the issue here:

https://github.com/git-lfs/git-lfs/issues/3568

The author there admitted that migrating the repository *and* keeping
the tags pointing to tags would be difficult. So I think the solution
they're going to end up going with is that when you migrate a
repository for LFS support, they will permanently peel your annotated
tags. Which I personally think is the correct solution.

So in summary, I think it's better for tags to be peeled when they're
created, or at least make the user aware of the fact that they're
creating something that they might not be expecting. Just because Git
handles peeled tags transparently doesn't mean that's what the user
wants, or what the user expects. I've been using git for years and I
never knew tags pointing to other tags could exist. It sounds like a
technicality that most users shouldn't care about.

That's my feedback as a user, take it or leave it. I'm not really
concerned with what's right or wrong from a git-internals perspective,
what I want is a tool that makes sense for my day to day job, and I
think this PR aligns with that.

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

[Junio C Hamano]

Robert Dailey <rcdailey.lists@gmail.com> writes:

> It might be fine within the realm of git itself, because git knows how
> to deal with them by peeling, as you say, but there are 3 reasons I
> dislike that this is allowed:

That sounds as if you have tools that forgets to peel when it should
in mind.  Isn't that what you should be looking into fixing?  After
all, tools that aim to work with Git should strive to come close to
"within the realm of git itsef" in the modern world.

> 1. The intent by the user was to create a tag pointing to the commit
> that another tag points to.

You make it sound as if you are convinced that is the truth.  I am
not.  If I want to tag the commit pointed at by tag v1.0, I'd say I
want to tag "v1.0^0", because otherwise there won't be a way to say

    $ git tag -s -m "i am aware of this tag" initial v1.0

i.e. making a tag that points at a tag.  So I take the lack of ^0
(or ^{commit}) peeling an explicit sign that shows the intent by the
user (well, those who know the tool, anyway) is clearly to create a
tag pointing to that tag.  In other words, peeling at the tagging
time is wrong, and rejecting tag creation is also wrong.

> 2. When users on my team do a `git show tag`, they see 2 descriptions
> and 2 tags. This creates a LOT of confusion.

So what?  Not everybody will forever stay to be a newbie ;-) 

As I said, an opt-in tag.allowCommitOnly is fine.  That would train
their fingers to peel with ^{commit} when they want to tag a commit.
An opt-in tag.autoPeelTags might also be fine, even though that
would not help training their fingers (so they will have to be
prepared for the same "confusion" on a fresh machine)

When the command line clearly tells us that the user wants to tag a
tag, we should not get overly "smart" and refuse to create a tag of
a tag, unless the user tells us otherwise with some means.

> 3. Even if Git internally handles peeling tags, external 3rd party
> tooling may not. As I mentioned in another thread, `git lfs migrate`
> was not programmed to peel tags. I reported the issue here:

That is good, and it is the right way.  After all, external 3rd
party tooling may tag a tag even after we castrate "git tag" to be
incapable of doing so, so a bug like the one you are helping to fix
in lfs needs to be fixed anyway.  In other words, thats the only
sensible way forward when you care about the entire Git ecosystem,
not just the main/reference implementation we work on here.

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

[Robert Dailey]

On Thu, Apr 4, 2019 at 4:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Robert Dailey <rcdailey.lists@gmail.com> writes:
>
> > It might be fine within the realm of git itself, because git knows how
> > to deal with them by peeling, as you say, but there are 3 reasons I
> > dislike that this is allowed:
>
> That sounds as if you have tools that forgets to peel when it should
> in mind.  Isn't that what you should be looking into fixing?  After
> all, tools that aim to work with Git should strive to come close to
> "within the realm of git itsef" in the modern world.
>
> > 1. The intent by the user was to create a tag pointing to the commit
> > that another tag points to.
>
> You make it sound as if you are convinced that is the truth.  I am
> not.  If I want to tag the commit pointed at by tag v1.0, I'd say I
> want to tag "v1.0^0", because otherwise there won't be a way to say
>
>     $ git tag -s -m "i am aware of this tag" initial v1.0
>
> i.e. making a tag that points at a tag.  So I take the lack of ^0
> (or ^{commit}) peeling an explicit sign that shows the intent by the
> user (well, those who know the tool, anyway) is clearly to create a
> tag pointing to that tag.  In other words, peeling at the tagging
> time is wrong, and rejecting tag creation is also wrong.
>
> > 2. When users on my team do a `git show tag`, they see 2 descriptions
> > and 2 tags. This creates a LOT of confusion.
>
> So what?  Not everybody will forever stay to be a newbie ;-)
>
> As I said, an opt-in tag.allowCommitOnly is fine.  That would train
> their fingers to peel with ^{commit} when they want to tag a commit.
> An opt-in tag.autoPeelTags might also be fine, even though that
> would not help training their fingers (so they will have to be
> prepared for the same "confusion" on a fresh machine)
>
> When the command line clearly tells us that the user wants to tag a
> tag, we should not get overly "smart" and refuse to create a tag of
> a tag, unless the user tells us otherwise with some means.
>
> > 3. Even if Git internally handles peeling tags, external 3rd party
> > tooling may not. As I mentioned in another thread, `git lfs migrate`
> > was not programmed to peel tags. I reported the issue here:
>
> That is good, and it is the right way.  After all, external 3rd
> party tooling may tag a tag even after we castrate "git tag" to be
> incapable of doing so, so a bug like the one you are helping to fix
> in lfs needs to be fixed anyway.  In other words, thats the only
> sensible way forward when you care about the entire Git ecosystem,
> not just the main/reference implementation we work on here.
>

Look, I'm not saying you're wrong. You're probably absolutely right
about all the technical details. You know a lot more about this than I
do, that's for sure. But based on my observations and experience,
everything you're saying isn't set in reality. I have guys on my team
at work that haven't learned anything significant about Git in the
past 3 years; they don't care to learn it. They use a GUI tool for
everything and it's a means to an end for them. Most folks just want
to check out branches, merge, and push commits. They don't understand
what objects or blobs are, or what a tree vs tag vs commit is. Hell, I
have trouble explaining rebase to most people, and that's arguably
much more straightforward. But all of this is OK. We're hired to be
programmers, not experts at Git.

I think what needs to be right is what most people expect, and from my
experience that's not in alignment with your opinion on how this
should work. And I want to apologize if it seems like I'm arguing with
you. My intent is just to clarify my point of thinking. I feel like
you're still very wrapped up in the bowels of Git and the deep
technical details. I just want to make sure I'm being clear about my
perspective, since I'm approaching this from a somewhat non-technical
angle.

Again, regardless of what gets decided, I very much appreciate
everyone looking into this. I feel like whatever you folks come up
with will ultimately be the best decision, even if it may not be 100%
what I'm expecting.

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

[Johannes Sixt]

Am 03.04.19 um 09:32 schrieb Junio C Hamano:
> Denton Liu <liu.denton@gmail.com> writes:
> 
>> 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
> 
> The latter is a stronger indication that this should be OK than the
> former ;-)  Thanks for digging and noting.

However, there is a difference in whether %n$ is in the code or just in
the translations: When it is not in the code, Git can be made to work on
a platform that does not support it by compiling with NO_GETTEXT. When
it is in the code, that won't be possible anymore.

I don't know whether there are any platforms that do not support %n$,
though.

-- Hannes

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

[Denton Liu]

On Wed, Apr 03, 2019 at 04:32:27PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > 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
> 
> The latter is a stronger indication that this should be OK than the
> former ;-)  Thanks for digging and noting.

Thank Ævar, I shamelessly stole this message from one of his patches
that didn't get included in[1].

> 
> > 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.
> >  --
> 
> In addition to 'advice', we may have to add a configuration to help
> projects that wants to tag tag objects regularly so that they do not
> have to keep typing "--allow-nested-tag".  But that can wait until a
> participant of such a project comes forward and makes a case for
> their workflow.
> 
> > +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.
> 
> I am not sure if this is so bad, actually.  Why do we need to treat
> it as a mistake?  When a command that wants a commit is fed a tag
> (either a tag that directly refers to a commit, or a tag that tags
> another tag that refers to a commit), the command knows how to peel
> so it's not like the user is forced to say "git log T^{commit}".

This patch came about because Robert Dailey expressed confusion after
accidentally creating a tag-to-a-tag a while back by mistake when he
actually meant to amend a tag.

In the discussion upthread, Peff noted that he has never seen a
tag-to-a-tag in the wild before. I think the conclusion was that for
the majority of users, doing this is an error. That is what this patch
is guarding against.

> 
> And if something that *MUST* take a commit refuses to (or more
> likely, forges to) peel a tag down to a commit and yields an error,
> I think that is what needs fixing, not the command that creates a
> tag.
> 
> So, I am fairly negative on this change---unless it is made much
> more clear in the doc and/or in the proposed log message what
> practical downside there are to the end users if we do not stop this
> "mistake", that is.

I can update the log message to include more detail.

> 
> > +Automatically erroring on nested tags was introduced in Git version
> > +2.22.0.
> 
> And please do not write something like this.  A feature gets in a
> release when it is ready, and we may not ship this in 2.22.

Ævar suggested that we include this because git tag gets used by a lot
of scripts so in case one ever starts failing, a maintainer can more
easily track down the reason why.

> 
> "git tag --help" the user is running may or may not have the
> paragraph about "nested tag", depending on the existence of the
> feature in the version of Git the user is running, so there is no
> need to say something like that.
> 
> And no, I do not buy arguments like "random web servers serve
> different versions of documentation without identifying which
> version of Git they describe".
> 

Thanks for the comments,

Denton

[1]: https://public-inbox.org/git/20181026192734.9609-8-avarab@gmail.com/

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

[Jeff King]

On Wed, Apr 03, 2019 at 02:33:18PM -0700, Denton Liu wrote:

> > I am not sure if this is so bad, actually.  Why do we need to treat
> > it as a mistake?  When a command that wants a commit is fed a tag
> > (either a tag that directly refers to a commit, or a tag that tags
> > another tag that refers to a commit), the command knows how to peel
> > so it's not like the user is forced to say "git log T^{commit}".
> 
> This patch came about because Robert Dailey expressed confusion after
> accidentally creating a tag-to-a-tag a while back by mistake when he
> actually meant to amend a tag.
> 
> In the discussion upthread, Peff noted that he has never seen a
> tag-to-a-tag in the wild before. I think the conclusion was that for
> the majority of users, doing this is an error. That is what this patch
> is guarding against.

I do still think it is likely to be a mistake. I think Junio's point,
though is: who cares if the mistake was made? For the most part you can
continue to use the tag as if the mistake had never been made, because
Git peels through multiple layers as necessary.

The only thing that caused the discussion in the first place is that
when "git show" displays each layer, there was a little head-scratching.

So if we were starting from scratch and designing the behavior, I think
putting a safety valve around this mistake would probably be a
no-brainer.

But given that we're changing long-standing behavior (that somebody
_could_ be using intentionally), is it worth it to fix what is a mostly
harmless outcome?

I'm on the fence personally. I think I do still lean slightly towards
"yes, detect this in the porcelain", but I can see an argument both
ways.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do still think it is likely to be a mistake. I think Junio's point,
> though is: who cares if the mistake was made? For the most part you can
> continue to use the tag as if the mistake had never been made, because
> Git peels through multiple layers as necessary.

Nicely said.

If we forget to peel, that is a bigger problem, but I do not think
it makes any sense to single out tag-of-tag as "curious" and forbid
it, when we silently allow tag-of-blob or tag-of-tree happily.

An opt-in (i.e. default to false) tag.allowTaggingOnlyCommits I do
not have any problem with, and I could be persuaded into taking an
opt-out (i.e. default to true) tag.forbidTaggingAnythingButCommits
configuration, perhaps, though.

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

[Jeff King]

On Thu, Apr 04, 2019 at 06:31:25PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I do still think it is likely to be a mistake. I think Junio's point,
> > though is: who cares if the mistake was made? For the most part you can
> > continue to use the tag as if the mistake had never been made, because
> > Git peels through multiple layers as necessary.
> 
> Nicely said.
> 
> If we forget to peel, that is a bigger problem, but I do not think
> it makes any sense to single out tag-of-tag as "curious" and forbid
> it, when we silently allow tag-of-blob or tag-of-tree happily.

IMHO the difference between those cases is that it is very easy to type
something natural to get a tag of tag, like:

  git tag -m foo mytag
  # oops, try again
  git tag -m bar -f mytag mytag

but you have to take pretty specific steps to get a tag of a blob or
tree, like:

  git tag mytag HEAD:path

or

  git tag mytag HEAD^{tree}

Unless you have a ref pointing to a blob or tree in the first place, of
course. But then it is a chicken-and-egg question. Didn't you have to do
something specific to get that tag in the first place?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> but you have to take pretty specific steps to get a tag of a blob or
> tree, like:
>
>   git tag mytag HEAD:path
>
> or
>
>   git tag mytag HEAD^{tree}

And the way to ask for commit and tag are

    git tag mytag master
    git tag mytag v1.0.0

I do not see why only the last one should either be forbidden or
automaticallly peel.  Each of these four names an object of a
specific type, and singling out a tag as "curious" makes the
interface uneven.

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

[Jeff King]

On Fri, Apr 05, 2019 at 06:54:06AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > but you have to take pretty specific steps to get a tag of a blob or
> > tree, like:
> >
> >   git tag mytag HEAD:path
> >
> > or
> >
> >   git tag mytag HEAD^{tree}
> 
> And the way to ask for commit and tag are
> 
>     git tag mytag master
>     git tag mytag v1.0.0
> 
> I do not see why only the last one should either be forbidden or
> automaticallly peel.  Each of these four names an object of a
> specific type, and singling out a tag as "curious" makes the
> interface uneven.

My point is that it's easy to accidentally tag a tag. It's not easy to
accidentally tag a tree. The counter example would be something that
looks as easy as your "git tag mytag v1.0.0" but actually tags a tree.
I couldn't think of one.

I do buy the argument that arguing for safety valves from a point of
"what's the easiest mistake to make in our current interface" might not
be the best one. It yields a set of operations that aren't necessarily
sensible or orthogonal with respect to the underlying reality of what's
possible and what's reasonable. Which I think is the point you're
making.

-Peff

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

[Eckhard Maaß]

On Thu, Apr 04, 2019 at 08:27:22AM -0400, Jeff King wrote:
> IMHO the difference between those cases is that it is very easy to type
> something natural to get a tag of tag, like:
> 
>   git tag -m foo mytag
>   # oops, try again
>   git tag -m bar -f mytag mytag

Could it be that the semantics of `-f` are unintuitive and the real
culprit? The documentation talks about replacing the tag, which at least
naively to me has the sense of "well, the old tag object gets garbage
collected sometimes". But if the tag is still referenced, the old tag
hangs around. And this gets quite confusing. For example:

git tag -m 'Message' -a myTag
git tag -m 'Message' -a myOtherTag myTag
git tag -m 'Message' -f -a myTag otherBranch

Now a `git show myOtherTag` gives me the old myTag - and labels it as
myTag. Copy and pasting this directs me to somewhere completely else,
though. Wouldn't it be the more sane approach here to fail, when the old
tag is (or as in your example will) still be referenced? Or make some
effort with git show that it is clear that the referenced tag can no
longer be referenced with its original name?

Greetings,
Eckhard

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

[Junio C Hamano]

"Eckhard Maaß" <eckhard.s.maass@googlemail.com> writes:

> ... Wouldn't it be the more sane approach here to fail, when the old
> tag is (or as in your example will) still be referenced?

That is unworkable in the distributed world.  There may be somebody
else who has a copy of the old tag and you may end up getting that
later.

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

[Elijah Newren]

On Thu, Apr 4, 2019 at 2:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > I do still think it is likely to be a mistake. I think Junio's point,
> > though is: who cares if the mistake was made? For the most part you can
> > continue to use the tag as if the mistake had never been made, because
> > Git peels through multiple layers as necessary.
>
> Nicely said.
>
> If we forget to peel, that is a bigger problem, but I do not think
> it makes any sense to single out tag-of-tag as "curious" and forbid
> it, when we silently allow tag-of-blob or tag-of-tree happily.
>
> An opt-in (i.e. default to false) tag.allowTaggingOnlyCommits I do
> not have any problem with, and I could be persuaded into taking an
> opt-out (i.e. default to true) tag.forbidTaggingAnythingButCommits
> configuration, perhaps, though.

I'm slightly in favor of the tag.forbidTaggingAnythingButCommits
route.  Two reasons:

  * Even core git commands can't handle these properly after more than
a decade, making me suspect that tools in the greater git ecosystem
are going to fail to handle them too.  In more detail...  Some
examples: fast-export with --tag-of-filtered-object=rewrite fails on
tags of tags and tags of blobs.  Without that flag, I think
fast-export munges tags of tags, but maybe that was only under some
other special case; I don't remember right now.  Also, filter-branch
munges tags of tags (though maybe that's documented; it may have
decided that tags of tags are an error in need of fixing with no flag
for users to opt out).  Considering core tools that have been part of
git for over a decade mishandle tags of anything other than commits
(or maybe even treat them as erroneous), I don't see why we'd expect
tools outside of git to handle them correctly.  Thus, I think it'd be
nice if people had to specify some kind of way to state they are sure
they want to tag something other than a commit.

  * The only two repositories I know of and have access to which has
such tags are linux.git (a tag of a tree) and git.git (a tag of a blob
and four or so tags of tags).  Further, these tags are all pretty old
too.  So, I think disallowing the creation of tags of non-commit
objects would be unlikely to negatively impact existing users.


Though, on the flipside, given how rare these seem to be in practice,
it might not be worth the effort.  Certainly not at the top of my
priority list.

Elijah

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> I'm slightly in favor of the tag.forbidTaggingAnythingButCommits
> route.  Two reasons:
>
>   * Even core git commands can't handle these properly after more than
> a decade, making me suspect that tools in the greater git ecosystem
> are going to fail to handle them too.  In more detail...  Some
> examples: fast-export with --tag-of-filtered-object=rewrite fails on
> tags of tags and tags of blobs.  Without that flag, I think ...

Fair enough.

Personally, I've never considered import/export tools as part of the
git core proper, and it is time like this that I am reminded that I
have been right all along---they just do not get attention to the
same degree as the truly core tools.

But as we ship them together with the core part of the system, the
users may have been trained to think that tags to tags are not
something they want due to the limitation of these fringe tools.

[PATCH v3 0/2] tag: advise on recursive tagging

[Denton Liu]

[ Sorry for the spam, I typoed the mailing list address the first time ]

Hi all,

I've been following the discussion and the tl;dr of it seems to be this:
Junio wants to keep the long-standing behaviour of being able to git tag
anything while Robert seems to believe that for most users, it is a
mistake and even though Git is able unpeel tags, other tool authors may
not be aware that this is even possible and not handle the case.

I've tried to compromise between both sides. Now, we keep the old
behaviour but instead of silently ignoring nested tags, we print out a
hint that the user can act on. Hopefully, this should address everyone's
concerns.

The added benefit is that this won't break any existing scripts' unless
they are parsing stderr for whatever reason, but no one would do that,
right? ;)

Denton Liu (2):
  tag: fix formatting
  tag: advise on nested tags

 Documentation/config/advice.txt |  2 ++
 advice.c                        |  2 ++
 advice.h                        |  1 +
 builtin/tag.c                   | 23 +++++++++++++++++------
 t/t7004-tag.sh                  | 11 +++++++++++
 5 files changed, 33 insertions(+), 6 deletions(-)

-- 
2.21.0.843.gd0ae0373aa

[PATCH v3 1/2] tag: fix formatting

[Denton Liu]

Wrap usage line at '<tagname>'. Also, wrap strings with '\n' at the end
of string fragments instead of at the beginning of the next string
fragment.

Convert a space-indent into a tab-indent for style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/tag.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279..faae364e0f 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>]\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
 };
@@ -215,7 +216,7 @@ 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."));
 
 	strbuf_addf(&header,
 		    "object %s\n"
-- 
2.21.0.843.gd0ae0373aa

[PATCH v3 2/2] tag: advise on nested tags

[Denton Liu]

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 providing advice on nested tags. Add tests to ensure
that advice is given for nested tags.

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>
---

The advice message probably needs more work. Also, we currently only
warn on nested tags because I agree with what Peff said here[1], but I
don't really have any strong opinions on the matter so I'm open to
changing it.

[1]: https://public-inbox.org/git/20190404122722.GA23024@sigill.intra.peff.net/

 Documentation/config/advice.txt |  2 ++
 advice.c                        |  2 ++
 advice.h                        |  1 +
 builtin/tag.c                   | 14 ++++++++++++--
 t/t7004-tag.sh                  | 11 +++++++++++
 5 files changed, 28 insertions(+), 2 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/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..32948fade0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -206,7 +206,14 @@ struct create_tag_options {
 	} cleanup_mode;
 };
 
-static void create_tag(const struct object_id *object, const char *tag,
+static const char message_advice_nested_tag[] =
+	N_("You have created a nested tag. The object referred to by your new is\n"
+	   "already a tag. If you meant to tag the object that it points to, use:\n"
+	   "\n"
+	   "\tgit tag -f %s %s^{}");
+
+static void create_tag(const struct object_id *object, const char *object_ref,
+		       const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
 		       struct object_id *prev, struct object_id *result)
 {
@@ -218,6 +225,9 @@ static void create_tag(const struct object_id *object, const char *tag,
 	if (type <= OBJ_NONE)
 		die(_("bad object type."));
 
+	if (type == OBJ_TAG && advice_nested_tag)
+		advise(_(message_advice_nested_tag), tag, object_ref);
+
 	strbuf_addf(&header,
 		    "object %s\n"
 		    "type %s\n"
@@ -551,7 +561,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
-		create_tag(&object, tag, &buf, &opt, &prev, &object);
+		create_tag(&object, object_ref, tag, &buf, &opt, &prev, &object);
 	}
 
 	transaction = ref_transaction_begin(&err);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0b01862c23..34c130ba1c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1700,6 +1700,17 @@ test_expect_success '--points-at finds annotated tags of tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'recursive tagging should give advice' '
+	cat <<-EOF >expected &&
+	hint: You have created a nested tag. The object referred to by your new is
+	hint: already a tag. If you meant to tag the object that it points to, use:
+	hint: 
+	hint: 	git tag -f nested annotated-v4.0^{}
+	EOF
+	git tag -m nested nested annotated-v4.0 2>actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'multiple --points-at are OR-ed together' '
 	cat >expect <<-\EOF &&
 	v2.0
-- 
2.21.0.843.gd0ae0373aa