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

Carlo Arenas [off-list ref] writes:

On Fri, Sep 17, 2021 at 4:06 PM Junio C Hamano [off-list ref] wrote:

quoted

Carlo Marcelo Arenas Belón [off-list ref] writes:

quoted

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.

...

quoted

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.

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 <redacted>
---
 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