| Message ID | d275d1d179b90592ddd7b5da2ae4573b3f7a37b7.1709307442.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | branch: advise about ref syntax rules | expand |
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > Notes (series): > Hopefully I am using `advice.h` correctly here. Let's see. > - if (strbuf_check_branch_ref(ref, name)) > - die(_("'%s' is not a valid branch name"), name); > + if (strbuf_check_branch_ref(ref, name)) { > + error(_("'%s' is not a valid branch name"), name); > + advise(_("See `man git check-ref-format`")); > + exit(1); > + } This will give the message with "hint:" prefix, which is a good starting point. The message is given unconditionally, without any way to turn it off. For those who ... > 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. ... do not understand why, it is helpful, but once they learned, it is one extra line of unwanted text. If you want to give it a way to squelch, see the comment before where enum advice_type is declared in advice.h header file. The callsites would become something like advise_if_enabled(ADVICE_VALID_REF_NAME, _("See `man git check-ref-format` for valid refname syntax.")); Another thing is that rewriting die() into error() + advice() + manual exit() is an anti-pattern these days. int code = die_message(_("'%s' is not a valid branch name"), name); advice_if_enabled(...); /* see above */ exit(code); In the same source file, you will find an existing example to mimic. Thanks.
Hi > This will give the message with "hint:" prefix, which is a good > starting point. > > The message is given unconditionally, without any way to turn it > off. For those who ... > >> 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. > > ... do not understand why, it is helpful, but once they learned, it > is one extra line of unwanted text. If you want to give it a way to > squelch, see the comment before where enum advice_type is declared > in advice.h header file. I thought of doing that, but I reckoned that people who have a good intuition for the ref syntax would not get this error enough to want to turn if off. I’ll add a squelch option in the next version. Cheers
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > I thought of doing that, but I reckoned that people who have a good > intuition for the ref syntax would not get this error enough to want to > turn if off. If that is your choice, that is perfectly OK, as long as the proposed log message clearly records why we did not bother using advice_if_enabled(). If that is the case, then a rewrite for existing die() would become: int code = die_message(_("'%s' is not a valid branch name"), name); advise(_("See `man git check-ref-format`")); exit(code); Thanks.
diff --git a/branch.c b/branch.c index 6719a181bd1..1386918c60e 100644 --- a/branch.c +++ b/branch.c @@ -370,8 +370,11 @@ 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)) { + error(_("'%s' is not a valid branch name"), name); + advise(_("See `man git check-ref-format`")); + exit(1); + } return ref_exists(ref->buf); } diff --git a/builtin/branch.c b/builtin/branch.c index cfb63cce5fb..fa81e359157 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -576,8 +576,11 @@ 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 { + error(_("invalid branch name: '%s'"), oldname); + advise(_("See `man git check-ref-format`")); + exit(1); + } } for (int i = 0; worktrees[i]; i++) { diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index de7d3014e4f..9400a8baa35 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1725,4 +1725,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' ' test_cmp_config "" --default "" branch.foo5.merge ' +cat <<\EOF >expect +error: 'foo..bar' is not a valid branch name +hint: See `man git check-ref-format` +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 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. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Notes (series): Hopefully I am using `advice.h` correctly here. § git-replace(1) I did not add a hint for a similar message in `builtin/replace.c`. `builtin/replace.c` has an error message in `check_ref_valid` for an invalid ref name: ``` return error(_("'%s' is not a valid ref name"), ref->buf); ``` But there doesn’t seem to be a point to placing a hint here. The preceding calls to `repo_get_oid` will catch both missing refs and existing refs with invalid names: ``` if (repo_get_oid(r, refname, &object)) return error(_("failed to resolve '%s' as a valid ref"), refname); ``` Like for example this: ``` $ printf $(git rev-parse @~) > .git/refs/heads/hello..goodbye $ git replace @ hello..goodbye error: failed to resolve 'hello..goodbye' as a valid ref […] $ git replace @ non-existing error: failed to resolve 'non-existing' as a valid ref ``` § Alternatives (to this change) While working on this I also thought that it might be nice to have a man page `gitrefsyntax`. That one could use a lot of the content from `man git check-ref-format` verbatim. Then the hint could point towards that man page. And it seems that AsciiDoc supports _includes_ which means that the rules don’t have to be duplicated between the two man pages. branch.c | 7 +++++-- builtin/branch.c | 7 +++++-- t/t3200-branch.sh | 10 ++++++++++ 3 files changed, 20 insertions(+), 4 deletions(-)