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

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:

quoted

quoted

On Fri, Sep 17, 2021 at 7:12 AM [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
 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