Kristoffer Haugsbakk <code@xxxxxxxxxxxxxxx> 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
'
