git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 3, 2024, 22:42 UTC
Message-ID
<xmqqil23uebw.fsf@gitster.g>
In-Reply-To
<4ad5d4190649dcb5f26c73a6f15ab731891b9dfd.1709491818.git.code@khaugsbakk.name>
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

Show 12 quoted lines
> 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
Show 24 quoted lines
> 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.
Show 16 quoted lines
> 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.
Show 17 quoted lines
> 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.
Show 18 quoted lines
> 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
 '
Previous: Kristoffer HaugsbakkNext: Kristoffer Haugsbakk
Message 7 of 28 in “branch: advise about ref syntax rules”
  1. branch: advise about ref syntax rulesKristoffer Haugsbakk, Mar 1, 2024
  2. Junio C HamanoMar 1, 2024
  3. Kristoffer HaugsbakkMar 1, 2024
  4. Junio C HamanoMar 1, 2024
  5. 0/1 advise about ref syntax rulesKristoffer Haugsbakk, Mar 3, 2024
  6. 1/1 branch: advise about ref syntax rulesKristoffer Haugsbakk, Mar 3, 2024
  7. Junio C HamanoMar 3, 2024
  8. Kristoffer HaugsbakkMar 3, 2024
  9. 0/5 advise about ref syntax rulesKristoffer Haugsbakk, Mar 4, 2024
  10. 1/5 t3200: improve test styleKristoffer Haugsbakk, Mar 4, 2024
  11. Junio C HamanoMar 5, 2024
  12. Kristoffer HaugsbakkMar 5, 2024
  13. Junio C HamanoMar 5, 2024
  14. 2/5 advice: make all entries stylistically consistentKristoffer Haugsbakk, Mar 4, 2024
  15. Junio C HamanoMar 4, 2024
  16. Kristoffer HaugsbakkMar 5, 2024
  17. 3/5 advice: use backticks for codeKristoffer Haugsbakk, Mar 4, 2024
  18. Junio C HamanoMar 4, 2024
  19. Kristoffer HaugsbakkMar 5, 2024
  20. 4/5 advice: use double quotes for regular quotingKristoffer Haugsbakk, Mar 4, 2024
  21. 5/5 branch: advise about ref syntax rulesKristoffer Haugsbakk, Mar 4, 2024
  22. 0/5 advise about ref syntax rulesKristoffer Haugsbakk, Mar 5, 2024
  23. 1/5 t3200: improve test styleKristoffer Haugsbakk, Mar 5, 2024
  24. 2/5 advice: make all entries stylistically consistentKristoffer Haugsbakk, Mar 5, 2024
  25. 3/5 advice: use backticks for verbatimKristoffer Haugsbakk, Mar 5, 2024
  26. 4/5 advice: use double quotes for regular quotingKristoffer Haugsbakk, Mar 5, 2024
  27. 5/5 branch: advise about ref syntax rulesKristoffer Haugsbakk, Mar 5, 2024
  28. Kristoffer HaugsbakkMar 3, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.