From: Junio C Hamano <gitster@pobox•com>
To: Kristoffer Haugsbakk <code@khaugsbakk•name>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v2 1/1] branch: advise about ref syntax rules
Date: Sun, 03 Mar 2024 14:42:59 -0800 [thread overview]
Message-ID: <xmqqil23uebw.fsf@gitster.g> (raw)
In-Reply-To: <4ad5d4190649dcb5f26c73a6f15ab731891b9dfd.1709491818.git.code@khaugsbakk.name> (Kristoffer Haugsbakk's message of "Sun, 3 Mar 2024 19:58:21 +0100")
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
'
next prev parent reply other threads:[~2024-03-03 22:43 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-01 15:38 [PATCH] branch: advise about ref syntax rules Kristoffer Haugsbakk
2024-03-01 18:06 ` Junio C Hamano
2024-03-01 18:13 ` Kristoffer Haugsbakk
2024-03-01 18:32 ` Junio C Hamano
2024-03-03 18:58 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
2024-03-03 18:58 ` [PATCH v2 1/1] branch: " Kristoffer Haugsbakk
2024-03-03 22:42 ` Junio C Hamano [this message]
2024-03-03 22:58 ` Kristoffer Haugsbakk
2024-03-04 22:07 ` [PATCH v3 0/5] " Kristoffer Haugsbakk
2024-03-04 22:07 ` [PATCH v3 1/5] t3200: improve test style Kristoffer Haugsbakk
2024-03-05 1:25 ` Junio C Hamano
2024-03-05 10:27 ` Kristoffer Haugsbakk
2024-03-05 16:02 ` Junio C Hamano
2024-03-04 22:07 ` [PATCH v3 2/5] advice: make all entries stylistically consistent Kristoffer Haugsbakk
2024-03-04 23:52 ` Junio C Hamano
2024-03-05 10:36 ` Kristoffer Haugsbakk
2024-03-04 22:07 ` [PATCH v3 3/5] advice: use backticks for code Kristoffer Haugsbakk
2024-03-04 23:54 ` Junio C Hamano
2024-03-05 10:29 ` Kristoffer Haugsbakk
2024-03-04 22:07 ` [PATCH v3 4/5] advice: use double quotes for regular quoting Kristoffer Haugsbakk
2024-03-04 22:07 ` [PATCH v3 5/5] branch: advise about ref syntax rules Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 0/5] " Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 1/5] t3200: improve test style Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 2/5] advice: make all entries stylistically consistent Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 3/5] advice: use backticks for verbatim Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 4/5] advice: use double quotes for regular quoting Kristoffer Haugsbakk
2024-03-05 20:29 ` [PATCH v4 5/5] branch: advise about ref syntax rules Kristoffer Haugsbakk
2024-03-03 19:10 ` [PATCH v2 0/1] " Kristoffer Haugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqil23uebw.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=code@khaugsbakk$(echo .)name \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox