| Message ID | 20241202070714.3028549-5-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | forbid HEAD as a tagname | expand |
On Mon, Dec 02, 2024 at 04:07:14PM +0900, Junio C Hamano wrote: > diff --git a/refs.c b/refs.c > index a24bfe3845..01ef2a3093 100644 > --- a/refs.c > +++ b/refs.c > @@ -735,7 +735,7 @@ int check_branch_ref(struct strbuf *sb, const char *name) > > int check_tag_ref(struct strbuf *sb, const char *name) > { > - if (name[0] == '-') > + if (name[0] == '-' || !strcmp(name, "HEAD")) > return -1; > > strbuf_reset(sb); I was thinking a bit about whether we can spin this a bit wider and disallow creation of any refname that looks like a root ref. But I don't think that would make sense, as root refs are defined as all-uppercase refs living in the root, and disallowing tags that look like this would go way too far. So I then wondered what a reasonable alternative would be, and the only rule I could come up with was to disallow root refs with "HEAD" in it. But even that doesn't feel reasonable to me. All to say: singling out "HEAD" feels like a sensible step, and I don't think we should handle root refs more generally here. The other patches look good to me, as well. Thanks! Patrick
On Mon, Dec 02, 2024 at 04:07:14PM +0900, Junio C Hamano wrote: [snip] > diff --git a/refs.c b/refs.c > index a24bfe3845..01ef2a3093 100644 > --- a/refs.c > +++ b/refs.c > @@ -735,7 +735,7 @@ int check_branch_ref(struct strbuf *sb, const char *name) > > int check_tag_ref(struct strbuf *sb, const char *name) > { > - if (name[0] == '-') > + if (name[0] == '-' || !strcmp(name, "HEAD")) I am wondering whether we should also update "check_refname_format" function to report "refs/heads/HEAD" and "refs/tags/HEAD" is bad ref name. > return -1; > > strbuf_reset(sb); Thanks, Jialuo
On Mon, Dec 02, 2024 at 04:07:14PM +0900, Junio C Hamano wrote: > Even though the plumbing level allows you to create refs/tags/HEAD > and refs/heads/HEAD, doing so makes it confusing within the context > of the UI Git Porcelain commands provides. Just like we prevent a > branch from getting called "HEAD" at the Porcelain layer (i.e. "git > branch" command), teach "git tag" to refuse to create a tag "HEAD". This sounds like a good step in the right direction for me. From the subject in this patch, I was worried that we were also preventing deletion. However, I have confirmed that we still allow the intuitive deletion of a tag named 'HEAD' with "git tag -d HEAD"; for example, in repositories where such a tag already exists. Perhaps tangential, but a silly change like this hasn't broken any tests: diff --git a/builtin/tag.c b/builtin/tag.c index 670e564178..b65f56e5b4 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -88,6 +88,8 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn, for (p = argv; *p; p++) { strbuf_reset(&ref); + if (!strcmp(*p, "HEAD")) + die("Hi!"); strbuf_addf(&ref, "refs/tags/%s", *p); if (refs_read_ref(get_main_ref_store(the_repository), ref.buf, &oid)) { error(_("tag '%s' not found."), *p); Therefore, if the previous seems reasonable, perhaps we should add a test like: --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -97,6 +97,11 @@ test_expect_success 'HEAD is forbidden as a tagname' ' test_must_fail git tag -a -m "useless" HEAD ' +test_expect_success 'allow deleting a tag named HEAD' ' + git update-ref refs/tags/HEAD HEAD && + git tag -d HEAD +' + test_expect_success 'creating a tag with --create-reflog should create reflog' ' git log -1 \ --format="format:tag: tagging %h (%s, %cd)%n" \ > > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > refs.c | 2 +- > t/t7004-tag.sh | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/refs.c b/refs.c > index a24bfe3845..01ef2a3093 100644 > --- a/refs.c > +++ b/refs.c > @@ -735,7 +735,7 @@ int check_branch_ref(struct strbuf *sb, const char *name) > > int check_tag_ref(struct strbuf *sb, const char *name) > { > - if (name[0] == '-') > + if (name[0] == '-' || !strcmp(name, "HEAD")) > return -1; > > strbuf_reset(sb); > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index b1316e62f4..2082ce63f7 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -91,6 +91,12 @@ test_expect_success 'creating a tag using default HEAD should succeed' ' > test_must_fail git reflog exists refs/tags/mytag > ' > > +test_expect_success 'HEAD is forbidden as a tagname' ' > + test_when_finished "git tag -d HEAD || :" && I'm not considering this as a test for it :-) > + test_must_fail git tag HEAD && > + test_must_fail git tag -a -m "useless" HEAD > +' > + > test_expect_success 'creating a tag with --create-reflog should create reflog' ' > git log -1 \ > --format="format:tag: tagging %h (%s, %cd)%n" \ > -- > 2.47.1-514-g9b43e7ecc4 >
On Mon, Dec 02, 2024 at 04:07:14PM +0900, Junio C Hamano wrote: > Even though the plumbing level allows you to create refs/tags/HEAD > and refs/heads/HEAD, doing so makes it confusing within the context > of the UI Git Porcelain commands provides. Just like we prevent a > branch from getting called "HEAD" at the Porcelain layer (i.e. "git > branch" command), teach "git tag" to refuse to create a tag "HEAD". This looks good and mostly as expected. I do think Rubén's suggestion to add an explicit deletion test might be worth having to future-proof things. > @@ -91,6 +91,12 @@ test_expect_success 'creating a tag using default HEAD should succeed' ' > test_must_fail git reflog exists refs/tags/mytag > ' > > +test_expect_success 'HEAD is forbidden as a tagname' ' > + test_when_finished "git tag -d HEAD || :" && > + test_must_fail git tag HEAD && > + test_must_fail git tag -a -m "useless" HEAD > +' The test_when_finished surprised me a little, just because we would not expect anything to have been created. I don't think we usually bother with cleaning up failure modes, as it is a losing battle (if the test did not succeed you are only guessing at what mess may have been left behind). But I don't think it's hurting anything, beyond a few wasted cycles to run what should be a noop. -Peff
shejialuo <shejialuo@gmail.com> writes: >> int check_tag_ref(struct strbuf *sb, const char *name) >> { >> - if (name[0] == '-') >> + if (name[0] == '-' || !strcmp(name, "HEAD")) > > I am wondering whether we should also update "check_refname_format" > function to report "refs/heads/HEAD" and "refs/tags/HEAD" is bad ref > name. Check the list archive for the past few days; it was considered and rejected. Thanks.
Rubén Justo <rjusto@gmail.com> writes: > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -97,6 +97,11 @@ test_expect_success 'HEAD is forbidden as a tagname' ' > test_must_fail git tag -a -m "useless" HEAD > ' > > +test_expect_success 'allow deleting a tag named HEAD' ' > + git update-ref refs/tags/HEAD HEAD && > + git tag -d HEAD > +' This concisely captures exactly what we want to see. The plumbing is still allowed to create something that the worldview given by the Git Porcelain UI may not like (so that people can create different UI) and our tool can still be usable to recover from a mistake of using HEAD as the name of a tag, which we more strongly discourage. Good idea. >> +test_expect_success 'HEAD is forbidden as a tagname' ' >> + test_when_finished "git tag -d HEAD || :" && > > I'm not considering this as a test for it :-) It is not. If "git tag HEAD" get broken in the future, the must-fail steps below may create such a tag, and we want to remove it before moving along.
On Tue, Dec 03, 2024 at 10:30:15AM +0900, Junio C Hamano wrote: > shejialuo <shejialuo@gmail.com> writes: > > >> int check_tag_ref(struct strbuf *sb, const char *name) > >> { > >> - if (name[0] == '-') > >> + if (name[0] == '-' || !strcmp(name, "HEAD")) > > > > I am wondering whether we should also update "check_refname_format" > > function to report "refs/heads/HEAD" and "refs/tags/HEAD" is bad ref > > name. > > Check the list archive for the past few days; it was considered and > rejected. Agreed, but maybe that is an indication we should discuss that alternative in the commit message. (I had looked for similar justification in the existing "branch" restriction, but couldn't find it, to the point that I wondered if I had hallucinated our reasoning back then). -Peff
On Thu, Dec 05, 2024 at 03:26:47PM -0500, Jeff King wrote: > > > I am wondering whether we should also update "check_refname_format" > > > function to report "refs/heads/HEAD" and "refs/tags/HEAD" is bad ref > > > name. > > > > Check the list archive for the past few days; it was considered and > > rejected. > > Agreed, but maybe that is an indication we should discuss that > alternative in the commit message. (I had looked for similar > justification in the existing "branch" restriction, but couldn't find > it, to the point that I wondered if I had hallucinated our reasoning > back then). Ah, nevermind. I just read your v2 and I think it covers this nicely. -Peff
diff --git a/refs.c b/refs.c index a24bfe3845..01ef2a3093 100644 --- a/refs.c +++ b/refs.c @@ -735,7 +735,7 @@ int check_branch_ref(struct strbuf *sb, const char *name) int check_tag_ref(struct strbuf *sb, const char *name) { - if (name[0] == '-') + if (name[0] == '-' || !strcmp(name, "HEAD")) return -1; strbuf_reset(sb); diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index b1316e62f4..2082ce63f7 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -91,6 +91,12 @@ test_expect_success 'creating a tag using default HEAD should succeed' ' test_must_fail git reflog exists refs/tags/mytag ' +test_expect_success 'HEAD is forbidden as a tagname' ' + test_when_finished "git tag -d HEAD || :" && + test_must_fail git tag HEAD && + test_must_fail git tag -a -m "useless" HEAD +' + test_expect_success 'creating a tag with --create-reflog should create reflog' ' git log -1 \ --format="format:tag: tagging %h (%s, %cd)%n" \
Even though the plumbing level allows you to create refs/tags/HEAD and refs/heads/HEAD, doing so makes it confusing within the context of the UI Git Porcelain commands provides. Just like we prevent a branch from getting called "HEAD" at the Porcelain layer (i.e. "git branch" command), teach "git tag" to refuse to create a tag "HEAD". Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- refs.c | 2 +- t/t7004-tag.sh | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)