[PATCH] branch: advise about ref syntax rules

[PATCH] branch: advise about ref syntax rules

[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

Re: [PATCH] branch: advise about ref syntax rules

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Notes (series):
>     Hopefully I am using `advice.h` correctly here.

Let's see.

> -	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);
> +	}

This will give the message with "hint:" prefix, which is a good
starting point.

The message is given unconditionally, without any way to turn it
off.  For those who ...

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

... do not understand why, it is helpful, but once they learned, it
is one extra line of unwanted text.  If you want to give it a way to
squelch, see the comment before where enum advice_type is declared
in advice.h header file.  The callsites would become something like

	advise_if_enabled(ADVICE_VALID_REF_NAME,
		_("See `man git check-ref-format` for valid refname syntax."));

Another thing is that rewriting die() into error() + advice() +
manual exit() is an anti-pattern these days.

	int code = die_message(_("'%s' is not a valid branch name"), name);
	advice_if_enabled(...); /* see above */
	exit(code);

In the same source file, you will find an existing example to mimic.

Thanks.

Re: [PATCH] branch: advise about ref syntax rules

[Kristoffer Haugsbakk]

Hi

> This will give the message with "hint:" prefix, which is a good
> starting point.
>
> The message is given unconditionally, without any way to turn it
> off.  For those who ...
>
>> 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.
>
> ... do not understand why, it is helpful, but once they learned, it
> is one extra line of unwanted text.  If you want to give it a way to
> squelch, see the comment before where enum advice_type is declared
> in advice.h header file.

I thought of doing that, but I reckoned that people who have a good
intuition for the ref syntax would not get this error enough to want to
turn if off.

I’ll add a squelch option in the next version.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH] branch: advise about ref syntax rules

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> I thought of doing that, but I reckoned that people who have a good
> intuition for the ref syntax would not get this error enough to want to
> turn if off.

If that is your choice, that is perfectly OK, as long as the
proposed log message clearly records why we did not bother using
advice_if_enabled().

If that is the case, then a rewrite for existing die() would become:

	int code = die_message(_("'%s' is not a valid branch name"), name);
	advise(_("See `man git check-ref-format`"));
	exit(code);

Thanks.

[PATCH v2 0/1] advise about ref syntax rules

[Kristoffer Haugsbakk]

Point the user towards the ref/branch name syntax rules if they give an
invalid name.

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

§ Changes in v2

• Make the advise optional via configuration
  • At first I thought that this wasn’t needed but I imagine the advice
    could get repetitive for typos and such
• Propagate error properly with `die_message(…)` instead of `exit(1)`
• Flesh out commit message a bit

Kristoffer Haugsbakk (1):
  branch: advise about ref syntax rules

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        |  8 ++++++--
 builtin/branch.c                |  8 ++++++--
 t/t3200-branch.sh               | 11 +++++++++++
 6 files changed, 28 insertions(+), 4 deletions(-)

-- 
2.44.0.64.g52b67adbeb2

[PATCH v2 1/1] branch: advise about ref syntax rules

[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 user might know that there are some limitations based on the *loose
ref* format (filenames), but there are also further rules for
easier integration with shell-based tools, pathname expansion, and
playing well with reference name expressions.

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. Also make this advise configurable since you might not want to be
reminded every time you make a little typo.

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

Notes (series):
    v2:
    • Make the advise optional via configuration
    • Propagate error properly with `die_message(…)` instead of `exit(1)`
    • Flesh out commit message a bit

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        |  8 ++++++--
 builtin/branch.c                |  8 ++++++--
 t/t3200-branch.sh               | 11 +++++++++++
 6 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..552cfbcd48c 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -94,6 +94,9 @@ advice.*::
 		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
 		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
 		simultaneously.
+	refSyntax::
+		Point the user towards the ref syntax documentation if
+		they give an invalid ref name.
 	resetNoRefresh::
 		Advice to consider using the `--no-refresh` option to
 		linkgit:git-reset[1] when the command takes more than 2 seconds
diff --git a/advice.c b/advice.c
index 6e9098ff089..550c2968908 100644
--- a/advice.c
+++ b/advice.c
@@ -68,6 +68,7 @@ static struct {
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
 	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_REF_SYNTAX]				= { "refSyntax" },
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
 	[ADVICE_RM_HINTS]				= { "rmHints" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..d15fe2351ab 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
 	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+	ADVICE_REF_SYNTAX,
 	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
diff --git a/branch.c b/branch.c
index 6719a181bd1..621019fcf4b 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,12 @@ 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)) {
+		int code = die_message(_("'%s' is not a valid branch name"), name);
+		advise_if_enabled(ADVICE_REF_SYNTAX,
+				  _("See `man git check-ref-format`"));
+		exit(code);
+	}
 
 	return ref_exists(ref->buf);
 }
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..1c122ee8a7b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,12 @@ 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 {
+			int code = die_message(_("invalid branch name: '%s'"), oldname);
+			advise_if_enabled(ADVICE_REF_SYNTAX,
+					  _("See `man git check-ref-format`"));
+			exit(code);
+		}
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..d21fdf09c90 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1725,4 +1725,15 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
 	test_cmp_config "" --default "" branch.foo5.merge
 '
 
+cat <<\EOF >expect
+fatal: 'foo..bar' is not a valid branch name
+hint: See `man git check-ref-format`
+hint: Disable this message with "git config advice.refSyntax false"
+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.64.g52b67adbeb2

Re: [PATCH v2 1/1] branch: advise about ref syntax rules

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:


This has sufficiently been advanced since the previous one, that
range-diff would need to be prodded with a larger --creation-factor
value to avoid getting a rather useless output.

1:  5548e6fa34 < -:  ---------- branch: advise about ref syntax rules
-:  ---------- > 1:  202d4e29ef branch: advise about ref syntax rules

> 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 user might know that there are some limitations based on the *loose
> ref* format (filenames), but there are also further rules for
> easier integration with shell-based tools, pathname expansion, and
> playing well with reference name expressions.
>
> 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. Also make this advise configurable since you might not want to be
> reminded every time you make a little typo.

Nicely written and easily read.  Well done.

> +	refSyntax::
> +		Point the user towards the ref syntax documentation if
> +		they give an invalid ref name.

I noticed a minor phrasing issue, but many other entries talk about
"shown when ...", even though a handful of them use "if ...".  Do we
want to make them consistent?

>  	resetNoRefresh::
>  		Advice to consider using the `--no-refresh` option to
>  		linkgit:git-reset[1] when the command takes more than 2 seconds

> diff --git a/advice.c b/advice.c
> index 6e9098ff089..550c2968908 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -68,6 +68,7 @@ static struct {
>  	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
>  	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
>  	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
> +	[ADVICE_REF_SYNTAX]				= { "refSyntax" },
>  	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
>  	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
>  	[ADVICE_RM_HINTS]				= { "rmHints" },
> diff --git a/advice.h b/advice.h
> index 9d4f49ae38b..d15fe2351ab 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -36,6 +36,7 @@ enum advice_type {
>  	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
>  	ADVICE_PUSH_UPDATE_REJECTED,
>  	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
> +	ADVICE_REF_SYNTAX,
>  	ADVICE_RESET_NO_REFRESH_WARNING,
>  	ADVICE_RESOLVE_CONFLICT,
>  	ADVICE_RM_HINTS,

Both of these are in lexicographic order, which is good.

> diff --git a/branch.c b/branch.c
> index 6719a181bd1..621019fcf4b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -370,8 +370,12 @@ 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)) {
> +		int code = die_message(_("'%s' is not a valid branch name"), name);
> +		advise_if_enabled(ADVICE_REF_SYNTAX,
> +				  _("See `man git check-ref-format`"));
> +		exit(code);
> +	}

Nice.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index cfb63cce5fb..1c122ee8a7b 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -576,8 +576,12 @@ 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 {
> +			int code = die_message(_("invalid branch name: '%s'"), oldname);
> +			advise_if_enabled(ADVICE_REF_SYNTAX,
> +					  _("See `man git check-ref-format`"));
> +			exit(code);
> +		}
>  	}

Good, too.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index de7d3014e4f..d21fdf09c90 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1725,4 +1725,15 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
>  	test_cmp_config "" --default "" branch.foo5.merge
>  '
>  
> +cat <<\EOF >expect
> +fatal: 'foo..bar' is not a valid branch name
> +hint: See `man git check-ref-format`
> +hint: Disable this message with "git config advice.refSyntax false"
> +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
> +'

Even though there are a few ancient style tests that have code to
set up expectations outside the test_expect_success, most of the
tests in t3200 do use a more modern style.  Let's not make it worse,
by moving it inside, perhaps like:

test_expect_success 'errors if given a bad branch name' '
        cat >expect <<-\EOF &&
        fatal: '\''foo..bar'\'' is not a valid branch name
        hint: See `man git check-ref-format`
        hint: Disable this message with "git config advice.refSyntax false"
        EOF
	test_must_fail git branch foo..bar >actual 2>&1 &&
	test_cmp expect actual
'

