Should `@` be really a valid git tag name?

Should `@` be really a valid git tag name?

[gitmailinglist.bentolor]

Hello Git-Community,

by accident I did:

    git tag -a -s @
    git push --tags

in v2.25.1 and pushed the tag named `@` to Github and had some minutes
of fun to get rid of it again (see: https://stackoverflow.com/a/69211383)

A SO commenter pointed out, that git-check-ref-format forbids @ and
maybe I should report this as potential bug. Is it?


Best,
Benjamin

P.S.: I'm not subscribed, so please CC me.

Re: Should `@` be really a valid git tag name?

[Carlo Arenas]

On Fri, Sep 17, 2021 at 7:12 AM <gitmailinglist.bentolor@xoxy.net> wrote:
>
> A SO commenter pointed out, that git-check-ref-format forbids @ and
> maybe I should report this as a potential bug. Is it?

a reference that is named "@" only is invalid, but refs/tags/@ is not.

Carlo

Re: Should `@` be really a valid git tag name?

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Fri, Sep 17, 2021 at 7:12 AM <gitmailinglist.bentolor@xoxy.net> wrote:
>>
>> A SO commenter pointed out, that git-check-ref-format forbids @ and
>> maybe I should report this as a potential bug. Is it?
>
> a reference that is named "@" only is invalid, but refs/tags/@ is not.

;-)  

"git check-ref-format master ; echo $?" would show that any single
level name is "forbidden", so probably the SO commenter (whatever
that is) was confused---it is not about @ at all.

In any case, a tag whose name is @ may be another source of
confusion in the modern world, after we added @ as a synonym to
HEAD.  I do not know, for example, offhand which between the HEAD or
that tag "git show @" would choose.  It makes sense to avoid it.

Re: Should `@` be really a valid git tag name?

[Jeff King]

On Fri, Sep 17, 2021 at 01:53:52PM -0700, Junio C Hamano wrote:

> Carlo Arenas <carenas@gmail.com> writes:
> 
> > On Fri, Sep 17, 2021 at 7:12 AM <gitmailinglist.bentolor@xoxy.net> wrote:
> >>
> >> A SO commenter pointed out, that git-check-ref-format forbids @ and
> >> maybe I should report this as a potential bug. Is it?
> >
> > a reference that is named "@" only is invalid, but refs/tags/@ is not.
> 
> ;-)  
> 
> "git check-ref-format master ; echo $?" would show that any single
> level name is "forbidden", so probably the SO commenter (whatever
> that is) was confused---it is not about @ at all.
> 
> In any case, a tag whose name is @ may be another source of
> confusion in the modern world, after we added @ as a synonym to
> HEAD.  I do not know, for example, offhand which between the HEAD or
> that tag "git show @" would choose.  It makes sense to avoid it.

In the past when we've had confusing names (like refs/heads/HEAD), we
continue to allow them at the plumbing level (to retain backwards
compatibility), but flag them at the porcelain level to prevent users
shooting themselves in the foot. This seems like a good candidate for
that (for both git-branch and git-tag).

-Peff

Re: Should `@` be really a valid git tag name?

[Carlo Marcelo Arenas Belón]

On Fri, Sep 17, 2021 at 05:47:17PM -0400, Jeff King wrote:
> On Fri, Sep 17, 2021 at 01:53:52PM -0700, Junio C Hamano wrote:
> > > On Fri, Sep 17, 2021 at 7:12 AM <gitmailinglist.bentolor@xoxy.net> wrote:
> > >>
> > >> A SO commenter pointed out, that git-check-ref-format forbids @ and
> > >> maybe I should report this as a potential bug. Is it?
> > >
> > > a reference that is named "@" only is invalid, but refs/tags/@ is not.
> > 
> > ;-)  
> > 
> > "git check-ref-format master ; echo $?" would show that any single
> > level name is "forbidden", so probably the SO commenter (whatever
> > that is) was confused---it is not about @ at all.
> > 
> > In any case, a tag whose name is @ may be another source of
> > confusion in the modern world, after we added @ as a synonym to
> > HEAD.  I do not know, for example, offhand which between the HEAD or
> > that tag "git show @" would choose.  It makes sense to avoid it.
> 
> In the past when we've had confusing names (like refs/heads/HEAD), we
> continue to allow them at the plumbing level (to retain backwards
> compatibility), but flag them at the porcelain level to prevent users
> shooting themselves in the foot. This seems like a good candidate for
> that (for both git-branch and git-tag).

