threads / discuss / 56522
Subject: Should `@` be really a valid git tag name?
Hello Git-Community,
by accident I did:
git tag -a -s @
git push --tagsin 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.
Carlo ArenasRe: Should `@` be really a valid git tag name?
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
Junio C HamanoRe: Should `@` be really a valid git tag name?
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.
Jeff KingRe: Should `@` be really a valid git tag name?
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
Carlo Marcelo Arenas BelónRe: 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: > > > 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
Junio C HamanoRe: Should `@` be really a valid git tag name?
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.
Carlo ArenasRe: Should `@` be really a valid git tag name?
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/
Junio C HamanoRe: Should `@` be really a valid git tag name?
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.
Carlo Marcelo Arenas BelónRe: Should `@` be really a valid git tag name?
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
Chris TorekRe: Should `@` be really a valid git tag name?
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.