[PATCH] branch: advise about ref syntax rules

* [PATCH] branch: advise about ref syntax rules
@ 2024-03-01 15:38 Kristoffer Haugsbakk
  2024-03-01 18:06 ` Junio C Hamano
  2024-03-03 18:58 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
  0 siblings, 2 replies; 28+ messages in thread
From: Kristoffer Haugsbakk @ 2024-03-01 15:38 UTC (permalink / raw)
  To: git; +Cc: Kristoffer Haugsbakk

git-branch(1) will error out if you give it a bad ref name. But the user
might not understand why or what part of the name is illegal. The man
page for git-check-ref-format(1) contains these rules. Let’s advise
about it since that is not a command that you just happen upon.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Hopefully I am using `advice.h` correctly here.
    
    § git-replace(1)
    
    I did not add a hint for a similar message in `builtin/replace.c`.
    
    `builtin/replace.c` has an error message in `check_ref_valid` for an
    invalid ref name:
    
    ```
    return error(_("'%s' is not a valid ref name"), ref->buf);
    ```
    
    But there doesn’t seem to be a point to placing a hint here.
    
    The preceding calls to `repo_get_oid` will catch both missing refs and
    existing refs with invalid names:
    
    ```
     if (repo_get_oid(r, refname, &object))
    	 return error(_("failed to resolve '%s' as a valid ref"), refname);
    ```
    
    Like for example this:
    
    ```
    $ printf $(git rev-parse @~) > .git/refs/heads/hello..goodbye
    $ git replace @ hello..goodbye
    error: failed to resolve 'hello..goodbye' as a valid ref
    […]
    $ git replace @ non-existing
    error: failed to resolve 'non-existing' as a valid ref
    ```
    
    § Alternatives (to this change)
    
    While working on this I also thought that it might be nice to have a
    man page `gitrefsyntax`. That one could use a lot of the content from
    `man git check-ref-format` verbatim. Then the hint could point towards
    that man page. And it seems that AsciiDoc supports _includes_ which
    means that the rules don’t have to be duplicated between the two man
    pages.

 branch.c          |  7 +++++--
 builtin/branch.c  |  7 +++++--
 t/t3200-branch.sh | 10 ++++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/branch.c b/branch.c
index 6719a181bd1..1386918c60e 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,11 @@ 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))
-		die(_("'%s' is not a valid branch name"), name);
+	if (strbuf_check_branch_ref(ref, name)) {
+		error(_("'%s' is not a valid branch name"), name);
+		advise(_("See `man git check-ref-format`"));
+		exit(1);
+	}
 
 	return ref_exists(ref->buf);
 }
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..fa81e359157 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 		 */
 		if (ref_exists(oldref.buf))
 			recovery = 1;
-		else
-			die(_("invalid branch name: '%s'"), oldname);
+		else {
+			error(_("invalid branch name: '%s'"), oldname);
+			advise(_("See `man git check-ref-format`"));
+			exit(1);
+		}
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..9400a8baa35 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1725,4 +1725,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
 	test_cmp_config "" --default "" branch.foo5.merge
 '
 
+cat <<\EOF >expect
+error: 'foo..bar' is not a valid branch name
+hint: See `man git check-ref-format`
+EOF
+
+test_expect_success 'errors if given a bad branch name' '
+	test_must_fail git branch foo..bar >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0.106.g650c15c891b


^ permalink raw reply related	[flat|nested] 28+ messages in thread