I was leaning towards something like that plus a Documentation update, but
noticed that the current behaviour was inconsistent, and the confusion
pointed out by Junio seems to indicate it is better if fully restricted.

Carlo

----- >8 -----
Subject: [PATCH] refs: mark "@" as an invalid refspec

9ba89f484e (Add new @ shortcut for HEAD, 2013-09-02), adds "@" to
the list of invalid refspec, but does it only for full refspec and
not as a component.

Move the logic to validate it at the component level to prevent also
tags to be created with that name.

Reported-by: Benjamin <gitmailinglist.bentolor@xoxy.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 refs.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 8b9f7c3a80..2ca6995c0f 100644
--- a/refs.c
+++ b/refs.c
@@ -47,13 +47,14 @@ int ref_storage_backend_exists(const char *name)
  * 4: A bad character: ASCII control characters, and
  *    ":", "?", "[", "\", "^", "~", SP, or TAB
  * 5: *, reject unless REFNAME_REFSPEC_PATTERN is set
+ * 6: @, only valid if used around valid characters
  */
 static unsigned char refname_disposition[256] = {
 	1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
 	4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
 	4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 2, 1,
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,
-	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
@@ -137,6 +138,14 @@ static int check_refname_component(const char *refname, int *flags,
 			 */
 			*flags &= ~ REFNAME_REFSPEC_PATTERN;
 			break;
+		case 6:
+			if (last == '\0' && *(cp + 1) == '\0') {
+				if (sanitized)
+					sanitized->buf[sanitized->len] = '-';
+				else
+					return -1;
+			}
+			break;
 		}
 		last = ch;
 	}
