| Message ID | 4ad5d4190649dcb5f26c73a6f15ab731891b9dfd.1709491818.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | advise about ref syntax rules | expand |
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 '
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?
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
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(-)