[PATCH 0/4] forbid HEAD as a tagname

[PATCH 0/4] forbid HEAD as a tagname

[Junio C Hamano]

So, here is a cleaned up version, which ended up to be a 4-patch
series.

 - The first two steps move code around to get rid of the "strbuf_"
   prefix from three misnamed helper functions that are more about
   refs than they are about string manipulation operations.  We now
   have:

   - copy_branchname() that copies a branchname while applying
     interpret_branch_name() like @{-1} for previous branch, @{u}
     for upstream, etc.

   - check_branch_ref() and check_tag_ref() that are allowed to be a
     bit stricter than check_refname_format(), e.g., to reject "HEAD"
     as the name of a branch or a tag.

 - The third step updates a test that assumes HEAD can be usable as
   the name of a tag, but the breakage the test tries to protect is
   not specific to any tagname.

 - The final step then forbids "git tag" from using "HEAD" as the
   name of a tag.

Junio C Hamano (4):
  refs: move ref name helpers around
  refs: drop strbuf_ prefix from helpers
  t5604: do not expect that HEAD is a valid tagname
  tag: "git tag" refuses to use HEAD as a tagname

 branch.c                   |  2 +-
 builtin/branch.c           | 10 ++++----
 builtin/check-ref-format.c |  2 +-
 builtin/checkout.c         |  2 +-
 builtin/merge.c            |  2 +-
 builtin/tag.c              | 13 +----------
 builtin/worktree.c         |  8 +++----
 gitweb/gitweb.perl         |  2 +-
 object-name.c              | 36 -----------------------------
 refs.c                     | 47 ++++++++++++++++++++++++++++++++++++++
 refs.h                     | 29 +++++++++++++++++++++++
 strbuf.h                   | 22 ------------------
 t/t5604-clone-reference.sh |  6 ++---
 t/t7004-tag.sh             |  6 +++++
 14 files changed, 100 insertions(+), 87 deletions(-)

-- 
2.47.1-514-g9b43e7ecc4

[PATCH 1/4] refs: move ref name helpers around

[Junio C Hamano]

strbuf_branchname(), strbuf_check_{branch,tag}_ref() are helper
functions to deal with branch and tag names, and the fact that they
happen to use strbuf to hold the name of a branch or a tag is not
essential.  These functions fit better in the refs API than strbuf
API, the latter of which is about string manipulations.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/tag.c | 11 -----------
 object-name.c | 36 ------------------------------------
 refs.c        | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 refs.h        | 29 +++++++++++++++++++++++++++++
 strbuf.h      | 22 ----------------------
 5 files changed, 76 insertions(+), 69 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 93d10d5915..8279dccbe0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -447,17 +447,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