@@ -167,14 +176,6 @@ static int check_or_sanitize_refname(const char *refname, int flags,
 {
 	int component_len, component_count = 0;
 
-	if (!strcmp(refname, "@")) {
-		/* Refname is a single character '@'. */
-		if (sanitized)
-			strbuf_addch(sanitized, '-');
-		else
-			return -1;
-	}
-
 	while (1) {
 		if (sanitized && sanitized->len)
 			strbuf_complete(sanitized, '/');
-- 
2.33.0.911.gbe391d4e11

Re: Should `@` be really a valid git tag name?

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> I was leaning towards something like that plus a Documentation update, but
> noticed that the current behaviour was inconsistent, and the confusion
> pointed out by Junio seems to indicate it is better if fully restricted.

That is a bad move, as existing repositories may have a ref with
such a name.

If we tighten anything retroactively, we probably should forgid '@'
short-hand that stands for HEAD, I would think.

Re: Should `@` be really a valid git tag name?

[Carlo Arenas]

On Fri, Sep 17, 2021 at 4:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>
> > I was leaning towards something like that plus a Documentation update, but
> > noticed that the current behaviour was inconsistent, and the confusion
> > pointed out by Junio seems to indicate it is better if fully restricted.
>
> That is a bad move, as existing repositories may have a ref with
> such a name.

but if they do, it is currently "ambiguous" as you pointed out, and my
patch still allows the use of @ when it is not ambiguous :

  $ git branch @/1 @

so Stefano[1] is safe, and anyone that has such a ref is better of
renaming it anyway (which is something I agree we have to add code to
my patch to allow somehow)

> If we tighten anything retroactively, we probably should forgid '@'
> short-hand that stands for HEAD, I would think.

regardless of the merits of that feature, it has been in git since
~v1.8.4, so its removal will also be breaking the user experience
IMHO.

Carlo

[1] https://lore.kernel.org/git/520BC017.7050907@gmail.com/

Re: Should `@` be really a valid git tag name?

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Fri, Sep 17, 2021 at 4:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>>
>> > I was leaning towards something like that plus a Documentation update, but
>> > noticed that the current behaviour was inconsistent, and the confusion
>> > pointed out by Junio seems to indicate it is better if fully restricted.
>>
>> That is a bad move, as existing repositories may have a ref with
>> such a name.
> ...
>> If we tighten anything retroactively, we probably should forgid '@'
>> short-hand that stands for HEAD, I would think.
>
> regardless of the merits of that feature, it has been in git since
> ~v1.8.4, so its removal will also be breaking the user experience
> IMHO.

I know, but it is lessor of two evils.

I think it is OK to forbid at the higher level Porcelain, while
still allowing read access, but keep the door open for plumbing,
just like Peff suggested.

Re: Should `@` be really a valid git tag name?

[Carlo Marcelo Arenas Belón]

On Fri, Sep 17, 2021 at 05:25:52PM -0700, Junio C Hamano wrote:
> 
> I think it is OK to forbid at the higher level Porcelain, while
> still allowing read access, but keep the door open for plumbing,
> just like Peff suggested.

I still think it would be better to forbid it fully (at least longterm)
and while I am not advocating for (neither against) the feature that
required a character to be reserved, I think it is good to have a way
to reserve more characters if needed, and so this might help as a POC.

The patch could use a better message, and covers all porcelain points
I am aware of (branch, tag, checkout and switch), but leaves push/pull
intentionally open.

I think preventing push might be worth adding, but I am concerned it
might be too intrusive; I coded a warning for tag, but I frankly suspect
no one really HAS a tag like this that they really want to keep, and the
reported problem behaves better with the new code (local/remote tag can
be removed normally):

  $ git log --oneline
  813e919 (HEAD -> master, tag: a, tag: @@, tag: @1, tag: 1@, origin/master, @) HEAD
  d52caf3 (tag: z, tag: foo, tag: bar, tag: @) init
  $ git tag -d @
  Deleted tag '@' (was d52caf3)
  $ git push origin :@
  To origin
   - [deleted]         @

Carlo
----- >8 -----
Subject: [RFC PATCH] refs: mark "@" as an invalid refname in the porcelain

9ba89f484e (Add new @ shortcut for HEAD, 2013-09-02) declares "@"
as an invalid refname, but only blocked it as a full refname and
not when a component of one, leaving a loophole that was tested
in t3204.11, even if ambiguous.

Remove the check and instead add it at the porcelain level, so
users will be blocked of creating tags or branches named "@", but
still allowed to delete or rename them in a consistent way.

To help transition, add a warning if "@" is used as a branch, so
that check could be removed and implemente properly in the future.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 branch.c                              | 7 +++++++
 builtin/branch.c                      | 3 +++
 builtin/checkout.c                    | 5 ++++-
 builtin/tag.c                         | 3 +++
 refs.c                                | 8 --------
 t/t3204-branch-name-interpretation.sh | 8 ++++----
 6 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index 7a88a4861e..a577a3ddc1 100644
--- a/branch.c
+++ b/branch.c
@@ -185,6 +185,13 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
  */
 int validate_branchname(const char *name, struct strbuf *ref)
 {
+	/*
+	 * since 9ba89f484e (Add new @ shortcut for HEAD, 2013-09-02)
+	 * "@" is no longer a valid reference.
+	 */
+	if (!strcmp(name, "@"))
+		die(_("'@' is an ambiguous refname"));
+
 	if (strbuf_check_branch_ref(ref, name))
 		die(_("'%s' is not a valid branch name."), name);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index b23b1d1752..7a5a10ad82 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -857,6 +857,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (track == BRANCH_TRACK_OVERRIDE)
 			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
 
+		if (!strcmp(argv[0], "@"))
+			die(_("'@' is ambiguous"));
+
 		create_branch(the_repository,
 			      argv[0], (argc == 2) ? argv[1] : head,
 			      force, 0, reflog, quiet, track);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a..bc92a2c723 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1151,8 +1151,11 @@ static void setup_new_branch_info_and_source_tree(
 	setup_branch_path(new_branch_info);
 
 	if (!check_refname_format(new_branch_info->path, 0) &&
-	    !read_ref(new_branch_info->path, &branch_rev))
+	    !read_ref(new_branch_info->path, &branch_rev)) {
 		oidcpy(rev, &branch_rev);
+		if (!strcmp(new_branch_info->name, "@"))
+			warning("ambiguous name, rename this branch ASAP");
+	}
 	else {
 		free((char *)new_branch_info->path);
 		new_branch_info->path = NULL; /* not an existing branch */
diff --git a/builtin/tag.c b/builtin/tag.c
index 82fcfc0982..357efc37f8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -608,6 +608,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	if (!strcmp(tag, "@"))
+		die(_("'@' is ambiguous"));
+
 	create_reflog_msg(&object, &reflog_msg);
 
 	if (create_tag_object) {
diff --git a/refs.c b/refs.c
index 8b9f7c3a80..6b5d869bf5 100644
--- a/refs.c
+++ b/refs.c
@@ -167,14 +167,6 @@ static int check_or_sanitize_refname(const char *refname, int flags,
 {
 	int component_len, component_count = 0;
 
-	if (!strcmp(refname, "@")) {
-		/* Refname is a single character '@'. */
-		if (sanitized)
-			strbuf_addch(sanitized, '-');
-		else
-			return -1;
-	}
-
 	while (1) {
 		if (sanitized && sanitized->len)
 			strbuf_complete(sanitized, '/');
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..862a5dff8e 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -110,11 +110,11 @@ test_expect_success 'disallow deleting remote branch via @{-1}' '
 
 # The thing we are testing here is that "@" is the real branch refs/heads/@,
 # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
-# sane thing, but it _is_ technically allowed for now. If we disallow it, these
-# can be switched to test_must_fail.
+# sane thing, and should go away once "@" is correctly marked as an invalid
+# refname
 test_expect_success 'create branch named "@"' '
-	git branch -f @ one &&
-	expect_branch refs/heads/@ one
+	test_must_fail git branch -f @ one 2>err &&
+	grep "fatal: '\''@'\'' is ambiguous" err
 '
 
 test_expect_success 'delete branch named "@"' '
-- 
2.33.0.911.gbe391d4e11

Re: Should `@` be really a valid git tag name?

[Chris Torek]

I was in fact the commenter, and I wasn't saying
that it is a bug in check-ref-format, but rather that
since `@` by itself is forbidden, perhaps the creation
of a tag or branch named `@` should be forbidden.
Maybe it should even be forbidden as any component
part, in new names, although obviously we would
have to allow deleting existing names!

I knew this would be a bit of a hornet's nest though,

Chris

On Fri, Sep 17, 2021 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > On Fri, Sep 17, 2021 at 7:12 AM <gitmailinglist.bentolor@xoxy.net> wrote:
> >>
> >> A SO commenter pointed out, that git-check-ref-format forbids @ and
> >> maybe I should report this as a potential bug. Is it?
> >
> > a reference that is named "@" only is invalid, but refs/tags/@ is not.
>
> ;-)
>
> "git check-ref-format master ; echo $?" would show that any single
> level name is "forbidden", so probably the SO commenter (whatever
> that is) was confused---it is not about @ at all.
>
> In any case, a tag whose name is @ may be another source of
> confusion in the modern world, after we added @ as a synonym to
> HEAD.  I do not know, for example, offhand which between the HEAD or
> that tag "git show @" would choose.  It makes sense to avoid it.