| Message ID | 20190402230345.GA5004@dev-l (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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".
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.
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
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?
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/
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
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.
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.
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
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.
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.
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.
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
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
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.
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.
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
"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.
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' '
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(-)