-{
-	if (name[0] == '-')
-		return -1;
-
-	strbuf_reset(sb);
-	strbuf_addf(sb, "refs/tags/%s", name);
-
-	return check_refname_format(sb->buf, 0);
-}
-
 int cmd_tag(int argc,
 	    const char **argv,
 	    const char *prefix,
diff --git a/object-name.c b/object-name.c
index c892fbe80a..9f2ae164e4 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1734,42 +1734,6 @@ int repo_interpret_branch_name(struct repository *r,
 	return -1;
 }
 
-void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
-{
-	int len = strlen(name);
-	struct interpret_branch_name_options options = {
-		.allowed = allowed
-	};
-	int used = repo_interpret_branch_name(the_repository, name, len, sb,
-					      &options);
-
-	if (used < 0)
-		used = 0;
-	strbuf_add(sb, name + used, len - used);
-}
-
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
-{
-	if (startup_info->have_repository)
-		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
-	else
-		strbuf_addstr(sb, name);
-
-	/*
-	 * This splice must be done even if we end up rejecting the
-	 * name; builtin/branch.c::copy_or_rename_branch() still wants
-	 * to see what the name expanded to so that "branch -m" can be
-	 * used as a tool to correct earlier mistakes.
-	 */
-	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
-
-	if (*name == '-' ||
-	    !strcmp(sb->buf, "refs/heads/HEAD"))
-		return -1;
-
-	return check_refname_format(sb->buf, 0);
-}
-
 void object_context_release(struct object_context *ctx)
 {
 	free(ctx->path);
diff --git a/refs.c b/refs.c
index 5f729ed412..59a9223d4c 100644
--- a/refs.c
+++ b/refs.c
@@ -697,6 +697,53 @@ static char *substitute_branch_name(struct repository *r,
 	return NULL;
 }
 
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
+{
+	int len = strlen(name);
+	struct interpret_branch_name_options options = {
+		.allowed = allowed
+	};
+	int used = repo_interpret_branch_name(the_repository, name, len, sb,
+					      &options);
+
+	if (used < 0)
+		used = 0;
+	strbuf_add(sb, name + used, len - used);
+}
+
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	if (startup_info->have_repository)
+		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	else
+		strbuf_addstr(sb, name);
+
+	/*
+	 * This splice must be done even if we end up rejecting the
+	 * name; builtin/branch.c::copy_or_rename_branch() still wants
+	 * to see what the name expanded to so that "branch -m" can be
+	 * used as a tool to correct earlier mistakes.
+	 */
+	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+	if (*name == '-' ||
+	    !strcmp(sb->buf, "refs/heads/HEAD"))
+		return -1;
+
+	return check_refname_format(sb->buf, 0);
+}
+
+int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
+{
+	if (name[0] == '-')
+		return -1;
+
+	strbuf_reset(sb);
+	strbuf_addf(sb, "refs/tags/%s", name);
+
+	return check_refname_format(sb->buf, 0);
+}
+
 int repo_dwim_ref(struct repository *r, const char *str, int len,
 		  struct object_id *oid, char **ref, int nonfatal_dangling_mark)
 {
diff --git a/refs.h b/refs.h
index 108dfc93b3..f19b0ad92f 100644
--- a/refs.h
+++ b/refs.h
@@ -180,6 +180,35 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_
  */
 char *repo_default_branch_name(struct repository *r, int quiet);
 
+/*
+ * Copy "name" to "sb", expanding any special @-marks as handled by
+ * repo_interpret_branch_name(). The result is a non-qualified branch name
+ * (so "foo" or "origin/master" instead of "refs/heads/foo" or
+ * "refs/remotes/origin/master").
+ *
+ * Note that the resulting name may not be a syntactically valid refname.
+ *
+ * If "allowed" is non-zero, restrict the set of allowed expansions. See
+ * repo_interpret_branch_name() for details.
+ */
+void strbuf_branchname(struct strbuf *sb, const char *name,
+		       unsigned allowed);
+
+/*
+ * Like strbuf_branchname() above, but confirm that the result is
+ * syntactically valid to be used as a local branch name in refs/heads/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
+
+/*
+ * Similar for a tag name in refs/tags/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
+int strbuf_check_tag_ref(struct strbuf *sb, const char *name);
+
 /*
  * A ref_transaction represents a collection of reference updates that
  * should succeed or fail together.
diff --git a/strbuf.h b/strbuf.h
index 003f880ff7..4dc05b4ba7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -637,28 +637,6 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
-/*
- * Copy "name" to "sb", expanding any special @-marks as handled by
- * repo_interpret_branch_name(). The result is a non-qualified branch name
- * (so "foo" or "origin/master" instead of "refs/heads/foo" or
- * "refs/remotes/origin/master").
- *
- * Note that the resulting name may not be a syntactically valid refname.
- *
- * If "allowed" is non-zero, restrict the set of allowed expansions. See
- * repo_interpret_branch_name() for details.
- */
-void strbuf_branchname(struct strbuf *sb, const char *name,
-		       unsigned allowed);
-
-/*
- * Like strbuf_branchname() above, but confirm that the result is
- * syntactically valid to be used as a local branch name in refs/heads/.
- *
- * The return value is "0" if the result is valid, and "-1" otherwise.
- */
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
-
 typedef int (*char_predicate)(char ch);
 
 void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,
-- 
2.47.1-514-g9b43e7ecc4

Re: [PATCH 1/4] refs: move ref name helpers around

[Jeff King]

On Mon, Dec 02, 2024 at 04:07:11PM +0900, Junio C Hamano wrote:

> strbuf_branchname(), strbuf_check_{branch,tag}_ref() are helper
> functions to deal with branch and tag names, and the fact that they
> happen to use strbuf to hold the name of a branch or a tag is not
> essential.  These functions fit better in the refs API than strbuf
> API, the latter of which is about string manipulations.

Wow, they are declared in strbuf.h but not even implemented there. So it
was doubly confusing. This looks like a nice cleanup.

-Peff

Re: [PATCH 1/4] refs: move ref name helpers around

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 02, 2024 at 04:07:11PM +0900, Junio C Hamano wrote:
>
>> strbuf_branchname(), strbuf_check_{branch,tag}_ref() are helper
>> functions to deal with branch and tag names, and the fact that they
>> happen to use strbuf to hold the name of a branch or a tag is not
>> essential.  These functions fit better in the refs API than strbuf
>> API, the latter of which is about string manipulations.
>
> Wow, they are declared in strbuf.h but not even implemented there. So it
> was doubly confusing. This looks like a nice cleanup.

Yup.  Another home that may want to adopt them is the object-name
API, but I think refs API is good enough, and certainly better than
strbuf.

[PATCH 2/4] refs: drop strbuf_ prefix from helpers

[Junio C Hamano]

The helper functions (strbuf_branchname, strbuf_check_branch_ref,
and strbuf_check_tag_ref) are about handling branch and tag names,
and it is a non-essential fact that these functions use strbuf to
hold these names.  Rename them to make it clarify that these are
more about "ref".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c                   |  2 +-
 builtin/branch.c           | 10 +++++-----
 builtin/check-ref-format.c |  2 +-
 builtin/checkout.c         |  2 +-
 builtin/merge.c            |  2 +-
 builtin/tag.c              |  2 +-
 builtin/worktree.c         |  8 ++++----
 gitweb/gitweb.perl         |  2 +-
 refs.c                     |  8 ++++----
 refs.h                     |  8 ++++----
 10 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/branch.c b/branch.c
index 08fa4094d2..58b61831af 100644
--- a/branch.c
+++ b/branch.c
@@ -372,7 +372,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
  */
 int validate_branchname(const char *name, struct strbuf *ref)
 {
-	if (strbuf_check_branch_ref(ref, name)) {
+	if (check_branch_ref(ref, name)) {
 		int code = die_message(_("'%s' is not a valid branch name"), name);
 		advise_if_enabled(ADVICE_REF_SYNTAX,
 				  _("See `man git check-ref-format`"));
diff --git a/builtin/branch.c b/builtin/branch.c
index fd1611ebf5..17acf598d2 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -257,7 +257,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		char *target = NULL;
 		int flags = 0;
 
-		strbuf_branchname(&bname, argv[i], allowed_interpret);
+		copy_branchname(&bname, argv[i], allowed_interpret);
 		free(name);
 		name = mkpathdup(fmt, bname.buf);
 
@@ -579,7 +579,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	int recovery = 0, oldref_usage = 0;
 	struct worktree **worktrees = get_worktrees();
 
-	if (strbuf_check_branch_ref(&oldref, oldname)) {
+	if (check_branch_ref(&oldref, oldname)) {
 		/*
 		 * Bad name --- this could be an attempt to rename a
 		 * ref that we used to allow to be created by accident.
@@ -894,7 +894,7 @@ int cmd_branch(int argc,
 				die(_("cannot give description to detached HEAD"));
 			branch_name = head;
 		} else if (argc == 1) {
-			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			copy_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
 			branch_name = buf.buf;
 		} else {
 			die(_("cannot edit description of more than one branch"));
@@ -933,7 +933,7 @@ int cmd_branch(int argc,
 		if (!argc)
 			branch = branch_get(NULL);
 		else if (argc == 1) {
-			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			copy_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
 			branch = branch_get(buf.buf);
 		} else
 			die(_("too many arguments to set new upstream"));
@@ -963,7 +963,7 @@ int cmd_branch(int argc,
 		if (!argc)
 			branch = branch_get(NULL);
 		else if (argc == 1) {
-			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
+			copy_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
 			branch = branch_get(buf.buf);
 		} else
 			die(_("too many arguments to unset upstream"));
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index e86d8ef980..cef1ffe3ce 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -42,7 +42,7 @@ static int check_ref_format_branch(const char *arg)
 	int nongit;
 
 	setup_git_directory_gently(&nongit);
-	if (strbuf_check_branch_ref(&sb, arg) ||
+	if (check_branch_ref(&sb, arg) ||
 	    !skip_prefix(sb.buf, "refs/heads/", &name))
 		die("'%s' is not a valid branch name", arg);
 	printf("%s\n", name);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c449558e66..5e5afa0f26 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -742,7 +742,7 @@ static void setup_branch_path(struct branch_info *branch)
 			   &branch->oid, &branch->refname, 0))
 		repo_get_oid_committish(the_repository, branch->name, &branch->oid);
 
-	strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
+	copy_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
 	if (strcmp(buf.buf, branch->name)) {
 		free(branch->name);
 		branch->name = xstrdup(buf.buf);
diff --git a/builtin/merge.c b/builtin/merge.c
index 84d0f3604b..d0c31d7714 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -498,7 +498,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	char *found_ref = NULL;
 	int len, early;
 
-	strbuf_branchname(&bname, remote, 0);
+	copy_branchname(&bname, remote, 0);
 	remote = bname.buf;
 
 	oidclr(&branch_head, the_repository->hash_algo);
diff --git a/builtin/tag.c b/builtin/tag.c
index 8279dccbe0..670e564178 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -639,7 +639,7 @@ int cmd_tag(int argc,
 	if (repo_get_oid(the_repository, object_ref, &object))
 		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
 
-	if (strbuf_check_tag_ref(&ref, tag))
+	if (check_tag_ref(&ref, tag))
 		die(_("'%s' is not a valid tag name."), tag);
 
 	if (refs_read_ref(get_main_ref_store(the_repository), ref.buf, &prev))
diff --git a/builtin/worktree.c b/builtin/worktree.c
index fc31d072a6..c68f601358 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -432,7 +432,7 @@ static int add_worktree(const char *path, const char *refname,
 	worktrees = NULL;
 
 	/* is 'refname' a branch or commit? */
-	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
+	if (!opts->detach && !check_branch_ref(&symref, refname) &&
 	    refs_ref_exists(get_main_ref_store(the_repository), symref.buf)) {
 		is_branch = 1;
 		if (!opts->force)
@@ -604,7 +604,7 @@ static void print_preparing_worktree_line(int detach,
 		fprintf_ln(stderr, _("Preparing worktree (new branch '%s')"), new_branch);
 	} else {
 		struct strbuf s = STRBUF_INIT;
-		if (!detach && !strbuf_check_branch_ref(&s, branch) &&
+		if (!detach && !check_branch_ref(&s, branch) &&
 		    refs_ref_exists(get_main_ref_store(the_repository), s.buf))
 			fprintf_ln(stderr, _("Preparing worktree (checking out '%s')"),
 				  branch);
@@ -745,7 +745,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
 
-	branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
+	branch_exists = !check_branch_ref(&ref, branchname) &&
 			refs_ref_exists(get_main_ref_store(the_repository),
 					ref.buf);
 	strbuf_release(&ref);
@@ -838,7 +838,7 @@ static int add(int ac, const char **av, const char *prefix)
 		new_branch = new_branch_force;
 
 		if (!opts.force &&
-		    !strbuf_check_branch_ref(&symref, new_branch) &&
+		    !check_branch_ref(&symref, new_branch) &&
 		    refs_ref_exists(get_main_ref_store(the_repository), symref.buf))
 			die_if_checked_out(symref.buf, 0);
 		strbuf_release(&symref);
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b09a8d0523..8cdb0d9b9f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2094,7 +2094,7 @@ sub format_log_line_html {
         (
             # The output of "git describe", e.g. v2.10.0-297-gf6727b0
             # or hadoop-20160921-113441-20-g094fb7d
-            (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
+            (?<!-) # see check_tag_ref(). Tags can't start with -
             [A-Za-z0-9.-]+
             (?!\.) # refs can't end with ".", see check_refname_format()
             -g$regex
diff --git a/refs.c b/refs.c
index 59a9223d4c..a24bfe3845 100644
--- a/refs.c
+++ b/refs.c
@@ -697,7 +697,7 @@ static char *substitute_branch_name(struct repository *r,
 	return NULL;
 }
 
-void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
+void copy_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 {
 	int len = strlen(name);
 	struct interpret_branch_name_options options = {
@@ -711,10 +711,10 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 	strbuf_add(sb, name + used, len - used);
 }
 
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+int check_branch_ref(struct strbuf *sb, const char *name)
 {
 	if (startup_info->have_repository)
-		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+		copy_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
 	else
 		strbuf_addstr(sb, name);
 
@@ -733,7 +733,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
-int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
+int check_tag_ref(struct strbuf *sb, const char *name)
 {
 	if (name[0] == '-')
 		return -1;
diff --git a/refs.h b/refs.h
index f19b0ad92f..c5280477f0 100644
--- a/refs.h
+++ b/refs.h
@@ -191,23 +191,23 @@ char *repo_default_branch_name(struct repository *r, int quiet);
  * If "allowed" is non-zero, restrict the set of allowed expansions. See
  * repo_interpret_branch_name() for details.
  */
-void strbuf_branchname(struct strbuf *sb, const char *name,
+void copy_branchname(struct strbuf *sb, const char *name,
 		       unsigned allowed);
 
 /*
- * Like strbuf_branchname() above, but confirm that the result is
+ * Like copy_branchname() above, but confirm that the result is
  * syntactically valid to be used as a local branch name in refs/heads/.
  *
  * The return value is "0" if the result is valid, and "-1" otherwise.
  */
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
+int check_branch_ref(struct strbuf *sb, const char *name);
 
 /*
  * Similar for a tag name in refs/tags/.
  *
  * The return value is "0" if the result is valid, and "-1" otherwise.
  */
-int strbuf_check_tag_ref(struct strbuf *sb, const char *name);
+int check_tag_ref(struct strbuf *sb, const char *name);
 
 /*
  * A ref_transaction represents a collection of reference updates that
-- 
2.47.1-514-g9b43e7ecc4

Re: [PATCH 2/4] refs: drop strbuf_ prefix from helpers

[Jeff King]

On Mon, Dec 02, 2024 at 04:07:12PM +0900, Junio C Hamano wrote:

> The helper functions (strbuf_branchname, strbuf_check_branch_ref,
> and strbuf_check_tag_ref) are about handling branch and tag names,
> and it is a non-essential fact that these functions use strbuf to
> hold these names.  Rename them to make it clarify that these are
> more about "ref".

Sounds good. I wasn't quite sure about the name copy_branchname(), since
it actually expands/interprets the name. But the word "interpret" is
already used for another similar function, repo_interpret_branch_name().

In fact, this function is a very thin wrapper around it, which made me
wonder if it has any value. It looks like the main useful bit is that on
error it will copy the name verbatim.

So I guess it is really more like copy_or_expand_branchname(). I don't
know if that is really adding much, though. Probably just the name
copy_branchname(), coupled with the documentation above the declaration,
will be sufficient.

  As a side note, repo_interpret_branch_name() is in object-file.[ch],
  but probably should also be in refs.[ch], as in your first patch.
  Let's not worry about it for your series, though.

-Peff

[PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Junio C Hamano]

09116a1c (refs: loosen over-strict "format" check, 2011-11-16)
introduced a test piece (originally in t5700) that expects to be
able to create a tag named "HEAD" and then a local clone using the
repository as its own reference works correctly.  Later, another
test piece started using this tag starting at acede2eb (t5700:
document a failure of alternates to affect fetch, 2012-02-11).

But the breakage 09116a1c fixed was not specific to the tagname
HEAD.  It would have failed exactly the same way if the tag used
were foo instead of HEAD.

Before forbidding "git tag" from creating "refs/tags/HEAD", update
these tests to use 'foo', not 'HEAD', as the name of the test tag.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5604-clone-reference.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 9b32db8478..5f5c650ff8 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -131,7 +131,7 @@ test_expect_success 'cloning with multiple references drops duplicates' '
 
 test_expect_success 'clone with reference from a tagged repository' '
 	(
-		cd A && git tag -a -m tagged HEAD
+		cd A && git tag -a -m tagged foo
 	) &&
 	git clone --reference=A A I
 '
@@ -156,10 +156,10 @@ test_expect_success 'fetch with incomplete alternates' '
 		git remote add J "file://$base_dir/J" &&
 		GIT_TRACE_PACKET=$U.K git fetch J
 	) &&
-	main_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/main) &&
+	main_object=$(git -C A rev-parse --verify refs/heads/main) &&
 	test -s "$U.K" &&
 	! grep " want $main_object" "$U.K" &&
-	tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
+	tag_object=$(git -C A rev-parse --verify refs/tags/foo) &&
 	! grep " want $tag_object" "$U.K"
 '
 
-- 
2.47.1-514-g9b43e7ecc4

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Kristoffer Haugsbakk]

On Mon, Dec 2, 2024, at 08:07, Junio C Hamano wrote:
> 09116a1c (refs: loosen over-strict "format" check, 2011-11-16)

Nit/confusion: the abbreviated hash is only eight hexes long.  I’m used to it
being 11 for this project?

Does the age of the commit matter?

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Jeff King]

On Mon, Dec 02, 2024 at 01:19:56PM +0100, Kristoffer Haugsbakk wrote:

> On Mon, Dec 2, 2024, at 08:07, Junio C Hamano wrote:
> > 09116a1c (refs: loosen over-strict "format" check, 2011-11-16)
> 
> Nit/confusion: the abbreviated hash is only eight hexes long.  I’m used to it
> being 11 for this project?

It's not a fixed size. Long ago, the rule was "enough to be unique, but
at least 7 (or whatever you set core.abbrev to)". These days that "7" is
scaled based on the number of objects in the repo. See e6c587c733
(abbrev: auto size the default abbreviation, 2016-09-30).

So I'd expect 10 digits in a fresh clone of git.git. It's possible Junio
has set core.abbrev to something fixed, though.

> Does the age of the commit matter?

Nope, it shouldn't.

-Peff

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Kristoffer Haugsbakk]

On Mon, Dec 2, 2024, at 22:00, Jeff King wrote:
> On Mon, Dec 02, 2024 at 01:19:56PM +0100, Kristoffer Haugsbakk wrote:
>
>> On Mon, Dec 2, 2024, at 08:07, Junio C Hamano wrote:
>> > 09116a1c (refs: loosen over-strict "format" check, 2011-11-16)
>>
>> Nit/confusion: the abbreviated hash is only eight hexes long.  I’m used to it
>> being 11 for this project?
>
> It's not a fixed size. Long ago, the rule was "enough to be unique, but
> at least 7 (or whatever you set core.abbrev to)". These days that "7" is
> scaled based on the number of objects in the repo. See e6c587c733
> (abbrev: auto size the default abbreviation, 2016-09-30).

Yes.  11 was based on the output I get as well as what seemed normal in the
recent git log.

>
> So I'd expect 10 digits in a fresh clone of git.git. It's possible Junio
> has set core.abbrev to something fixed, though.
>
>> Does the age of the commit matter?
>
> Nope, it shouldn't.

Makes sense.  Thanks.

-- 
Kristoffer Haugsbakk

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I'd expect 10 digits in a fresh clone of git.git. It's possible Junio
> has set core.abbrev to something fixed, though.

Thanks.

I have "git one" (and "git who") aliased to this script:

    $ cat $(type --path git-onewho)
    #!/bin/sh
    if sha1=$(git rev-parse -q --verify "$1")
    then
            git show --date=short -s --abbrev=8 --pretty='format:%h (%s, %ad)' "$1"
    else
            git log -1 --format="%aN <%aE>" --author="$1" --all
    fi | tr -d "\012"
    $ git help one
    'one' is aliased to 'onewho'
    $ git help who
    'who' is aliased to 'onewho'
    
so that I can say "\C-u ESC ! git one HEAD" (or "git one peff")
while writing a piece of e-mail.  I can drop --abbrev=8 from there
but the machinery knows to bust that limit if it is necessary to
ensure uniqueness, so ...

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Jeff King]

On Tue, Dec 03, 2024 at 10:29:14AM +0900, Junio C Hamano wrote:

> I have "git one" (and "git who") aliased to this script:
> 
>     $ cat $(type --path git-onewho)
>     #!/bin/sh
>     if sha1=$(git rev-parse -q --verify "$1")
>     then
>             git show --date=short -s --abbrev=8 --pretty='format:%h (%s, %ad)' "$1"
>     else
>             git log -1 --format="%aN <%aE>" --author="$1" --all
>     fi | tr -d "\012"
>     $ git help one
>     'one' is aliased to 'onewho'
>     $ git help who
>     'who' is aliased to 'onewho'
>     
> so that I can say "\C-u ESC ! git one HEAD" (or "git one peff")
> while writing a piece of e-mail.  I can drop --abbrev=8 from there
> but the machinery knows to bust that limit if it is necessary to
> ensure uniqueness, so ...

Yeah, I have something similar. IMHO a manual --abbrev there is working
against your goal.

We do increase that to find a unique answer, but that is not very
future-proof; it is only extending by one character taking into account
what objects you have _now_. It might not be true for somebody else's
repo with more objects, or even your own repo in the near future.

The auto-scaling of core.abbrev done by default these days also suffers
from that problem (it can only count how many objects you have now, not
how many you expect to have a year from now). But I think our heuristics
there give a bit higher safety margin for future-proofing the values.

-Peff

Re: [PATCH 3/4] t5604: do not expect that HEAD is a valid tagname

[Jeff King]

On Mon, Dec 02, 2024 at 04:07:13PM +0900, Junio C Hamano wrote:

> 09116a1c (refs: loosen over-strict "format" check, 2011-11-16)
> introduced a test piece (originally in t5700) that expects to be
> able to create a tag named "HEAD" and then a local clone using the
> repository as its own reference works correctly.  Later, another
> test piece started using this tag starting at acede2eb (t5700:
> document a failure of alternates to affect fetch, 2012-02-11).
> 
> But the breakage 09116a1c fixed was not specific to the tagname
> HEAD.  It would have failed exactly the same way if the tag used
> were foo instead of HEAD.
> 
> Before forbidding "git tag" from creating "refs/tags/HEAD", update
> these tests to use 'foo', not 'HEAD', as the name of the test tag.

Yeah, I think this is worth doing independently. The patch looks good,
though...

> @@ -131,7 +131,7 @@ test_expect_success 'cloning with multiple references drops duplicates' '
>  
>  test_expect_success 'clone with reference from a tagged repository' '
>  	(
> -		cd A && git tag -a -m tagged HEAD
> +		cd A && git tag -a -m tagged foo
>  	) &&
>  	git clone --reference=A A I
>  '
> @@ -156,10 +156,10 @@ test_expect_success 'fetch with incomplete alternates' '
>  		git remote add J "file://$base_dir/J" &&
>  		GIT_TRACE_PACKET=$U.K git fetch J
>  	) &&
> -	main_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/main) &&
> +	main_object=$(git -C A rev-parse --verify refs/heads/main) &&
>  	test -s "$U.K" &&
>  	! grep " want $main_object" "$U.K" &&
> -	tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
> +	tag_object=$(git -C A rev-parse --verify refs/tags/foo) &&
>  	! grep " want $tag_object" "$U.K"
>  '

I notice that you swapped out "cd A && git" for "git -C A" in the second
hunk (evne in the line which does not otherwise need to be touched). I
think that is good, but is it worth doing the same in the first hunk?
That would actually let us drop the subshell.

-Peff

[PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Junio C Hamano]

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(-)

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" \
-- 
2.47.1-514-g9b43e7ecc4

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Patrick Steinhardt]

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

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[shejialuo]

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

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

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

Check the list archive for the past few days; it was considered and
rejected.

Thanks.

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Jeff King]

On Tue, Dec 03, 2024 at 10:30:15AM +0900, Junio C Hamano wrote:

> shejialuo <shejialuo@gmail.com> writes:
> 
> >>  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.
> 
> Check the list archive for the past few days; it was considered and
> rejected.

Agreed, but maybe that is an indication we should discuss that
alternative in the commit message. (I had looked for similar
justification in the existing "branch" restriction, but couldn't find
it, to the point that I wondered if I had hallucinated our reasoning
back then).

-Peff

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Jeff King]

On Thu, Dec 05, 2024 at 03:26:47PM -0500, Jeff King wrote:

> > > 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.
> > 
> > Check the list archive for the past few days; it was considered and
> > rejected.
> 
> Agreed, but maybe that is an indication we should discuss that
> alternative in the commit message. (I had looked for similar
> justification in the existing "branch" restriction, but couldn't find
> it, to the point that I wondered if I had hallucinated our reasoning
> back then).

Ah, nevermind. I just read your v2 and I think it covers this nicely.

-Peff

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Rubén Justo]

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
> 

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> --- 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
> +'

This concisely captures exactly what we want to see.  The plumbing
is still allowed to create something that the worldview given by the
Git Porcelain UI may not like (so that people can create different UI)
and our tool can still be usable to recover from a mistake of using
HEAD as the name of a tag, which we more strongly discourage.

Good idea.

>> +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 :-)

It is not.  If "git tag HEAD" get broken in the future, the
must-fail steps below may create such a tag, and we want to remove
it before moving along.

Re: [PATCH 4/4] tag: "git tag" refuses to use HEAD as a tagname

[Jeff King]

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