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

[Junio C Hamano <gitster@pobox.com>]

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
 '