We could make a preliminary clean-up to the file in question before
adding the above test, if we wanted to.  Or we can do so after the
dust settles.  Such a fix may look like the attached.

Thanks.

 t/t3200-branch.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git c/t/t3200-branch.sh w/t/t3200-branch.sh
index 94b536ef51..ba1e0eace5 100755
--- c/t/t3200-branch.sh
+++ w/t/t3200-branch.sh
@@ -1112,14 +1112,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups
 	test_cmp expect actual
 "
 
-# Keep this test last, as it changes the current branch
-cat >expect <<EOF
-$HEAD refs/heads/g/h/i@{0}: branch: Created from main
-EOF
 test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git checkout -b g/h/i -l main &&
 	test_ref_exists refs/heads/g/h/i &&
+
+	cat >expect <<-EOF &&
+	$HEAD refs/heads/g/h/i@{0}: branch: Created from main
+	EOF
 	git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
 	test_cmp expect actual
 '

Re: [PATCH v2 1/1] branch: advise about ref syntax rules

[Kristoffer Haugsbakk]

On Sun, Mar 3, 2024, at 23:42, Junio C Hamano wrote:
>> +	refSyntax::
>> +		Point the user towards the ref syntax documentation if
>> +		they give an invalid ref name.
>
> I noticed a minor phrasing issue, but many other entries talk about
> "shown when ...", even though a handful of them use "if ...".  Do we
> want to make them consistent?

Sure thing. Do you prefer the “shown when” alternative?

-- 
Kristoffer Haugsbakk

[PATCH v3 0/5] advise about ref syntax rules

[Kristoffer Haugsbakk]

Point the user towards the ref/branch name syntax rules if they give an
invalid name.

Also make some spatially-appropriate improvements:

• Test style
• `advice.txt`

§ git-replace(1)

(see previous cover letter)

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

§ CC

For changes to `advice.txt`:

Cc: Elijah Newren <newren@gmail.com>
Cc: Jean-Noël Avila <avila.jn@gmail.com>

§ Changes in v3

• New preliminary patches 1–4
  • Fix test style
  • Improvements to `advice.txt` (style consistency and other)
• Patch 5/5:
  • Tweak advice doc for the new entry
  • Better test style

Kristoffer Haugsbakk (5):
  t3200: improve test style
  advice: make all entries stylistically consistent
  advice: use backticks for code
  advice: use double quotes for regular quoting
  branch: advise about ref syntax rules

 Documentation/config/advice.txt |  91 ++++++++++++-----------
 advice.c                        |   1 +
 advice.h                        |   1 +
 branch.c                        |   8 +-
 builtin/branch.c                |   8 +-
 t/t3200-branch.sh               | 125 +++++++++++++++++---------------
 6 files changed, 127 insertions(+), 107 deletions(-)

Range-diff against v2:
-:  ----------- > 1:  e6a2628ce57 t3200: improve test style
-:  ----------- > 2:  d48b4719c27 advice: make all entries stylistically consistent
-:  ----------- > 3:  30d662a04c7 advice: use backticks for code
-:  ----------- > 4:  3028713357f advice: use double quotes for regular quoting
1:  4ad5d419064 ! 5:  402b7937951 branch: advise about ref syntax rules
    @@ Commit message
     
     
      ## Notes (series) ##
    +    v3:
    +    • Tweak advice doc for the new entry
    +    • Better test style
         v2:
         • Make the advise optional via configuration
         • Propagate error properly with `die_message(…)` instead of `exit(1)`
    @@ Notes (series)
     
      ## Documentation/config/advice.txt ##
     @@ Documentation/config/advice.txt: advice.*::
    - 		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
    - 		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
    + 		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
    + 		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
      		simultaneously.
     +	refSyntax::
    -+		Point the user towards the ref syntax documentation if
    -+		they give an invalid ref name.
    ++		Shown when the user provides an illegal ref name: point
    ++		towards the ref syntax documentation.
      	resetNoRefresh::
    - 		Advice to consider using the `--no-refresh` option to
    - 		linkgit:git-reset[1] when the command takes more than 2 seconds
    + 		Shown when linkgit:git-reset[1] takes more than 2
    + 		seconds to refresh the index after reset: tell the user
     
      ## advice.c ##
     @@ advice.c: static struct {
    @@ t/t3200-branch.sh: test_expect_success '--track overrides branch.autoSetupMerge'
      	test_cmp_config "" --default "" branch.foo5.merge
      '
      
    -+cat <<\EOF >expect
    -+fatal: 'foo..bar' is not a valid branch name
    -+hint: See `man git check-ref-format`
    -+hint: Disable this message with "git config advice.refSyntax false"
    -+EOF
    -+
     +test_expect_success 'errors if given a bad branch name' '
    ++	cat <<-\EOF >expect &&
    ++	fatal: '\''foo..bar'\'' is not a valid branch name
    ++	hint: See `man git check-ref-format`
    ++	hint: Disable this message with "git config advice.refSyntax false"
    ++	EOF
     +	test_must_fail git branch foo..bar >actual 2>&1 &&
     +	test_cmp expect actual
     +'
-- 
2.44.0.64.g52b67adbeb2

[PATCH v3 1/5] t3200: improve test style

[Kristoffer Haugsbakk]

Some tests use a preliminary heredoc for `expect` or have setup and
teardown commands before and after, respectively. It is however
preferred to keep all the logic in the test itself. Let’s move these
into the tests.

Also:

• Remove a now-irrelevant comment about test placement and switch back
  to `main` post-test.
• Prefer indented literal heredocs (`-\EOF`) except for a block which
  says that this is intentional
• Move a `git config` command into the test and mark it as `setup` since
  the next test depends on it

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t3200-branch.sh | 115 ++++++++++++++++++++++------------------------
 1 file changed, 56 insertions(+), 59 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..273a57a72d8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -75,13 +75,13 @@ test_expect_success 'git branch HEAD should fail' '
 	test_must_fail git branch HEAD
 '
 
-cat >expect <<EOF
-$HEAD refs/heads/d/e/f@{0}: branch: Created from main
-EOF
 test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
 	test_ref_exists refs/heads/d/e/f &&
+	cat >expect <<-EOF &&
+	$HEAD refs/heads/d/e/f@{0}: branch: Created from main
+	EOF
 	git reflog show --no-abbrev-commit refs/heads/d/e/f >actual &&
 	test_cmp expect actual
 '
@@ -440,10 +440,10 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
-	cat >expect <<\EOF &&
-  a/b/c   bam     foo     l     * main    n       o/p     r
-  abc     bar     j/k     m/m     mb      o/o     q       topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c   bam     foo     l     * main    n       o/p     r
+	  abc     bar     j/k     m/m     mb      o/o     q       topic
+	EOF
 	test_cmp expect actual
 '
 
@@ -453,25 +453,25 @@ test_expect_success 'git branch --column with an extremely long branch name' '
 	test_when_finished "git branch -d $long" &&
 	git branch $long &&
 	COLUMNS=80 git branch --column=column >actual &&
-	cat >expect <<EOF &&
-  a/b/c
-  abc
-  bam
-  bar
-  foo
-  j/k
-  l
-  m/m
-* main
-  mb
-  n
-  o/o
-  o/p
-  q
-  r
-  topic
-  $long
-EOF
+	cat >expect <<-EOF &&
+	  a/b/c
+	  abc
+	  bam
+	  bar
+	  foo
+	  j/k
+	  l
+	  m/m
+	* main
+	  mb
+	  n
+	  o/o
+	  o/p
+	  q
+	  r
+	  topic
+	  $long
+	EOF
 	test_cmp expect actual
 '
 
@@ -481,10 +481,10 @@ test_expect_success 'git branch with column.*' '
 	COLUMNS=80 git branch >actual &&
 	git config --unset column.branch &&
 	git config --unset column.ui &&
-	cat >expect <<\EOF &&
-  a/b/c   bam   foo   l   * main   n     o/p   r
-  abc     bar   j/k   m/m   mb     o/o   q     topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c   bam   foo   l   * main   n     o/p   r
+	  abc     bar   j/k   m/m   mb     o/o   q     topic
+	EOF
 	test_cmp expect actual
 '
 
@@ -496,39 +496,36 @@ test_expect_success 'git branch -v with column.ui ignored' '
 	git config column.ui column &&
 	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
 	git config --unset column.ui &&
-	cat >expect <<\EOF &&
-  a/b/c
-  abc
-  bam
-  bar
-  foo
-  j/k
-  l
-  m/m
-* main
-  mb
-  n
-  o/o
-  o/p
-  q
-  r
-  topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c
+	  abc
+	  bam
+	  bar
+	  foo
+	  j/k
+	  l
+	  m/m
+	* main
+	  mb
+	  n
+	  o/o
+	  o/p
+	  q
+	  r
+	  topic
+	EOF
 	test_cmp expect actual
 '
 
-mv .git/config .git/config-saved
-
 test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
+	test_when_finished mv .git/config-saved .git/config &&
+	mv .git/config .git/config-saved &&
 	git branch -m q q2 &&
 	git branch -m q2 q
 '
 
-mv .git/config-saved .git/config
-
-git config branch.s/s.dummy Hello
-
-test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
+test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
+	git config branch.s/s.dummy Hello &&
 	git branch --create-reflog s/s &&
 	git reflog exists refs/heads/s/s &&
 	git branch --create-reflog s/t &&
@@ -1141,14 +1138,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups
 	test_cmp expect actual
 "
 
-# Keep this test last, as it changes the current branch
-cat >expect <<EOF
-$HEAD refs/heads/g/h/i@{0}: branch: Created from main
-EOF
 test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
+	test_when_finished git checkout main &&
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git checkout -b g/h/i -l main &&
 	test_ref_exists refs/heads/g/h/i &&
+	cat >expect <<-EOF &&
+	$HEAD refs/heads/g/h/i@{0}: branch: Created from main
+	EOF
 	git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
 	test_cmp expect actual
 '
-- 
2.44.0.64.g52b67adbeb2

Re: [PATCH v3 1/5] t3200: improve test style

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Also:
>
> • Remove a now-irrelevant comment about test placement and switch back
>   to `main` post-test.
> • Prefer indented literal heredocs (`-\EOF`) except for a block which
>   says that this is intentional
> • Move a `git config` command into the test and mark it as `setup` since
>   the next test depends on it
>

Especially the change to use "-\EOF" to make them align better
caused too many tests to be touched, but overall the result may have
become much easier to follow.  Good job.

> -mv .git/config .git/config-saved
> -
>  test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
> +	test_when_finished mv .git/config-saved .git/config &&
> +	mv .git/config .git/config-saved &&
>  	git branch -m q q2 &&
>  	git branch -m q2 q
>  '
>  
> -mv .git/config-saved .git/config

The above is a truly valuable clean-up.

But I am not really sure if the paritcular condition is worth
testing in the first place these days.  No configuration file means
we cannot even read the repository format version, and working under
such a condition is quite a bad promise that we would rather not to
having to keep.  But that is an entirely different topic from what
this patch is doing.

> -git config branch.s/s.dummy Hello
> -
> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
> +	git config branch.s/s.dummy Hello &&
>  	git branch --create-reflog s/s &&
>  	git reflog exists refs/heads/s/s &&
>  	git branch --create-reflog s/t &&

I do not know if the change of the title is warranted.  It is doing
its own test, not just setup.  It may be merely donw for the side
effect of making the step unskippable, but still ....

> -# Keep this test last, as it changes the current branch

Yes, removal of this line is really appreciated ;-)

