| Message ID | 20241202070714.3028549-5-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New |
| 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
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(-)