From 8c5001c68e8f768e7b97cb5c366bc0b1e7a31ba7 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Tue, 5 Mar 2024 21:29:39 +0100 Subject: [PATCH 1/5] t3200: improve test style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some tests use a preliminary heredoc for `expect` or have setup and teardown commands before and after, respectively. It is however preferred to keep all the logic in the test itself. Let’s move these into the tests. Also: • Remove a now-irrelevant comment about test placement and switch back to `main` post-test • Prefer indented literal heredocs (`-\EOF`) except for a block which says that this is intentional Helped-by: Junio C Hamano Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- t/t3200-branch.sh | 113 ++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 58 deletions(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index de7d3014e4fb91..060b27097e8a8f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -75,13 +75,13 @@ test_expect_success 'git branch HEAD should fail' ' test_must_fail git branch HEAD ' -cat >expect <expect <<-EOF && + $HEAD refs/heads/d/e/f@{0}: branch: Created from main + EOF git reflog show --no-abbrev-commit refs/heads/d/e/f >actual && test_cmp expect actual ' @@ -440,10 +440,10 @@ test_expect_success 'git branch --list -v with --abbrev' ' test_expect_success 'git branch --column' ' COLUMNS=81 git branch --column=column >actual && - cat >expect <<\EOF && - a/b/c bam foo l * main n o/p r - abc bar j/k m/m mb o/o q topic -EOF + cat >expect <<-\EOF && + a/b/c bam foo l * main n o/p r + abc bar j/k m/m mb o/o q topic + EOF test_cmp expect actual ' @@ -453,25 +453,25 @@ test_expect_success 'git branch --column with an extremely long branch name' ' test_when_finished "git branch -d $long" && git branch $long && COLUMNS=80 git branch --column=column >actual && - cat >expect <expect <<-EOF && + a/b/c + abc + bam + bar + foo + j/k + l + m/m + * main + mb + n + o/o + o/p + q + r + topic + $long + EOF test_cmp expect actual ' @@ -481,10 +481,10 @@ test_expect_success 'git branch with column.*' ' COLUMNS=80 git branch >actual && git config --unset column.branch && git config --unset column.ui && - cat >expect <<\EOF && - a/b/c bam foo l * main n o/p r - abc bar j/k m/m mb o/o q topic -EOF + cat >expect <<-\EOF && + a/b/c bam foo l * main n o/p r + abc bar j/k m/m mb o/o q topic + EOF test_cmp expect actual ' @@ -496,39 +496,36 @@ test_expect_success 'git branch -v with column.ui ignored' ' git config column.ui column && COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual && git config --unset column.ui && - cat >expect <<\EOF && - a/b/c - abc - bam - bar - foo - j/k - l - m/m -* main - mb - n - o/o - o/p - q - r - topic -EOF + cat >expect <<-\EOF && + a/b/c + abc + bam + bar + foo + j/k + l + m/m + * main + mb + n + o/o + o/p + q + r + topic + EOF test_cmp expect actual ' -mv .git/config .git/config-saved - test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' + test_when_finished mv .git/config-saved .git/config && + mv .git/config .git/config-saved && git branch -m q q2 && git branch -m q2 q ' -mv .git/config-saved .git/config - -git config branch.s/s.dummy Hello - test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' + git config branch.s/s.dummy Hello && git branch --create-reflog s/s && git reflog exists refs/heads/s/s && git branch --create-reflog s/t && @@ -1141,14 +1138,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 <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 ' From 95c987e6fad7cd3b3fe57542dbc6fb91532642d8 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Tue, 5 Mar 2024 21:29:40 +0100 Subject: [PATCH 2/5] advice: make all entries stylistically consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In general, rewrite entries to the following form: 1. Clause or sentence describing when the advice is shown 2. Optional “to ” clause which says what the advice is about (e.g. for resetNoRefresh: tell the user that they can use `--no-refresh`) Concretely: 1. Use “shown” instead of “advice shown” • “advice” is implied and a bit repetitive 2. Use “when” instead of “if” 3. Lead with “Shown when” and end the entry with the effect it has, where applicable 4. Use “the user” instead of “a user” or “you” 5. implicitIdentity: rewrite description in order to lead with *when* the advice is shown (see point (3)) 6. Prefer the present tense (with the exception of pushNonFFMatching) 7. waitingForEditor: give example of relevance in this new context 8. pushUpdateRejected: exception to the above principles Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 82 ++++++++++++++++----------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index c7ea70f2e2e9d2..72cd9f9e9d96fb 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -6,23 +6,23 @@ advice.*:: + -- addEmbeddedRepo:: - Advice on what to do when you've accidentally added one + Shown when the user accidentally adds one git repo inside of another. addEmptyPathspec:: - Advice shown if a user runs the add command without providing + Shown when the user runs the add command without providing the pathspec parameter. addIgnoredFile:: - Advice shown if a user attempts to add an ignored file to + Shown when the user attempts to add an ignored file to the index. amWorkDir:: - Advice that shows the location of the patch file when - linkgit:git-am[1] fails to apply it. + Shown when linkgit:git-am[1] fails to apply a patch + file, to tell the user the location of the file. ambiguousFetchRefspec:: - Advice shown when a fetch refspec for multiple remotes maps to + Shown when a fetch refspec for multiple remotes maps to the same remote-tracking branch namespace and causes branch tracking set-up to fail. checkoutAmbiguousRemoteBranchName:: - Advice shown when the argument to + Shown when the argument to linkgit:git-checkout[1] and linkgit:git-switch[1] ambiguously resolves to a remote tracking branch on more than one remote in @@ -33,31 +33,31 @@ advice.*:: to be used by default in some situations where this advice would be printed. commitBeforeMerge:: - Advice shown when linkgit:git-merge[1] refuses to + Shown when linkgit:git-merge[1] refuses to merge to avoid overwriting local changes. detachedHead:: - Advice shown when you used + Shown when the user uses linkgit:git-switch[1] or linkgit:git-checkout[1] - to move to the detached HEAD state, to instruct how to - create a local branch after the fact. + to move to the detached HEAD state, to tell the user how + to create a local branch after the fact. diverging:: - Advice shown when a fast-forward is not possible. + Shown when a fast-forward is not possible. fetchShowForcedUpdates:: - Advice shown when linkgit:git-fetch[1] takes a long time + Shown when linkgit:git-fetch[1] takes a long time to calculate forced updates after ref updates, or to warn that the check is disabled. forceDeleteBranch:: - Advice shown when a user tries to delete a not fully merged + Shown when the user tries to delete a not fully merged branch without the force option set. ignoredHook:: - Advice shown if a hook is ignored because the hook is not + Shown when a hook is ignored because the hook is not set as executable. implicitIdentity:: - Advice on how to set your identity configuration when - your information is guessed from the system username and - domain name. + Shown when the user's information is guessed from the + system username and domain name, to tell the user how to + set their identity configuration. nestedTag:: - Advice shown if a user attempts to recursively tag a tag object. + Shown when a user attempts to recursively tag a tag object. pushAlreadyExists:: Shown when linkgit:git-push[1] rejects an update that does not qualify for fast-forwarding (e.g., a tag.) @@ -71,12 +71,12 @@ advice.*:: object that is not a commit-ish, or make the remote ref point at an object that is not a commit-ish. pushNonFFCurrent:: - Advice shown when linkgit:git-push[1] fails due to a + Shown when linkgit:git-push[1] fails due to a non-fast-forward update to the current branch. pushNonFFMatching:: - Advice shown when you ran linkgit:git-push[1] and pushed - 'matching refs' explicitly (i.e. you used ':', or - specified a refspec that isn't your current branch) and + Shown when the user ran linkgit:git-push[1] and pushed + 'matching refs' explicitly (i.e. used ':', or + specified a refspec that isn't the current branch) and it resulted in a non-fast-forward error. pushRefNeedsUpdate:: Shown when linkgit:git-push[1] rejects a forced update of @@ -95,17 +95,17 @@ advice.*:: 'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate' simultaneously. resetNoRefresh:: - Advice to consider using the `--no-refresh` option to - linkgit:git-reset[1] when the command takes more than 2 seconds - to refresh the index after reset. + Shown when linkgit:git-reset[1] takes more than 2 + seconds to refresh the index after reset, to tell the user + that they can use the `--no-refresh` option. resolveConflict:: - Advice shown by various commands when conflicts + Shown by various commands when conflicts prevent the operation from being performed. rmHints:: - In case of failure in the output of linkgit:git-rm[1], - show directions on how to proceed from the current state. + Shown on failure in the output of linkgit:git-rm[1], to + give directions on how to proceed from the current state. sequencerInUse:: - Advice shown when a sequencer command is already in progress. + Shown when a sequencer command is already in progress. skippedCherryPicks:: Shown when linkgit:git-rebase[1] skips a commit that has already been cherry-picked onto the upstream branch. @@ -123,27 +123,27 @@ advice.*:: by linkgit:git-switch[1] or linkgit:git-checkout[1] when switching branches. statusUoption:: - Advise to consider using the `-u` option to linkgit:git-status[1] - when the command takes more than 2 seconds to enumerate untracked - files. + Shown when linkgit:git-status[1] takes more than 2 + seconds to enumerate untracked files, to tell the user that + they can use the `-u` option. submoduleAlternateErrorStrategyDie:: - Advice shown when a submodule.alternateErrorStrategy option + Shown when a submodule.alternateErrorStrategy option configured to "die" causes a fatal error. submodulesNotUpdated:: - Advice shown when a user runs a submodule command that fails + Shown when a user runs a submodule command that fails because `git submodule update --init` was not run. suggestDetachingHead:: - Advice shown when linkgit:git-switch[1] refuses to detach HEAD + Shown when linkgit:git-switch[1] refuses to detach HEAD without the explicit `--detach` option. updateSparsePath:: - Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] + Shown when either linkgit:git-add[1] or linkgit:git-rm[1] is asked to update index entries outside the current sparse checkout. waitingForEditor:: - Print a message to the terminal whenever Git is waiting for - editor input from the user. + Shown when Git is waiting for editor input. Relevant + when e.g. the editor is not launched inside the terminal. worktreeAddOrphan:: - Advice shown when a user tries to create a worktree from an - invalid reference, to instruct how to create a new unborn + Shown when the user tries to create a worktree from an + invalid reference, to tell the user how to create a new unborn branch instead. -- From 3ccc4782ce9b454c0428a7d6a4d0fd7c3d1eaea7 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Tue, 5 Mar 2024 21:29:41 +0100 Subject: [PATCH 3/5] advice: use backticks for verbatim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use backticks for inline-verbatim rather than single quotes. Also quote the unquoted ref globs. Also replace “the add command” with “`git add`”. Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 72cd9f9e9d96fb..c8d6c625f2a6cf 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -2,14 +2,14 @@ advice.*:: These variables control various optional help messages designed to aid new users. When left unconfigured, Git will give the message alongside instructions on how to squelch it. You can tell Git - that you do not need the help message by setting these to 'false': + that you do not need the help message by setting these to `false`: + -- addEmbeddedRepo:: Shown when the user accidentally adds one git repo inside of another. addEmptyPathspec:: - Shown when the user runs the add command without providing + Shown when the user runs `git add` without providing the pathspec parameter. addIgnoredFile:: Shown when the user attempts to add an ignored file to @@ -75,7 +75,7 @@ advice.*:: non-fast-forward update to the current branch. pushNonFFMatching:: Shown when the user ran linkgit:git-push[1] and pushed - 'matching refs' explicitly (i.e. used ':', or + 'matching refs' explicitly (i.e. used `:`, or specified a refspec that isn't the current branch) and it resulted in a non-fast-forward error. pushRefNeedsUpdate:: @@ -87,12 +87,12 @@ advice.*:: guess based on the source and destination refs what remote ref namespace the source belongs in, but where we can still suggest that the user push to either - refs/heads/* or refs/tags/* based on the type of the + `refs/heads/*` or `refs/tags/*` based on the type of the source object. pushUpdateRejected:: - Set this variable to 'false' if you want to disable - 'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists', - 'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate' + Set this variable to `false` if you want to disable + `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`, + `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate` simultaneously. resetNoRefresh:: Shown when linkgit:git-reset[1] takes more than 2 From 15cb03728f2fa6f153f873307ec92bb187681766 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Tue, 5 Mar 2024 21:29:42 +0100 Subject: [PATCH 4/5] advice: use double quotes for regular quoting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use double quotes like we use for “die” in this document. Signed-off-by: Kristoffer Haugsbakk Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index c8d6c625f2a6cf..dd52041bc94f13 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -75,7 +75,7 @@ advice.*:: non-fast-forward update to the current branch. pushNonFFMatching:: Shown when the user ran linkgit:git-push[1] and pushed - 'matching refs' explicitly (i.e. used `:`, or + "matching refs" explicitly (i.e. used `:`, or specified a refspec that isn't the current branch) and it resulted in a non-fast-forward error. pushRefNeedsUpdate:: From 8fbd903e58503cbdd1f1c816dd0c6c3c4d591b13 Mon Sep 17 00:00:00 2001 From: Kristoffer Haugsbakk Date: Tue, 5 Mar 2024 21:29:43 +0100 Subject: [PATCH 5/5] branch: advise about ref syntax rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 3 +++ advice.c | 1 + advice.h | 1 + branch.c | 8 ++++++-- builtin/branch.c | 8 ++++++-- t/t3200-branch.sh | 10 ++++++++++ 6 files changed, 27 insertions(+), 4 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index dd52041bc94f13..06c754899c553e 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:: + Shown when the user provides an illegal ref name, to + tell the user about the ref syntax documentation. resetNoRefresh:: Shown when linkgit:git-reset[1] takes more than 2 seconds to refresh the index after reset, to tell the user diff --git a/advice.c b/advice.c index 6e9098ff08935a..550c2968908251 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 9d4f49ae38bcfe..d15fe2351abcd9 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 6719a181bd1f03..621019fcf4bde0 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 cfb63cce5fb9df..1c122ee8a7b081 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 060b27097e8a8f..dd7525d1b8c502 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1722,4 +1722,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' ' test_cmp_config "" --default "" branch.foo5.merge ' +test_expect_success 'errors if given a bad branch name' ' + 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_must_fail git branch foo..bar >actual 2>&1 && + test_cmp expect actual +' + test_done