> -cat >expect <<EOF
> -$HEAD refs/heads/g/h/i@{0}: branch: Created from main
> -EOF
>  test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
> +	test_when_finished git checkout main &&
>  	GIT_COMMITTER_DATE="2005-05-26 23:30" \
>  	git checkout -b g/h/i -l main &&
>  	test_ref_exists refs/heads/g/h/i &&
> +	cat >expect <<-EOF &&
> +	$HEAD refs/heads/g/h/i@{0}: branch: Created from main
> +	EOF
>  	git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
>  	test_cmp expect actual
>  '

Thanks.

Re: [PATCH v3 1/5] t3200: improve test style

[Kristoffer Haugsbakk]

On Tue, Mar 5, 2024, at 02:25, Junio C Hamano wrote:
> Especially the change to use "-\EOF" to make them align better
> caused too many tests to be touched, but overall the result may have
> become much easier to follow.  Good job.

I reckon that this can be worth doing now as long as no other topics in
`next` or `seen` happen to touch the same code. What do you think? I can
evict hunks if they happen to overlap with other in-flight topics.

>> -mv .git/config .git/config-saved
>> -
>>  test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
>> +	test_when_finished mv .git/config-saved .git/config &&
>> +	mv .git/config .git/config-saved &&
>>  	git branch -m q q2 &&
>>  	git branch -m q2 q
>>  '
>>
>> -mv .git/config-saved .git/config
>
> The above is a truly valuable clean-up.
>
> But I am not really sure if the paritcular condition is worth
> testing in the first place these days.  No configuration file means
> we cannot even read the repository format version, and working under
> such a condition is quite a bad promise that we would rather not to
> having to keep.  But that is an entirely different topic from what
> this patch is doing.

Okay. I could undo this change and remove the test in its own commit?

>
>> -git config branch.s/s.dummy Hello
>> -
>> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
>> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
>> +	git config branch.s/s.dummy Hello &&
>>  	git branch --create-reflog s/s &&
>>  	git reflog exists refs/heads/s/s &&
>>  	git branch --create-reflog s/t &&
>
> I do not know if the change of the title is warranted.  It is doing
> its own test, not just setup.  It may be merely donw for the side
> effect of making the step unskippable, but still ....

Sure, I’ll remove `(setup)`. The test name suggests that the test
depends on the previous one in any case.

Re: [PATCH v3 1/5] t3200: improve test style

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

>>> -mv .git/config .git/config-saved
>>> -
>>>  test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
>>> +	test_when_finished mv .git/config-saved .git/config &&
>>> +	mv .git/config .git/config-saved &&
>>>  	git branch -m q q2 &&
>>>  	git branch -m q2 q
>>>  '
>>>
>>> -mv .git/config-saved .git/config
>>
>> The above is a truly valuable clean-up.
>>
>> But I am not really sure if the paritcular condition is worth
>> testing in the first place these days.  No configuration file means
>> we cannot even read the repository format version, and working under
>> such a condition is quite a bad promise that we would rather not to
>> having to keep.  But that is an entirely different topic from what
>> this patch is doing.
>
> Okay. I could undo this change and remove the test in its own commit?

No, please keep it.

I think removing it is totally outside the scope of this series.  We
do preliminary clean-up in various areas, so that the last step can
do the advise thing for "git branch".  In the context of the series,
removing this test does not fit anywhere.  It is not a clean-up like
any other preliminary steps.

Thanks.

[PATCH v3 2/5] advice: make all entries stylistically consistent

[Kristoffer Haugsbakk]

1. Use “shown” instead of “advice shown”
   • “advice” is implied and a bit repetitive
2. Use “when” instead of “if”
3. Lead with “Shown when” and end the entry with the effect it has,
   where applicable
4. Use “the user” instead of “a user” or “you”
5. detachedHead: connect clause with a semicolon to make the sentence
   flow better in this new context
6. implicitIdentity: rewrite description in order to lead with *when*
   the advice is shown (see point (3))
7. Prefer the present tense (with the exception of pushNonFFMatching)
8. Use a colon to connect the last clause instead of a comma
9. waitingForEditor: give example of relevance in this new context
10. pushUpdateRejected: exception to the above principles

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Maybe the style that we eventually agree on should be documented outside the
    commit log?

 Documentation/config/advice.txt | 80 ++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..cfca87a6aa2 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -6,23 +6,23 @@ advice.*::
 +
 --
 	addEmbeddedRepo::
-		Advice on what to do when you've accidentally added one
+		Shown when the user accidentally adds one
 		git repo inside of another.
 	addEmptyPathspec::
-		Advice shown if a user runs the add command without providing
+		Shown when the user runs the add command without providing
 		the pathspec parameter.
 	addIgnoredFile::
-		Advice shown if a user attempts to add an ignored file to
+		Shown when the user attempts to add an ignored file to
 		the index.
 	amWorkDir::
-		Advice that shows the location of the patch file when
-		linkgit:git-am[1] fails to apply it.
+		Shown when linkgit:git-am[1] fails to apply a patch
+		file: tell the location of the file.
 	ambiguousFetchRefspec::
-		Advice shown when a fetch refspec for multiple remotes maps to
+		Shown when a fetch refspec for multiple remotes maps to
 		the same remote-tracking branch namespace and causes branch
 		tracking set-up to fail.
 	checkoutAmbiguousRemoteBranchName::
-		Advice shown when the argument to
+		Shown when the argument to
 		linkgit:git-checkout[1] and linkgit:git-switch[1]
 		ambiguously resolves to a
 		remote tracking branch on more than one remote in
@@ -33,31 +33,31 @@ advice.*::
 		to be used by default in some situations where this
 		advice would be printed.
 	commitBeforeMerge::
-		Advice shown when linkgit:git-merge[1] refuses to
+		Shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
 	detachedHead::
-		Advice shown when you used
+		Shown when the user uses
 		linkgit:git-switch[1] or linkgit:git-checkout[1]
-		to move to the detached HEAD state, to instruct how to
+		to move to the detached HEAD state; instruct how to
 		create a local branch after the fact.
 	diverging::
-		Advice shown when a fast-forward is not possible.
+		Shown when a fast-forward is not possible.
 	fetchShowForcedUpdates::
-		Advice shown when linkgit:git-fetch[1] takes a long time
+		Shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
 		that the check is disabled.
 	forceDeleteBranch::
-		Advice shown when a user tries to delete a not fully merged
+		Shown when the user tries to delete a not fully merged
 		branch without the force option set.
 	ignoredHook::
-		Advice shown if a hook is ignored because the hook is not
+		Shown when a hook is ignored because the hook is not
 		set as executable.
 	implicitIdentity::
-		Advice on how to set your identity configuration when
-		your information is guessed from the system username and
-		domain name.
+		Shown when the user's information is guessed from the
+		system username and domain name: tell the user how to
+		set their identity configuration.
 	nestedTag::
-		Advice shown if a user attempts to recursively tag a tag object.
+		Shown when a user attempts to recursively tag a tag object.
 	pushAlreadyExists::
 		Shown when linkgit:git-push[1] rejects an update that
 		does not qualify for fast-forwarding (e.g., a tag.)
@@ -71,12 +71,12 @@ advice.*::
 		object that is not a commit-ish, or make the remote
 		ref point at an object that is not a commit-ish.
 	pushNonFFCurrent::
-		Advice shown when linkgit:git-push[1] fails due to a
+		Shown when linkgit:git-push[1] fails due to a
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
-		Advice shown when you ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. you used ':', or
-		specified a refspec that isn't your current branch) and
+		Shown when the user ran linkgit:git-push[1] and pushed
+		'matching refs' explicitly (i.e. used ':', or
+		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
 		Shown when linkgit:git-push[1] rejects a forced update of
@@ -95,17 +95,17 @@ advice.*::
 		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
 		simultaneously.
 	resetNoRefresh::
-		Advice to consider using the `--no-refresh` option to
-		linkgit:git-reset[1] when the command takes more than 2 seconds
-		to refresh the index after reset.
+		Shown when linkgit:git-reset[1] takes more than 2
+		seconds to refresh the index after reset: tell the user
+		that they can use the `--no-refresh` option.
 	resolveConflict::
-		Advice shown by various commands when conflicts
+		Shown by various commands when conflicts
 		prevent the operation from being performed.
 	rmHints::
-		In case of failure in the output of linkgit:git-rm[1],
-		show directions on how to proceed from the current state.
+		Shown on failure in the output of linkgit:git-rm[1]:
+		give directions on how to proceed from the current state.
 	sequencerInUse::
-		Advice shown when a sequencer command is already in progress.
+		Shown when a sequencer command is already in progress.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
@@ -123,27 +123,27 @@ advice.*::
 		by linkgit:git-switch[1] or
 		linkgit:git-checkout[1] when switching branches.
 	statusUoption::
-		Advise to consider using the `-u` option to linkgit:git-status[1]
-		when the command takes more than 2 seconds to enumerate untracked
-		files.
+		Shown when linkgit:git-status[1] takes more than 2
+		seconds to enumerate untracked files: consider using the
+		`-u` option.
 	submoduleAlternateErrorStrategyDie::
-		Advice shown when a submodule.alternateErrorStrategy option
+		Shown when a submodule.alternateErrorStrategy option
 		configured to "die" causes a fatal error.
 	submodulesNotUpdated::
-		Advice shown when a user runs a submodule command that fails
+		Shown when a user runs a submodule command that fails
 		because `git submodule update --init` was not run.
 	suggestDetachingHead::
-		Advice shown when linkgit:git-switch[1] refuses to detach HEAD
+		Shown when linkgit:git-switch[1] refuses to detach HEAD
 		without the explicit `--detach` option.
 	updateSparsePath::
-		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
+		Shown when either linkgit:git-add[1] or linkgit:git-rm[1]
 		is asked to update index entries outside the current sparse
 		checkout.
 	waitingForEditor::
-		Print a message to the terminal whenever Git is waiting for
-		editor input from the user.
+		Shown when Git is waiting for editor input. Relevant
+		when e.g. the editor is not launched inside the terminal.
 	worktreeAddOrphan::
-		Advice shown when a user tries to create a worktree from an
-		invalid reference, to instruct how to create a new unborn
+		Shown when the user tries to create a worktree from an
+		invalid reference: instruct how to create a new unborn
 		branch instead.
 --
-- 
2.44.0.64.g52b67adbeb2

Re: [PATCH v3 2/5] advice: make all entries stylistically consistent

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> 1. Use “shown” instead of “advice shown”
>    • “advice” is implied and a bit repetitive
> 2. Use “when” instead of “if”
> 3. Lead with “Shown when” and end the entry with the effect it has,
>    where applicable
> 4. Use “the user” instead of “a user” or “you”
> 5. detachedHead: connect clause with a semicolon to make the sentence
>    flow better in this new context
> 6. implicitIdentity: rewrite description in order to lead with *when*
>    the advice is shown (see point (3))
> 7. Prefer the present tense (with the exception of pushNonFFMatching)
> 8. Use a colon to connect the last clause instead of a comma
> 9. waitingForEditor: give example of relevance in this new context
> 10. pushUpdateRejected: exception to the above principles

I'll let others comment on these as general principles.  I do not
immediately see anything objectionable, but I may change my mind
after reading the updated text in the patch.

> Suggested-by: Junio C Hamano <gitster@pobox.com>

I am getting too much credit for this; I merely suggested to use
"when" instead of "if" in the one you are newly adding.

>  	detachedHead::
> -		Advice shown when you used
> +		Shown when the user uses
>  		linkgit:git-switch[1] or linkgit:git-checkout[1]
> -		to move to the detached HEAD state, to instruct how to
> +		to move to the detached HEAD state; instruct how to
>  		create a local branch after the fact.

I agree "Advice shown when" -> "Shown when" is a good change for
brevity, but I do not think the other change is an improvement.

This advice message is shown when the user does X, in order to
instruct the user how to do Y after that.  And "to instruct" is a
common way to say the same thing as "in order to instruct".

>  	implicitIdentity::
> -		Advice on how to set your identity configuration when
> -		your information is guessed from the system username and
> -		domain name.
> +		Shown when the user's information is guessed from the
> +		system username and domain name: tell the user how to
> +		set their identity configuration.

Should that be a colon?  Stopping a half-sentence and connecting to
another half-sentence is usually done with a semicolon (like you did
in the new version of detachedHEAD above).

	Shown when ... and domain name, to tell the user how to set
	their identity configuration.

perhaps?  There may be other similar entries whose updated text uses
colon followed by an imperative sentence, but I didn't look very
carefully.

>  	statusUoption::
> -		Advise to consider using the `-u` option to linkgit:git-status[1]
> -		when the command takes more than 2 seconds to enumerate untracked
> -		files.
> +		Shown when linkgit:git-status[1] takes more than 2
> +		seconds to enumerate untracked files: consider using the
> +		`-u` option.

Earlier ones after a colon (or semicolon in detachedHEAD case), you
gave an order to the advice message (e.g. "hey detachedHead advice,
tell the user how to create a local branch"), but this one is giving
an order to the end user, which feels inconsistent.

I do not have a strong objection against giving an order to the
advice message, as long as it is done consistently.  If we did so,
the part after the colon would start with "instruct the user ..." or
"tell the user ..." and the like, and the gist of what this one
would say would be "shown when it is taking too long: suggest the
user to consider `-u`".

FWIW, my earlier "in order to" took an approach that is different
from either of the two "giving an order" approaches.  I was trying
to make the description explain what the message tries to do and/or
why the message is given (e.g., "shown if it takes too long in order
to suggest users to consider the -u option").

Thanks.

Re: [PATCH v3 2/5] advice: make all entries stylistically consistent

[Kristoffer Haugsbakk]

On Tue, Mar 5, 2024, at 00:52, Junio C Hamano wrote:
>>  	detachedHead::
>> -		Advice shown when you used
>> +		Shown when the user uses
>>  		linkgit:git-switch[1] or linkgit:git-checkout[1]
>> -		to move to the detached HEAD state, to instruct how to
>> +		to move to the detached HEAD state; instruct how to
>>  		create a local branch after the fact.
>
> I agree "Advice shown when" -> "Shown when" is a good change for
> brevity, but I do not think the other change is an improvement.
>
> This advice message is shown when the user does X, in order to
> instruct the user how to do Y after that.  And "to instruct" is a
> common way to say the same thing as "in order to instruct".

Well argued. I’ll go back to the comma.

>>  	implicitIdentity::
>> -		Advice on how to set your identity configuration when
>> -		your information is guessed from the system username and
>> -		domain name.
>> +		Shown when the user's information is guessed from the
>> +		system username and domain name: tell the user how to
>> +		set their identity configuration.
>
> Should that be a colon?  Stopping a half-sentence and connecting to
> another half-sentence is usually done with a semicolon (like you did
> in the new version of detachedHEAD above).
>
> 	Shown when ... and domain name, to tell the user how to set
> 	their identity configuration.
>
> perhaps?  There may be other similar entries whose updated text uses
> colon followed by an imperative sentence, but I didn't look very
> carefully.

I’ll spoil it for you: there are a lot of colons. ;)

Good point. I’ll go over it again and probably use more semicolons
instead.

>>  	statusUoption::
>> -		Advise to consider using the `-u` option to linkgit:git-status[1]
>> -		when the command takes more than 2 seconds to enumerate untracked
>> -		files.
>> +		Shown when linkgit:git-status[1] takes more than 2
>> +		seconds to enumerate untracked files: consider using the
>> +		`-u` option.
>
> Earlier ones after a colon (or semicolon in detachedHEAD case), you
> gave an order to the advice message (e.g. "hey detachedHead advice,
> tell the user how to create a local branch"), but this one is giving
> an order to the end user, which feels inconsistent.
>
> I do not have a strong objection against giving an order to the
> advice message, as long as it is done consistently.  If we did so,
> the part after the colon would start with "instruct the user ..." or
> "tell the user ..." and the like, and the gist of what this one
> would say would be "shown when it is taking too long: suggest the
> user to consider `-u`".

Yeah, I paused for a minute when writing that. I’ll change to “tell” or
something similar.

> FWIW, my earlier "in order to" took an approach that is different
> from either of the two "giving an order" approaches.  I was trying
> to make the description explain what the message tries to do and/or
> why the message is given (e.g., "shown if it takes too long in order
> to suggest users to consider the -u option").
>
> Thanks.

-- 
Kristoffer Haugsbakk

[PATCH v3 3/5] advice: use backticks for code

[Kristoffer Haugsbakk]

Use backticks for quoting code rather than single quotes.

Also replace “the add command” with “`git add`”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/config/advice.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index cfca87a6aa2..df447dd5d14 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -2,14 +2,14 @@ advice.*::
 	These variables control various optional help messages designed to
 	aid new users.  When left unconfigured, Git will give the message
 	alongside instructions on how to squelch it.  You can tell Git
-	that you do not need the help message by setting these to 'false':
+	that you do not need the help message by setting these to `false`:
 +
 --
 	addEmbeddedRepo::
 		Shown when the user accidentally adds one
 		git repo inside of another.
 	addEmptyPathspec::
-		Shown when the user runs the add command without providing
+		Shown when the user runs `git add` without providing
 		the pathspec parameter.
 	addIgnoredFile::
 		Shown when the user attempts to add an ignored file to
@@ -75,7 +75,7 @@ advice.*::
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
 		Shown when the user ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. used ':', or
+		'matching refs' explicitly (i.e. used `:`, or
 		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
@@ -90,9 +90,9 @@ advice.*::
 		refs/heads/* or refs/tags/* based on the type of the
 		source object.
 	pushUpdateRejected::
-		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
-		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+		Set this variable to `false` if you want to disable
+		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
+		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
 		simultaneously.
 	resetNoRefresh::
 		Shown when linkgit:git-reset[1] takes more than 2
-- 
2.44.0.64.g52b67adbeb2

Re: [PATCH v3 3/5] advice: use backticks for code

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Use backticks for quoting code rather than single quotes.

Good.  Technically it does not have to be "code", but rather what
the user would literally type from their keyboard verbatim, but
"quoting code" is so concise way to describe, it probably is good
enough hint for future developers who will find this commit via "git
blame" and read "git show" to read this explanation.

> Also replace “the add command” with “`git add`”.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  Documentation/config/advice.txt | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index cfca87a6aa2..df447dd5d14 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -2,14 +2,14 @@ advice.*::
>  	These variables control various optional help messages designed to
>  	aid new users.  When left unconfigured, Git will give the message
>  	alongside instructions on how to squelch it.  You can tell Git
> -	that you do not need the help message by setting these to 'false':
> +	that you do not need the help message by setting these to `false`:
>  +
>  --
>  	addEmbeddedRepo::
>  		Shown when the user accidentally adds one
>  		git repo inside of another.
>  	addEmptyPathspec::
> -		Shown when the user runs the add command without providing
> +		Shown when the user runs `git add` without providing
>  		the pathspec parameter.
>  	addIgnoredFile::
>  		Shown when the user attempts to add an ignored file to
> @@ -75,7 +75,7 @@ advice.*::
>  		non-fast-forward update to the current branch.
>  	pushNonFFMatching::
>  		Shown when the user ran linkgit:git-push[1] and pushed
> -		'matching refs' explicitly (i.e. used ':', or
> +		'matching refs' explicitly (i.e. used `:`, or
>  		specified a refspec that isn't the current branch) and
>  		it resulted in a non-fast-forward error.
>  	pushRefNeedsUpdate::
> @@ -90,9 +90,9 @@ advice.*::
>  		refs/heads/* or refs/tags/* based on the type of the
>  		source object.
>  	pushUpdateRejected::
> -		Set this variable to 'false' if you want to disable
> -		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
> -		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
> +		Set this variable to `false` if you want to disable
> +		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
> +		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
>  		simultaneously.
>  	resetNoRefresh::
>  		Shown when linkgit:git-reset[1] takes more than 2

Re: [PATCH v3 3/5] advice: use backticks for code

[Kristoffer Haugsbakk]

On Tue, Mar 5, 2024, at 00:54, Junio C Hamano wrote:
>> Use backticks for quoting code rather than single quotes.
>
> Good.  Technically it does not have to be "code", but rather what
> the user would literally type from their keyboard verbatim, but
> "quoting code" is so concise way to describe, it probably is good
> enough hint for future developers who will find this commit via "git
> blame" and read "git show" to read this explanation.

I agree. Either works fine but “verbatim” is a more general term. I’ll
use that.

[PATCH v3 4/5] advice: use double quotes for regular quoting

[Kristoffer Haugsbakk]

Use double quotes like we use for “die” in this document.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/config/advice.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index df447dd5d14..c5d3d6790a5 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -75,7 +75,7 @@ advice.*::
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
 		Shown when the user ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. used `:`, or
+		"matching refs" explicitly (i.e. used `:`, or
 		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
-- 
2.44.0.64.g52b67adbeb2

[PATCH v3 5/5] branch: advise about ref syntax rules

[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 user might know that there are some limitations based on the *loose
ref* format (filenames), but there are also further rules for
easier integration with shell-based tools, pathname expansion, and
playing well with reference name expressions.

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. Also make this advise configurable since you might not want to be
reminded every time you make a little typo.

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

Notes (series):
    v3:
    • Tweak advice doc for the new entry
    • Better test style
    v2:
    • Make the advise optional via configuration
    • Propagate error properly with `die_message(…)` instead of `exit(1)`
    • Flesh out commit message a bit

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        |  8 ++++++--
 builtin/branch.c                |  8 ++++++--
 t/t3200-branch.sh               | 10 ++++++++++
 6 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c5d3d6790a5..06a3a3cc9b5 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -94,6 +94,9 @@ advice.*::
 		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
 		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
 		simultaneously.
+	refSyntax::
+		Shown when the user provides an illegal ref name: point
+		towards the ref syntax documentation.
 	resetNoRefresh::
 		Shown when linkgit:git-reset[1] takes more than 2
 		seconds to refresh the index after reset: tell the user
diff --git a/advice.c b/advice.c
index 6e9098ff089..550c2968908 100644
--- a/advice.c
+++ b/advice.c
@@ -68,6 +68,7 @@ static struct {
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
 	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_REF_SYNTAX]				= { "refSyntax" },
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
 	[ADVICE_RM_HINTS]				= { "rmHints" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..d15fe2351ab 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
 	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+	ADVICE_REF_SYNTAX,
 	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
diff --git a/branch.c b/branch.c
index 6719a181bd1..621019fcf4b 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,12 @@ 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)) {
+		int code = die_message(_("'%s' is not a valid branch name"), name);
+		advise_if_enabled(ADVICE_REF_SYNTAX,
+				  _("See `man git check-ref-format`"));
+		exit(code);
+	}
 
 	return ref_exists(ref->buf);
 }
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..1c122ee8a7b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,12 @@ 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 {
+			int code = die_message(_("invalid branch name: '%s'"), oldname);
+			advise_if_enabled(ADVICE_REF_SYNTAX,
+					  _("See `man git check-ref-format`"));
+			exit(code);
+		}
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 273a57a72d8..30a97e3776e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1722,4 +1722,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
 	test_cmp_config "" --default "" branch.foo5.merge
 '
 
+test_expect_success 'errors if given a bad branch name' '
+	cat <<-\EOF >expect &&
+	fatal: '\''foo..bar'\'' is not a valid branch name
+	hint: See `man git check-ref-format`
+	hint: Disable this message with "git config advice.refSyntax false"
+	EOF
+	test_must_fail git branch foo..bar >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 0/5] advise about ref syntax rules

[Kristoffer Haugsbakk]

Point the user towards the ref/branch name syntax rules if they give an
invalid name.

Also make some spatially-appropriate improvements:

• Test style
• `advice.txt`

§ git-replace(1)

(see cover letter for v2)

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

§ CC

For changes to `advice.txt`:

Cc: Elijah Newren <newren@gmail.com>
Cc: Jean-Noël Avila <avila.jn@gmail.com>

§ Changes in v4

Mostly about the style rewrite in `advice.txt`.

• Patch 1:
  • Drop `(setup)` change
  • Drop superflouos bullet point
  • Don’t use period to end bullet point
• Patch 2:
  • Drop trailer since this took on a life of its own
  • Drop uses of colons and semicolons in favor of a “to <verb>”
    clause (mostly “to tell”)
  • Simplify some of the “effect clauses” by using “to tell” instead of
    verbs like “instruct”
• Patch 3:
  • Also quote ref globs
• Patch 5:
  • Update refSyntax entry for consistency with the rest of the entries

Kristoffer Haugsbakk (5):
  t3200: improve test style
  advice: make all entries stylistically consistent
  advice: use backticks for verbatim
  advice: use double quotes for regular quoting
  branch: advise about ref syntax rules

 Documentation/config/advice.txt |  95 ++++++++++++------------
 advice.c                        |   1 +
 advice.h                        |   1 +
 branch.c                        |   8 ++-
 builtin/branch.c                |   8 ++-
 t/t3200-branch.sh               | 123 +++++++++++++++++---------------
 6 files changed, 128 insertions(+), 108 deletions(-)

Range-diff against v3:
1:  e6a2628ce57 ! 1:  ad101c72a60 t3200: improve test style
    @@ Commit message
         Also:
     
         • Remove a now-irrelevant comment about test placement and switch back
    -      to `main` post-test.
    +      to `main` post-test
         • Prefer indented literal heredocs (`-\EOF`) except for a block which
           says that this is intentional
    -    • Move a `git config` command into the test and mark it as `setup` since
    -      the next test depends on it
     
         Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +
    + ## Notes (series) ##
    +    v4:
    +    • Drop `(setup)` change
    +    • Drop superflouos bullet point
    +    • Don’t use period to end bullet point
    +
      ## t/t3200-branch.sh ##
     @@ t/t3200-branch.sh: test_expect_success 'git branch HEAD should fail' '
      	test_must_fail git branch HEAD
    @@ t/t3200-branch.sh: test_expect_success 'git branch -v with column.ui ignored' '
     -
     -git config branch.s/s.dummy Hello
     -
    --test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
    -+test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
    + test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
     +	git config branch.s/s.dummy Hello &&
      	git branch --create-reflog s/s &&
      	git reflog exists refs/heads/s/s &&
2:  d48b4719c27 ! 2:  7017ff3fff7 advice: make all entries stylistically consistent
    @@ Metadata
      ## Commit message ##
         advice: make all entries stylistically consistent
     
    +    In general, rewrite entries to the following form:
    +
    +    1. Clause or sentence describing when the advice is shown
    +    2. Optional “to <verb>” clause which says what the advice is
    +       about (e.g. for resetNoRefresh: tell the user that they can use
    +       `--no-refresh`)
    +
    +    Concretely:
    +
         1. Use “shown” instead of “advice shown”
            • “advice” is implied and a bit repetitive
         2. Use “when” instead of “if”
         3. Lead with “Shown when” and end the entry with the effect it has,
            where applicable
         4. Use “the user” instead of “a user” or “you”
    -    5. detachedHead: connect clause with a semicolon to make the sentence
    -       flow better in this new context
    -    6. implicitIdentity: rewrite description in order to lead with *when*
    +    5. implicitIdentity: rewrite description in order to lead with *when*
            the advice is shown (see point (3))
    -    7. Prefer the present tense (with the exception of pushNonFFMatching)
    -    8. Use a colon to connect the last clause instead of a comma
    -    9. waitingForEditor: give example of relevance in this new context
    -    10. pushUpdateRejected: exception to the above principles
    +    6. Prefer the present tense (with the exception of pushNonFFMatching)
    +    7. waitingForEditor: give example of relevance in this new context
    +    8. pushUpdateRejected: exception to the above principles
     
    -    Suggested-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
     
      ## Notes (series) ##
    -    Maybe the style that we eventually agree on should be documented outside the
    -    commit log?
    +    v4:
    +    • Drop trailer since this took on a life of its own
    +    • Drop uses of colons and semicolons in favor of a “to <verb>”
    +      clause (mostly “to tell”)
    +    • Simplify some of the “effect clauses” by using “to tell” instead of
    +      verbs like “instruct”
    +    v3:
    +    • Comment: Maybe the style that we eventually agree on should be
    +      documented outside the commit log?
     
      ## Documentation/config/advice.txt ##
     @@ Documentation/config/advice.txt: advice.*::
    @@ Documentation/config/advice.txt: advice.*::
     -		Advice that shows the location of the patch file when
     -		linkgit:git-am[1] fails to apply it.
     +		Shown when linkgit:git-am[1] fails to apply a patch
    -+		file: tell the location of the file.
    ++		file, to tell the user the location of the file.
      	ambiguousFetchRefspec::
     -		Advice shown when a fetch refspec for multiple remotes maps to
     +		Shown when a fetch refspec for multiple remotes maps to
    @@ Documentation/config/advice.txt: advice.*::
     +		Shown when the user uses
      		linkgit:git-switch[1] or linkgit:git-checkout[1]
     -		to move to the detached HEAD state, to instruct how to
    -+		to move to the detached HEAD state; instruct how to
    - 		create a local branch after the fact.
    +-		create a local branch after the fact.
    ++		to move to the detached HEAD state, to tell the user how
    ++		to create a local branch after the fact.
      	diverging::
     -		Advice shown when a fast-forward is not possible.
     +		Shown when a fast-forward is not possible.
    @@ Documentation/config/advice.txt: advice.*::
     -		your information is guessed from the system username and
     -		domain name.
     +		Shown when the user's information is guessed from the
    -+		system username and domain name: tell the user how to
    ++		system username and domain name, to tell the user how to
     +		set their identity configuration.
      	nestedTag::
     -		Advice shown if a user attempts to recursively tag a tag object.
    @@ Documentation/config/advice.txt: advice.*::
     -		linkgit:git-reset[1] when the command takes more than 2 seconds
     -		to refresh the index after reset.
     +		Shown when linkgit:git-reset[1] takes more than 2
    -+		seconds to refresh the index after reset: tell the user
    ++		seconds to refresh the index after reset, to tell the user
     +		that they can use the `--no-refresh` option.
      	resolveConflict::
     -		Advice shown by various commands when conflicts
    @@ Documentation/config/advice.txt: advice.*::
      	rmHints::
     -		In case of failure in the output of linkgit:git-rm[1],
     -		show directions on how to proceed from the current state.
    -+		Shown on failure in the output of linkgit:git-rm[1]:
    ++		Shown on failure in the output of linkgit:git-rm[1], to
     +		give directions on how to proceed from the current state.
      	sequencerInUse::
     -		Advice shown when a sequencer command is already in progress.
    @@ Documentation/config/advice.txt: advice.*::
     -		when the command takes more than 2 seconds to enumerate untracked
     -		files.
     +		Shown when linkgit:git-status[1] takes more than 2
    -+		seconds to enumerate untracked files: consider using the
    -+		`-u` option.
    ++		seconds to enumerate untracked files, to tell the user that
    ++		they can use the `-u` option.
      	submoduleAlternateErrorStrategyDie::
     -		Advice shown when a submodule.alternateErrorStrategy option
     +		Shown when a submodule.alternateErrorStrategy option
    @@ Documentation/config/advice.txt: advice.*::
     -		Advice shown when a user tries to create a worktree from an
     -		invalid reference, to instruct how to create a new unborn
     +		Shown when the user tries to create a worktree from an
    -+		invalid reference: instruct how to create a new unborn
    ++		invalid reference, to tell the user how to create a new unborn
      		branch instead.
      --
3:  30d662a04c7 ! 3:  df9b872afd1 advice: use backticks for code
    @@ Metadata
     Author: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
      ## Commit message ##
    -    advice: use backticks for code
    +    advice: use backticks for verbatim
     
    -    Use backticks for quoting code rather than single quotes.
    +    Use backticks for inline-verbatim rather than single quotes. Also quote
    +    the unquoted ref globs.
     
         Also replace “the add command” with “`git add`”.
     
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +
    + ## Notes (series) ##
    +    v4:
    +    • Also quote ref globs
    +
      ## Documentation/config/advice.txt ##
     @@ Documentation/config/advice.txt: advice.*::
      	These variables control various optional help messages designed to
    @@ Documentation/config/advice.txt: advice.*::
      		it resulted in a non-fast-forward error.
      	pushRefNeedsUpdate::
     @@ Documentation/config/advice.txt: advice.*::
    - 		refs/heads/* or refs/tags/* based on the type of the
    + 		guess based on the source and destination refs what
    + 		remote ref namespace the source belongs in, but where
    + 		we can still suggest that the user push to either
    +-		refs/heads/* or refs/tags/* based on the type of the
    ++		`refs/heads/*` or `refs/tags/*` based on the type of the
      		source object.
      	pushUpdateRejected::
     -		Set this variable to 'false' if you want to disable
4:  3028713357f = 4:  15594b2a3a8 advice: use double quotes for regular quoting
5:  402b7937951 ! 5:  97b53c04894 branch: advise about ref syntax rules
    @@ Commit message
     
     
      ## Notes (series) ##
    +    v4:
    +    • Update refSyntax entry for consistency with the rest of the entries
         v3:
         • Tweak advice doc for the new entry
         • Better test style
    @@ Documentation/config/advice.txt: advice.*::
      		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
      		simultaneously.
     +	refSyntax::
    -+		Shown when the user provides an illegal ref name: point
    -+		towards the ref syntax documentation.
    ++		Shown when the user provides an illegal ref name, to
    ++		tell the user about the ref syntax documentation.
      	resetNoRefresh::
      		Shown when linkgit:git-reset[1] takes more than 2
    - 		seconds to refresh the index after reset: tell the user
    + 		seconds to refresh the index after reset, to tell the user
     
      ## advice.c ##
     @@ advice.c: static struct {
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 1/5] t3200: improve test style

[Kristoffer Haugsbakk]

Some tests use a preliminary heredoc for `expect` or have setup and
teardown commands before and after, respectively. It is however
preferred to keep all the logic in the test itself. Let’s move these
into the tests.

Also:

• Remove a now-irrelevant comment about test placement and switch back
  to `main` post-test
• Prefer indented literal heredocs (`-\EOF`) except for a block which
  says that this is intentional

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v4:
    • Drop `(setup)` change
    • Drop superflouos bullet point
    • Don’t use period to end bullet point

 t/t3200-branch.sh | 113 ++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 58 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..060b27097e8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -75,13 +75,13 @@ test_expect_success 'git branch HEAD should fail' '
 	test_must_fail git branch HEAD
 '
 
-cat >expect <<EOF
-$HEAD refs/heads/d/e/f@{0}: branch: Created from main
-EOF
 test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
 	test_ref_exists refs/heads/d/e/f &&
+	cat >expect <<-EOF &&
+	$HEAD refs/heads/d/e/f@{0}: branch: Created from main
+	EOF
 	git reflog show --no-abbrev-commit refs/heads/d/e/f >actual &&
 	test_cmp expect actual
 '
@@ -440,10 +440,10 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
-	cat >expect <<\EOF &&
-  a/b/c   bam     foo     l     * main    n       o/p     r
-  abc     bar     j/k     m/m     mb      o/o     q       topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c   bam     foo     l     * main    n       o/p     r
+	  abc     bar     j/k     m/m     mb      o/o     q       topic
+	EOF
 	test_cmp expect actual
 '
 
@@ -453,25 +453,25 @@ test_expect_success 'git branch --column with an extremely long branch name' '
 	test_when_finished "git branch -d $long" &&
 	git branch $long &&
 	COLUMNS=80 git branch --column=column >actual &&
-	cat >expect <<EOF &&
-  a/b/c
-  abc
-  bam
-  bar
-  foo
-  j/k
-  l
-  m/m
-* main
-  mb
-  n
-  o/o
-  o/p
-  q
-  r
-  topic
-  $long
-EOF
+	cat >expect <<-EOF &&
+	  a/b/c
+	  abc
+	  bam
+	  bar
+	  foo
+	  j/k
+	  l
+	  m/m
+	* main
+	  mb
+	  n
+	  o/o
+	  o/p
+	  q
+	  r
+	  topic
+	  $long
+	EOF
 	test_cmp expect actual
 '
 
@@ -481,10 +481,10 @@ test_expect_success 'git branch with column.*' '
 	COLUMNS=80 git branch >actual &&
 	git config --unset column.branch &&
 	git config --unset column.ui &&
-	cat >expect <<\EOF &&
-  a/b/c   bam   foo   l   * main   n     o/p   r
-  abc     bar   j/k   m/m   mb     o/o   q     topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c   bam   foo   l   * main   n     o/p   r
+	  abc     bar   j/k   m/m   mb     o/o   q     topic
+	EOF
 	test_cmp expect actual
 '
 
@@ -496,39 +496,36 @@ test_expect_success 'git branch -v with column.ui ignored' '
 	git config column.ui column &&
 	COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
 	git config --unset column.ui &&
-	cat >expect <<\EOF &&
-  a/b/c
-  abc
-  bam
-  bar
-  foo
-  j/k
-  l
-  m/m
-* main
-  mb
-  n
-  o/o
-  o/p
-  q
-  r
-  topic
-EOF
+	cat >expect <<-\EOF &&
+	  a/b/c
+	  abc
+	  bam
+	  bar
+	  foo
+	  j/k
+	  l
+	  m/m
+	* main
+	  mb
+	  n
+	  o/o
+	  o/p
+	  q
+	  r
+	  topic
+	EOF
 	test_cmp expect actual
 '
 
-mv .git/config .git/config-saved
-
 test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
+	test_when_finished mv .git/config-saved .git/config &&
+	mv .git/config .git/config-saved &&
 	git branch -m q q2 &&
 	git branch -m q2 q
 '
 
-mv .git/config-saved .git/config
-
-git config branch.s/s.dummy Hello
-
 test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
+	git config branch.s/s.dummy Hello &&
 	git branch --create-reflog s/s &&
 	git reflog exists refs/heads/s/s &&
 	git branch --create-reflog s/t &&
@@ -1141,14 +1138,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups
 	test_cmp expect actual
 "
 
-# Keep this test last, as it changes the current branch
-cat >expect <<EOF
-$HEAD refs/heads/g/h/i@{0}: branch: Created from main
-EOF
 test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
+	test_when_finished git checkout main &&
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git checkout -b g/h/i -l main &&
 	test_ref_exists refs/heads/g/h/i &&
+	cat >expect <<-EOF &&
+	$HEAD refs/heads/g/h/i@{0}: branch: Created from main
+	EOF
 	git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
 	test_cmp expect actual
 '
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 2/5] advice: make all entries stylistically consistent

[Kristoffer Haugsbakk]

In general, rewrite entries to the following form:

1. Clause or sentence describing when the advice is shown
2. Optional “to <verb>” clause which says what the advice is
   about (e.g. for resetNoRefresh: tell the user that they can use
   `--no-refresh`)

Concretely:

1. Use “shown” instead of “advice shown”
   • “advice” is implied and a bit repetitive
2. Use “when” instead of “if”
3. Lead with “Shown when” and end the entry with the effect it has,
   where applicable
4. Use “the user” instead of “a user” or “you”
5. implicitIdentity: rewrite description in order to lead with *when*
   the advice is shown (see point (3))
6. Prefer the present tense (with the exception of pushNonFFMatching)
7. waitingForEditor: give example of relevance in this new context
8. pushUpdateRejected: exception to the above principles

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

Notes (series):
    v4:
    • Drop trailer since this took on a life of its own
    • Drop uses of colons and semicolons in favor of a “to <verb>”
      clause (mostly “to tell”)
    • Simplify some of the “effect clauses” by using “to tell” instead of
      verbs like “instruct”
    v3:
    • Comment: Maybe the style that we eventually agree on should be
      documented outside the commit log?

 Documentation/config/advice.txt | 82 ++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..72cd9f9e9d9 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -6,23 +6,23 @@ advice.*::
 +
 --
 	addEmbeddedRepo::
-		Advice on what to do when you've accidentally added one
+		Shown when the user accidentally adds one
 		git repo inside of another.
 	addEmptyPathspec::
-		Advice shown if a user runs the add command without providing
+		Shown when the user runs the add command without providing
 		the pathspec parameter.
 	addIgnoredFile::
-		Advice shown if a user attempts to add an ignored file to
+		Shown when the user attempts to add an ignored file to
 		the index.
 	amWorkDir::
-		Advice that shows the location of the patch file when
-		linkgit:git-am[1] fails to apply it.
+		Shown when linkgit:git-am[1] fails to apply a patch
+		file, to tell the user the location of the file.
 	ambiguousFetchRefspec::
-		Advice shown when a fetch refspec for multiple remotes maps to
+		Shown when a fetch refspec for multiple remotes maps to
 		the same remote-tracking branch namespace and causes branch
 		tracking set-up to fail.
 	checkoutAmbiguousRemoteBranchName::
-		Advice shown when the argument to
+		Shown when the argument to
 		linkgit:git-checkout[1] and linkgit:git-switch[1]
 		ambiguously resolves to a
 		remote tracking branch on more than one remote in
@@ -33,31 +33,31 @@ advice.*::
 		to be used by default in some situations where this
 		advice would be printed.
 	commitBeforeMerge::
-		Advice shown when linkgit:git-merge[1] refuses to
+		Shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
 	detachedHead::
-		Advice shown when you used
+		Shown when the user uses
 		linkgit:git-switch[1] or linkgit:git-checkout[1]
-		to move to the detached HEAD state, to instruct how to
-		create a local branch after the fact.
+		to move to the detached HEAD state, to tell the user how
+		to create a local branch after the fact.
 	diverging::
-		Advice shown when a fast-forward is not possible.
+		Shown when a fast-forward is not possible.
 	fetchShowForcedUpdates::
-		Advice shown when linkgit:git-fetch[1] takes a long time
+		Shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
 		that the check is disabled.
 	forceDeleteBranch::
-		Advice shown when a user tries to delete a not fully merged
+		Shown when the user tries to delete a not fully merged
 		branch without the force option set.
 	ignoredHook::
-		Advice shown if a hook is ignored because the hook is not
+		Shown when a hook is ignored because the hook is not
 		set as executable.
 	implicitIdentity::
-		Advice on how to set your identity configuration when
-		your information is guessed from the system username and
-		domain name.
+		Shown when the user's information is guessed from the
+		system username and domain name, to tell the user how to
+		set their identity configuration.
 	nestedTag::
-		Advice shown if a user attempts to recursively tag a tag object.
+		Shown when a user attempts to recursively tag a tag object.
 	pushAlreadyExists::
 		Shown when linkgit:git-push[1] rejects an update that
 		does not qualify for fast-forwarding (e.g., a tag.)
@@ -71,12 +71,12 @@ advice.*::
 		object that is not a commit-ish, or make the remote
 		ref point at an object that is not a commit-ish.
 	pushNonFFCurrent::
-		Advice shown when linkgit:git-push[1] fails due to a
+		Shown when linkgit:git-push[1] fails due to a
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
-		Advice shown when you ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. you used ':', or
-		specified a refspec that isn't your current branch) and
+		Shown when the user ran linkgit:git-push[1] and pushed
+		'matching refs' explicitly (i.e. used ':', or
+		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
 		Shown when linkgit:git-push[1] rejects a forced update of
@@ -95,17 +95,17 @@ advice.*::
 		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
 		simultaneously.
 	resetNoRefresh::
-		Advice to consider using the `--no-refresh` option to
-		linkgit:git-reset[1] when the command takes more than 2 seconds
-		to refresh the index after reset.
+		Shown when linkgit:git-reset[1] takes more than 2
+		seconds to refresh the index after reset, to tell the user
+		that they can use the `--no-refresh` option.
 	resolveConflict::
-		Advice shown by various commands when conflicts
+		Shown by various commands when conflicts
 		prevent the operation from being performed.
 	rmHints::
-		In case of failure in the output of linkgit:git-rm[1],
-		show directions on how to proceed from the current state.
+		Shown on failure in the output of linkgit:git-rm[1], to
+		give directions on how to proceed from the current state.
 	sequencerInUse::
-		Advice shown when a sequencer command is already in progress.
+		Shown when a sequencer command is already in progress.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
@@ -123,27 +123,27 @@ advice.*::
 		by linkgit:git-switch[1] or
 		linkgit:git-checkout[1] when switching branches.
 	statusUoption::
-		Advise to consider using the `-u` option to linkgit:git-status[1]
-		when the command takes more than 2 seconds to enumerate untracked
-		files.
+		Shown when linkgit:git-status[1] takes more than 2
+		seconds to enumerate untracked files, to tell the user that
+		they can use the `-u` option.
 	submoduleAlternateErrorStrategyDie::
-		Advice shown when a submodule.alternateErrorStrategy option
+		Shown when a submodule.alternateErrorStrategy option
 		configured to "die" causes a fatal error.
 	submodulesNotUpdated::
-		Advice shown when a user runs a submodule command that fails
+		Shown when a user runs a submodule command that fails
 		because `git submodule update --init` was not run.
 	suggestDetachingHead::
-		Advice shown when linkgit:git-switch[1] refuses to detach HEAD
+		Shown when linkgit:git-switch[1] refuses to detach HEAD
 		without the explicit `--detach` option.
 	updateSparsePath::
-		Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
+		Shown when either linkgit:git-add[1] or linkgit:git-rm[1]
 		is asked to update index entries outside the current sparse
 		checkout.
 	waitingForEditor::
-		Print a message to the terminal whenever Git is waiting for
-		editor input from the user.
+		Shown when Git is waiting for editor input. Relevant
+		when e.g. the editor is not launched inside the terminal.
 	worktreeAddOrphan::
-		Advice shown when a user tries to create a worktree from an
-		invalid reference, to instruct how to create a new unborn
+		Shown when the user tries to create a worktree from an
+		invalid reference, to tell the user how to create a new unborn
 		branch instead.
 --
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 3/5] advice: use backticks for verbatim

[Kristoffer Haugsbakk]

Use backticks for inline-verbatim rather than single quotes. Also quote
the unquoted ref globs.

Also replace “the add command” with “`git add`”.

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

Notes (series):
    v4:
    • Also quote ref globs

 Documentation/config/advice.txt | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 72cd9f9e9d9..c8d6c625f2a 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -2,14 +2,14 @@ advice.*::
 	These variables control various optional help messages designed to
 	aid new users.  When left unconfigured, Git will give the message
 	alongside instructions on how to squelch it.  You can tell Git
-	that you do not need the help message by setting these to 'false':
+	that you do not need the help message by setting these to `false`:
 +
 --
 	addEmbeddedRepo::
 		Shown when the user accidentally adds one
 		git repo inside of another.
 	addEmptyPathspec::
-		Shown when the user runs the add command without providing
+		Shown when the user runs `git add` without providing
 		the pathspec parameter.
 	addIgnoredFile::
 		Shown when the user attempts to add an ignored file to
@@ -75,7 +75,7 @@ advice.*::
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
 		Shown when the user ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. used ':', or
+		'matching refs' explicitly (i.e. used `:`, or
 		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
@@ -87,12 +87,12 @@ advice.*::
 		guess based on the source and destination refs what
 		remote ref namespace the source belongs in, but where
 		we can still suggest that the user push to either
-		refs/heads/* or refs/tags/* based on the type of the
+		`refs/heads/*` or `refs/tags/*` based on the type of the
 		source object.
 	pushUpdateRejected::
-		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
-		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+		Set this variable to `false` if you want to disable
+		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
+		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
 		simultaneously.
 	resetNoRefresh::
 		Shown when linkgit:git-reset[1] takes more than 2
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 4/5] advice: use double quotes for regular quoting

[Kristoffer Haugsbakk]

Use double quotes like we use for “die” in this document.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 Documentation/config/advice.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c8d6c625f2a..dd52041bc94 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -75,7 +75,7 @@ advice.*::
 		non-fast-forward update to the current branch.
 	pushNonFFMatching::
 		Shown when the user ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. used `:`, or
+		"matching refs" explicitly (i.e. used `:`, or
 		specified a refspec that isn't the current branch) and
 		it resulted in a non-fast-forward error.
 	pushRefNeedsUpdate::
-- 
2.44.0.64.g52b67adbeb2

[PATCH v4 5/5] branch: advise about ref syntax rules

[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 user might know that there are some limitations based on the *loose
ref* format (filenames), but there are also further rules for
easier integration with shell-based tools, pathname expansion, and
playing well with reference name expressions.

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. Also make this advise configurable since you might not want to be
reminded every time you make a little typo.

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

Notes (series):
    v4:
    • Update refSyntax entry for consistency with the rest of the entries
    v3:
    • Tweak advice doc for the new entry
    • Better test style
    v2:
    • Make the advise optional via configuration
    • Propagate error properly with `die_message(…)` instead of `exit(1)`
    • Flesh out commit message a bit

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 branch.c                        |  8 ++++++--
 builtin/branch.c                |  8 ++++++--
 t/t3200-branch.sh               | 10 ++++++++++
 6 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index dd52041bc94..06c754899c5 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -94,6 +94,9 @@ advice.*::
 		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
 		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
 		simultaneously.
+	refSyntax::
+		Shown when the user provides an illegal ref name, to
+		tell the user about the ref syntax documentation.
 	resetNoRefresh::
 		Shown when linkgit:git-reset[1] takes more than 2
 		seconds to refresh the index after reset, to tell the user
diff --git a/advice.c b/advice.c
index 6e9098ff089..550c2968908 100644
--- a/advice.c
+++ b/advice.c
@@ -68,6 +68,7 @@ static struct {
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
 	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_REF_SYNTAX]				= { "refSyntax" },
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
 	[ADVICE_RM_HINTS]				= { "rmHints" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..d15fe2351ab 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
 	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+	ADVICE_REF_SYNTAX,
 	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
diff --git a/branch.c b/branch.c
index 6719a181bd1..621019fcf4b 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,12 @@ 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)) {
+		int code = die_message(_("'%s' is not a valid branch name"), name);
+		advise_if_enabled(ADVICE_REF_SYNTAX,
+				  _("See `man git check-ref-format`"));
+		exit(code);
+	}
 
 	return ref_exists(ref->buf);
 }
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..1c122ee8a7b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,12 @@ 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 {
+			int code = die_message(_("invalid branch name: '%s'"), oldname);
+			advise_if_enabled(ADVICE_REF_SYNTAX,
+					  _("See `man git check-ref-format`"));
+			exit(code);
+		}
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 060b27097e8..dd7525d1b8c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1722,4 +1722,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
 	test_cmp_config "" --default "" branch.foo5.merge
 '
 
+test_expect_success 'errors if given a bad branch name' '
+	cat <<-\EOF >expect &&
+	fatal: '\''foo..bar'\'' is not a valid branch name
+	hint: See `man git check-ref-format`
+	hint: Disable this message with "git config advice.refSyntax false"
+	EOF
+	test_must_fail git branch foo..bar >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0.64.g52b67adbeb2

Re: [PATCH v2 0/1] advise about ref syntax rules

[Kristoffer Haugsbakk]

I forgot the range-diff. But turns out it’s empty.

-- 
Kristoffer Haugsbakk