{"thread":{"id":"58679","subject":"[PATCH] branch: error and informative messages","startedAt":"2022-10-23T22:29:52Z","lastAt":"2022-11-06T21:52:03Z","messageCount":6,"participants":["Rubén Justo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465603","messageId":"3f63d53d-2e14-ffe0-6263-2a15f83453ad@gmail.com","threadId":"58679","inReplyTo":null,"subject":"[PATCH] branch: error and informative messages","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-23T22:29:37Z","receivedAt":"2022-10-23T22:29:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Review die(), error(), warning() and output messages used in branch,\nfollowing the guidelines we have in Documentation/CodingGuideLines.\nMainly:\n\tError messages\n\n \t - Do not end error messages with a full stop.\n\n \t - Do not capitalize the first word, ...\n\nBeside applying this rules, some unneeded messages have been removed and\nothers reworded to normalize and simplify.\n\n - Four similar messages has been merged into the most common, the last\n   one:\n\t- \"no such branch '%s'\"\n\t- \"branch '%s' does not exist\"\n\t- \"invalid branch name: '%s'\"\n\t+ \"no branch named '%s'\"\n\n - Four specific messages has been reworded and merged:\n\t- \"too many branches for a copy operation\"\n\t- \"too many arguments for a rename operation\"\n\t- \"too many arguments to set new upstream\"\n\t- \"too many arguments to unset upstream\"\n\t+ \"too many arguments\"\n\n - Two specific messages has been reworded and merged:\n\t- \"could not set upstream of HEAD to %s when\n\t\tit does not point to any branch.\"\n\t- \"could not unset upstream of HEAD when it\n\t\tdoes not point to any branch.\"\n\t+ \"cannot modify upstream to detached HEAD\"\n\n - An error message reworded\n\t- \"options '%s' and '%s' cannot be used together\",\n\t\t\"--column\", \"--verbose\"\n\t+ \"options --column and --verbose used together\"\n\n - An error message reworded\n\t- \"The -a, and -r, options to 'git branch' do not take a branch name.\\n\"\n\t\t\"Did you mean to use: -a|-r --list <pattern>?\"));\n\t+ \"options -a and -r do not take a branch name\\n\"\n\t\t\"Did you mean to use: -a|-r --list <pattern>?\"));\n\n - Two error messages not needed, removed:\n\t- \"cannot copy the current branch while not on any.\"\n\t- \"cannot rename the current branch while not on any.\"\n\n - A deprecation message has been reworded, note the '\\n':\n\t- \"the '--set-upstream' option is no longer supported.\"\n\t\t\"Please use '--track' or '--set-upstream-to' instead.\"\n\t+ \"option --set-upstream is no longer supported\\n\"\n\t\t\"Please use --track or --set-upstream-to instead\"\n\n - \"%s\" and \"'%s'\" was used to format a branch name in different\n   messages.  \"'%s'\" has been used to normalize as it's the more\n   frequently used in this file and very common in the rest of the\n   codebase.  The opposite has been done for options: \"-a\" used vs\n   \"'-a'\".\n\nA new informative, non-error, message has been introduced for\ncreate_branch:\n\t+ \"New branch '%s' created from '%s'\\n\"\n\nThe tests for the modified messages have been updated and a new test for\nthe create_branch message has been added.\n\nFinally, let's change the return code on error for --edit-description,\nfrom -1 to 1.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/branch.c          | 163 ++++++++++++++++----------------------\n t/t1430-bad-ref-name.sh   |  10 +--\n t/t2407-worktree-heads.sh |   2 +-\n t/t3200-branch.sh         |  27 ++++---\n t/t3202-show-branch.sh    |  25 ++----\n 5 files changed, 96 insertions(+), 131 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 407517ba68..ca472436f0 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -159,17 +159,13 @@ static int branch_merged(int kind, const char *name,\n \t * any of the following code, but during the transition period,\n \t * a gentle reminder is in order.\n \t */\n-\tif ((head_rev != reference_rev) &&\n-\t    in_merge_bases(rev, head_rev) != merged) {\n-\t\tif (merged)\n-\t\t\twarning(_(\"deleting branch '%s' that has been merged to\\n\"\n-\t\t\t\t\"         '%s', but not yet merged to HEAD.\"),\n-\t\t\t\tname, reference_name);\n-\t\telse\n-\t\t\twarning(_(\"not deleting branch '%s' that is not yet merged to\\n\"\n+\tif ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)\n+\t\twarning(merged\n+\t\t\t? _(\"deleting branch '%s' that has been merged to\\n\"\n+\t\t\t\t\"         '%s', but not yet merged to HEAD.\")\n+\t\t\t: _(\"not deleting branch '%s' that is not yet merged to\\n\"\n \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n \t\t\t\tname, reference_name);\n-\t}\n \tfree(reference_name_to_free);\n \treturn merged;\n }\n@@ -179,16 +175,12 @@ static int check_branch_commit(const char *branchname, const char *refname,\n \t\t\t       int kinds, int force)\n {\n \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n-\tif (!force && !rev) {\n-\t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n-\t\treturn -1;\n-\t}\n-\tif (!force && !branch_merged(kinds, branchname, rev, head_rev)) {\n-\t\terror(_(\"The branch '%s' is not fully merged.\\n\"\n+\tif (!force && !rev)\n+\t\treturn error(_(\"couldn't look up commit object for '%s'\"), refname);\n+\tif (!force && !branch_merged(kinds, branchname, rev, head_rev))\n+\t\treturn error(_(\"the branch '%s' is not fully merged\\n\"\n \t\t      \"If you are sure you want to delete it, \"\n-\t\t      \"run 'git branch -D %s'.\"), branchname, branchname);\n-\t\treturn -1;\n-\t}\n+\t\t      \"run 'git branch -D %s'\"), branchname, branchname);\n \treturn 0;\n }\n \n@@ -197,7 +189,7 @@ static void delete_branch_config(const char *branchname)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstrbuf_addf(&buf, \"branch.%s\", branchname);\n \tif (git_config_rename_section(buf.buf, NULL) < 0)\n-\t\twarning(_(\"Update of config-file failed\"));\n+\t\twarning(_(\"update of config-file failed\"));\n \tstrbuf_release(&buf);\n }\n \n@@ -238,7 +230,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tif (!force) {\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n \t\tif (!head_rev)\n-\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n+\t\t\tdie(_(\"couldn't look up commit object for HEAD\"));\n \t}\n \n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n@@ -252,7 +244,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n \t\t\tconst char *path;\n \t\t\tif ((path = branch_checked_out(name))) {\n-\t\t\t\terror(_(\"Cannot delete branch '%s' \"\n+\t\t\t\terror(_(\"cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, path);\n \t\t\t\tret = 1;\n@@ -267,8 +259,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t&oid, &flags);\n \t\tif (!target) {\n \t\t\terror(remote_branch\n-\t\t\t      ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t      : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\t      ? _(\"remote-tracking branch '%s' not found\")\n+\t\t\t      : _(\"branch '%s' not found\"), bname.buf);\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -299,8 +291,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\tchar *refname = name + branch_name_pos;\n \t\t\tif (!quiet)\n \t\t\t\tprintf(remote_branch\n-\t\t\t\t\t? _(\"Deleted remote-tracking branch %s (was %s).\\n\")\n-\t\t\t\t\t: _(\"Deleted branch %s (was %s).\\n\"),\n+\t\t\t\t\t? _(\"Deleted remote-tracking branch '%s' (was %s)\\n\")\n+\t\t\t\t\t: _(\"Deleted branch '%s' (was %s)\\n\"),\n \t\t\t\t\tname + branch_name_pos, describe_ref);\n \n \t\t\tdelete_branch_config(refname);\n@@ -501,11 +493,11 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \t\t\tcontinue;\n \n \t\tif (is_worktree_being_rebased(wt, target))\n-\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n+\t\t\tdie(_(\"branch '%s' is being rebased at '%s'\"),\n \t\t\t    target, wt->path);\n \n \t\tif (is_worktree_being_bisected(wt, target))\n-\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n+\t\t\tdie(_(\"branch '%s' is being bisected at '%s'\"),\n \t\t\t    target, wt->path);\n \t}\n \n@@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n \n-\tif (!oldname) {\n-\t\tif (copy)\n-\t\t\tdie(_(\"cannot copy the current branch while not on any.\"));\n-\t\telse\n-\t\t\tdie(_(\"cannot rename the current branch while not on any.\"));\n-\t}\n-\n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n \t\t * Bad name --- this could be an attempt to rename a\n@@ -535,15 +520,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tif (ref_exists(oldref.buf))\n \t\t\trecovery = 1;\n \t\telse\n-\t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n+\t\t\tdie(_(\"no branch named '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n-\t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n-\t\telse\n-\t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n-\t}\n+\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf))\n+\t\tdie(copy && !strcmp(head, oldname)\n+\t\t\t? _(\"no commit on branch '%s' yet\")\n+\t\t\t: _(\"no branch named '%s'\"), oldname);\n \n \t/*\n \t * A command like \"git branch -M currentbranch currentbranch\" cannot\n@@ -561,32 +544,25 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tBUG(\"expected prefix missing for refs\");\n \t}\n \n-\tif (copy)\n-\t\tstrbuf_addf(&logmsg, \"Branch: copied %s to %s\",\n-\t\t\t    oldref.buf, newref.buf);\n-\telse\n-\t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n-\t\t\t    oldref.buf, newref.buf);\n+\tstrbuf_addf(&logmsg, copy\n+\t\t? \"Branch: copied %s to %s\"\n+\t\t: \"Branch: renamed %s to %s\", oldref.buf, newref.buf);\n \n \tif (!copy &&\n \t    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n \t    rename_ref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch rename failed\"));\n+\t\tdie(_(\"branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch copy failed\"));\n+\t\tdie(_(\"branch copy failed\"));\n \n-\tif (recovery) {\n-\t\tif (copy)\n-\t\t\twarning(_(\"Created a copy of a misnamed branch '%s'\"),\n+\tif (recovery)\n+\t\twarning(copy ? _(\"created a copy of a misnamed branch '%s'\")\n+\t\t\t     : _(\"renamed a misnamed branch '%s' away\"),\n \t\t\t\tinterpreted_oldname);\n-\t\telse\n-\t\t\twarning(_(\"Renamed a misnamed branch '%s' away\"),\n-\t\t\t\tinterpreted_oldname);\n-\t}\n \n \tif (!copy &&\n \t    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n+\t\tdie(_(\"branch renamed to '%s', but HEAD is not updated\"), newname);\n \n \tstrbuf_release(&logmsg);\n \n@@ -595,9 +571,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstrbuf_addf(&newsection, \"branch.%s\", interpreted_newname);\n \tstrbuf_release(&newref);\n \tif (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)\n-\t\tdie(_(\"Branch is renamed, but update of config-file failed\"));\n+\t\tdie(_(\"branch is renamed, but update of config-file failed\"));\n \tif (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)\n-\t\tdie(_(\"Branch is copied, but update of config-file failed\"));\n+\t\tdie(_(\"branch is copied, but update of config-file failed\"));\n \tstrbuf_release(&oldsection);\n \tstrbuf_release(&newsection);\n }\n@@ -715,11 +691,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \thead = resolve_refdup(\"HEAD\", 0, &head_oid, NULL);\n \tif (!head)\n-\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\t\tdie(_(\"failed to resolve HEAD as a valid ref\"));\n \tif (!strcmp(head, \"HEAD\"))\n \t\tfilter.detached = 1;\n \telse if (!skip_prefix(head, \"refs/heads/\", &head))\n-\t\tdie(_(\"HEAD not found below refs/heads!\"));\n+\t\tdie(_(\"HEAD not found below refs/heads\"));\n \n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t     0);\n@@ -740,9 +716,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules_explicit) {\n \t\tif (!submodule_propagate_branches)\n-\t\t\tdie(_(\"branch with --recurse-submodules can only be used if submodule.propagateBranches is enabled\"));\n+\t\t\tdie(_(\"branch with --recurse-submodules can only be \"\n+\t\t\t\t\"used if submodule.propagateBranches is enabled\"));\n \t\tif (noncreate_actions)\n-\t\t\tdie(_(\"--recurse-submodules can only be used to create branches\"));\n+\t\t\tdie(_(\"--recurse-submodules can only be used to \"\n+\t\t\t\t\"create branches\"));\n \t}\n \n \trecurse_submodules =\n@@ -756,7 +734,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tfinalize_colopts(&colopts, -1);\n \tif (filter.verbose) {\n \t\tif (explicitly_enable_column(colopts))\n-\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--column\", \"--verbose\");\n+\t\t\tdie(_(\"options --column and --verbose used together\"));\n \t\tcolopts = 0;\n \t}\n \n@@ -805,7 +783,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n-\t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n+\t\t\t\tdie(_(\"cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n \t\t} else if (argc == 1) {\n \t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n@@ -816,9 +794,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\tret = error((!argc || !strcmp(head, branch_name))\n-\t\t\t      ? _(\"No commit on branch '%s' yet.\")\n-\t\t\t      : _(\"No branch named '%s'.\"),\n+\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\t      ? _(\"no commit on branch '%s' yet\")\n+\t\t\t      : _(\"no branch named '%s'\"),\n \t\t\t      branch_name);\n \t\telse if (!edit_branch_description(branch_name))\n \t\t\tret = 0; /* happy */\n@@ -835,7 +813,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse if (argc == 2)\n \t\t\tcopy_or_rename_branch(argv[0], argv[1], 1, copy > 1);\n \t\telse\n-\t\t\tdie(_(\"too many branches for a copy operation\"));\n+\t\t\tdie(_(\"too many arguments\"));\n \t} else if (rename) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n@@ -844,7 +822,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse if (argc == 2)\n \t\t\tcopy_or_rename_branch(argv[0], argv[1], 0, rename > 1);\n \t\telse\n-\t\t\tdie(_(\"too many arguments for a rename operation\"));\n+\t\t\tdie(_(\"too many arguments\"));\n \t} else if (new_upstream) {\n \t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n@@ -855,21 +833,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n \t\t\tbranch = branch_get(buf.buf);\n \t\t} else\n-\t\t\tdie(_(\"too many arguments to set new upstream\"));\n+\t\t\tdie(_(\"too many arguments\"));\n \n \t\tif (!branch) {\n \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n-\t\t\t\tdie(_(\"could not set upstream of HEAD to %s when \"\n-\t\t\t\t      \"it does not point to any branch.\"),\n-\t\t\t\t    new_upstream);\n-\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\t\t\t\tdie(_(\"cannot modify upstream to detached HEAD\"));\n+\t\t\tdie(_(\"no branch named '%s'\"), argv[0]);\n \t\t}\n \n-\t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n-\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n-\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n-\t\t}\n+\t\tif (!ref_exists(branch->refname))\n+\t\t\tdie(!argc || !strcmp(head, branch->name)\n+\t\t\t\t? _(\"no commit on branch '%s' yet\")\n+\t\t\t\t: _(\"no branch named '%s'\"), branch->name);\n \n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n@@ -885,17 +860,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n \t\t\tbranch = branch_get(buf.buf);\n \t\t} else\n-\t\t\tdie(_(\"too many arguments to unset upstream\"));\n+\t\t\tdie(_(\"too many arguments\"));\n \n \t\tif (!branch) {\n \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n-\t\t\t\tdie(_(\"could not unset upstream of HEAD when \"\n-\t\t\t\t      \"it does not point to any branch.\"));\n-\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\t\t\t\tdie(_(\"cannot modify upstream to detached HEAD\"));\n+\t\t\tdie(_(\"no branch named '%s'\"), argv[0]);\n \t\t}\n \n \t\tif (!branch_has_merge_config(branch))\n-\t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n+\t\t\tdie(_(\"branch '%s' has no upstream information\"), branch->name);\n \n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n@@ -909,20 +883,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tconst char *start_name = argc == 2 ? argv[1] : head;\n \n \t\tif (filter.kind != FILTER_REFS_BRANCHES)\n-\t\t\tdie(_(\"The -a, and -r, options to 'git branch' do not take a branch name.\\n\"\n-\t\t\t\t  \"Did you mean to use: -a|-r --list <pattern>?\"));\n+\t\t\tdie(_(\"options -a and -r do not take a branch name\\n\"\n+\t\t\t\t\"Did you mean to use: -a|-r --list <pattern>?\"));\n \n \t\tif (track == BRANCH_TRACK_OVERRIDE)\n-\t\t\tdie(_(\"the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.\"));\n+\t\t\tdie(_(\"option --set-upstream is no longer supported\\n\"\n+\t\t\t\t\"Please use --track or --set-upstream-to instead\"));\n \n-\t\tif (recurse_submodules) {\n+\t\tif (recurse_submodules)\n \t\t\tcreate_branches_recursively(the_repository, branch_name,\n \t\t\t\t\t\t    start_name, NULL, force,\n \t\t\t\t\t\t    reflog, quiet, track, 0);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tcreate_branch(the_repository, branch_name, start_name, force, 0,\n-\t\t\t      reflog, quiet, track, 0);\n+\t\telse\n+\t\t\tcreate_branch(the_repository, branch_name, start_name,\n+\t\t\t\t\tforce, 0, reflog, quiet, track, 0);\n+\n+\t\tprintf(_(\"New branch '%s' created from '%s'\\n\"),\n+\t\t\tbranch_name, start_name);\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex ff1c967d55..de3249eb9d 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -192,7 +192,7 @@ test_expect_success 'branch -d can delete broken name' '\n \ttest-tool ref-store main update-ref msg \"refs/heads/broken...ref\" $main_sha1 $ZERO_OID REF_SKIP_REFNAME_VERIFICATION &&\n \ttest_when_finished \"test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref\" &&\n \tgit branch -d broken...ref >output 2>error &&\n-\ttest_i18ngrep \"Deleted branch broken...ref (was broken)\" output &&\n+\ttest_i18ngrep \"Deleted branch '\"'\"'broken...ref'\"'\"' (was broken)\" output &&\n \ttest_must_be_empty error &&\n \tgit branch >output 2>error &&\n \t! grep -e \"broken\\.\\.\\.ref\" error &&\n@@ -218,7 +218,7 @@ test_expect_success 'branch -d can delete symref to broken name' '\n \ttest_when_finished \"test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname\" &&\n \tgit branch -d badname >output 2>error &&\n \ttest_path_is_missing .git/refs/heads/badname &&\n-\ttest_i18ngrep \"Deleted branch badname (was refs/heads/broken\\.\\.\\.ref)\" output &&\n+\ttest_i18ngrep \"Deleted branch '\"'\"'badname'\"'\"' (was refs/heads/broken\\.\\.\\.ref)\" output &&\n \ttest_must_be_empty error\n '\n \n@@ -236,7 +236,7 @@ test_expect_success 'branch -d can delete dangling symref to broken name' '\n \ttest_when_finished \"test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname\" &&\n \tgit branch -d badname >output 2>error &&\n \ttest_path_is_missing .git/refs/heads/badname &&\n-\ttest_i18ngrep \"Deleted branch badname (was refs/heads/broken\\.\\.\\.ref)\" output &&\n+\ttest_i18ngrep \"Deleted branch '\"'\"'badname'\"'\"' (was refs/heads/broken\\.\\.\\.ref)\" output &&\n \ttest_must_be_empty error\n '\n \n@@ -265,7 +265,7 @@ test_expect_success 'branch -d can delete symref with broken name' '\n \ttest_when_finished \"test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref\" &&\n \tgit branch -d broken...symref >output 2>error &&\n \ttest_path_is_missing .git/refs/heads/broken...symref &&\n-\ttest_i18ngrep \"Deleted branch broken...symref (was refs/heads/main)\" output &&\n+\ttest_i18ngrep \"Deleted branch '\"'\"'broken...symref'\"'\"' (was refs/heads/main)\" output &&\n \ttest_must_be_empty error\n '\n \n@@ -283,7 +283,7 @@ test_expect_success 'branch -d can delete dangling symref with broken name' '\n \ttest_when_finished \"test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref\" &&\n \tgit branch -d broken...symref >output 2>error &&\n \ttest_path_is_missing .git/refs/heads/broken...symref &&\n-\ttest_i18ngrep \"Deleted branch broken...symref (was refs/heads/idonotexist)\" output &&\n+\ttest_i18ngrep \"Deleted branch '\"'\"'broken...symref'\"'\"' (was refs/heads/idonotexist)\" output &&\n \ttest_must_be_empty error\n '\n \ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 019a40df2c..b0992d5f91 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \t\tgrep \"cannot force update the branch\" err &&\n \n \t\ttest_must_fail git branch -D wt-$i 2>err &&\n-\t\tgrep \"Cannot delete branch\" err || return 1\n+\t\tgrep \"cannot delete branch\" err || return 1\n \tdone\n '\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7d8edff9c3..326c505c29 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -39,7 +39,10 @@ test_expect_success 'branch -h in broken repository' '\n '\n \n test_expect_success 'git branch abc should create a branch' '\n-\tgit branch abc && test_path_is_file .git/refs/heads/abc\n+\tgit branch abc >actual &&\n+\techo \"New branch '\"'\"'abc'\"'\"' created from '\"'\"'main'\"'\"'\" >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_file .git/refs/heads/abc\n '\n \n test_expect_success 'git branch abc should fail when abc exists' '\n@@ -716,7 +719,7 @@ EOF\n test_expect_success 'deleting a symref' '\n \tgit branch target &&\n \tgit symbolic-ref refs/heads/symref refs/heads/target &&\n-\techo \"Deleted branch symref (was refs/heads/target).\" >expect &&\n+\techo \"Deleted branch '\"'\"'symref'\"'\"' (was refs/heads/target)\" >expect &&\n \tgit branch -d symref >actual &&\n \ttest_path_is_file .git/refs/heads/target &&\n \ttest_path_is_missing .git/refs/heads/symref &&\n@@ -726,7 +729,7 @@ test_expect_success 'deleting a symref' '\n test_expect_success 'deleting a dangling symref' '\n \tgit symbolic-ref refs/heads/dangling-symref nowhere &&\n \ttest_path_is_file .git/refs/heads/dangling-symref &&\n-\techo \"Deleted branch dangling-symref (was nowhere).\" >expect &&\n+\techo \"Deleted branch '\"'\"'dangling-symref'\"'\"' (was nowhere)\" >expect &&\n \tgit branch -d dangling-symref >actual &&\n \ttest_path_is_missing .git/refs/heads/dangling-symref &&\n \ttest_cmp expect actual\n@@ -735,7 +738,7 @@ test_expect_success 'deleting a dangling symref' '\n test_expect_success 'deleting a self-referential symref' '\n \tgit symbolic-ref refs/heads/self-reference refs/heads/self-reference &&\n \ttest_path_is_file .git/refs/heads/self-reference &&\n-\techo \"Deleted branch self-reference (was refs/heads/self-reference).\" >expect &&\n+\techo \"Deleted branch '\"'\"'self-reference'\"'\"' (was refs/heads/self-reference)\" >expect &&\n \tgit branch -d self-reference >actual &&\n \ttest_path_is_missing .git/refs/heads/self-reference &&\n \ttest_cmp expect actual\n@@ -853,7 +856,7 @@ test_expect_success 'test deleting branch deletes branch config' '\n test_expect_success 'test deleting branch without config' '\n \tgit branch my7 s &&\n \tsha1=$(git rev-parse my7 | cut -c 1-7) &&\n-\techo \"Deleted branch my7 (was $sha1).\" >expect &&\n+\techo \"Deleted branch '\"'\"'my7'\"'\"' (was $sha1)\" >expect &&\n \tgit branch -d my7 >actual 2>&1 &&\n \ttest_cmp expect actual\n '\n@@ -923,7 +926,7 @@ test_expect_success 'simple tracking skips when remote ref is not a branch' '\n '\n \n test_expect_success '--set-upstream-to fails on multiple branches' '\n-\techo \"fatal: too many arguments to set new upstream\" >expect &&\n+\techo \"fatal: too many arguments\" >expect &&\n \ttest_must_fail git branch --set-upstream-to main a b c 2>err &&\n \ttest_cmp expect err\n '\n@@ -931,13 +934,13 @@ test_expect_success '--set-upstream-to fails on multiple branches' '\n test_expect_success '--set-upstream-to fails on detached HEAD' '\n \tgit checkout HEAD^{} &&\n \ttest_when_finished git checkout - &&\n-\techo \"fatal: could not set upstream of HEAD to main when it does not point to any branch.\" >expect &&\n+\techo \"fatal: cannot modify upstream to detached HEAD\" >expect &&\n \ttest_must_fail git branch --set-upstream-to main 2>err &&\n \ttest_cmp expect err\n '\n \n test_expect_success '--set-upstream-to fails on a missing dst branch' '\n-\techo \"fatal: branch '\"'\"'does-not-exist'\"'\"' does not exist\" >expect &&\n+\techo \"fatal: no branch named '\"'\"'does-not-exist'\"'\"'\" >expect &&\n \ttest_must_fail git branch --set-upstream-to main does-not-exist 2>err &&\n \ttest_cmp expect err\n '\n@@ -979,7 +982,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '\n '\n \n test_expect_success '--unset-upstream should fail if given a non-existent branch' '\n-\techo \"fatal: Branch '\"'\"'i-dont-exist'\"'\"' has no upstream information\" >expect &&\n+\techo \"fatal: branch '\"'\"'i-dont-exist'\"'\"' has no upstream information\" >expect &&\n \ttest_must_fail git branch --unset-upstream i-dont-exist 2>err &&\n \ttest_cmp expect err\n '\n@@ -1001,13 +1004,13 @@ test_expect_success 'test --unset-upstream on HEAD' '\n \ttest_must_fail git config branch.main.remote &&\n \ttest_must_fail git config branch.main.merge &&\n \t# fail for a branch without upstream set\n-\techo \"fatal: Branch '\"'\"'main'\"'\"' has no upstream information\" >expect &&\n+\techo \"fatal: branch '\"'\"'main'\"'\"' has no upstream information\" >expect &&\n \ttest_must_fail git branch --unset-upstream 2>err &&\n \ttest_cmp expect err\n '\n \n test_expect_success '--unset-upstream should fail on multiple branches' '\n-\techo \"fatal: too many arguments to unset upstream\" >expect &&\n+\techo \"fatal: too many arguments\" >expect &&\n \ttest_must_fail git branch --unset-upstream a b c 2>err &&\n \ttest_cmp expect err\n '\n@@ -1015,7 +1018,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '\n test_expect_success '--unset-upstream should fail on detached HEAD' '\n \tgit checkout HEAD^{} &&\n \ttest_when_finished git checkout - &&\n-\techo \"fatal: could not unset upstream of HEAD when it does not point to any branch.\" >expect &&\n+\techo \"fatal: cannot modify upstream to detached HEAD\" >expect &&\n \ttest_must_fail git branch --unset-upstream 2>err &&\n \ttest_cmp expect err\n '\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..6b4be35e4c 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -9,9 +9,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n \n test_expect_success 'error descriptions on empty repository' '\n \tcurrent=$(git branch --show-current) &&\n-\tcat >expect <<-EOF &&\n-\terror: No commit on branch '\\''$current'\\'' yet.\n-\tEOF\n+\techo \"error: no commit on branch '\"'\"'$current'\"'\"' yet\" > expect &&\n \ttest_must_fail git branch --edit-description 2>actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git branch --edit-description $current 2>actual &&\n@@ -20,9 +18,7 @@ test_expect_success 'error descriptions on empty repository' '\n \n test_expect_success 'fatal descriptions on empty repository' '\n \tcurrent=$(git branch --show-current) &&\n-\tcat >expect <<-EOF &&\n-\tfatal: No commit on branch '\\''$current'\\'' yet.\n-\tEOF\n+\techo \"fatal: no commit on branch '\"'\"'$current'\"'\"' yet\" > expect &&\n \ttest_must_fail git branch --set-upstream-to=non-existent 2>actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git branch -c new-branch 2>actual &&\n@@ -198,23 +194,12 @@ done <<\\EOF\n EOF\n \n test_expect_success 'error descriptions on non-existent branch' '\n-\tcat >expect <<-EOF &&\n-\terror: No branch named '\\''non-existent'\\'.'\n-\tEOF\n+\techo \"error: no branch named '\"'\"'non-existent'\"'\"'\" > expect &&\n \ttest_must_fail git branch --edit-description non-existent 2>actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'fatal descriptions on non-existent branch' '\n-\tcat >expect <<-EOF &&\n-\tfatal: branch '\\''non-existent'\\'' does not exist\n-\tEOF\n+\ttest_cmp expect actual &&\n+\techo \"fatal: no branch named '\"'\"'non-existent'\"'\"'\" > expect &&\n \ttest_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&\n \ttest_cmp expect actual &&\n-\n-\tcat >expect <<-EOF &&\n-\tfatal: No branch named '\\''non-existent'\\''.\n-\tEOF\n \ttest_must_fail git branch -c non-existent new-branch 2>actual &&\n \ttest_cmp expect actual &&\n \ttest_must_fail git branch -m non-existent new-branch 2>actual &&\n-- \n2.36.1\n"},{"id":"465605","messageId":"xmqq5ygaul5k.fsf@gitster.g","threadId":"58679","inReplyTo":"3f63d53d-2e14-ffe0-6263-2a15f83453ad@gmail.com","subject":"Re: [PATCH] branch: error and informative messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T00:20:55Z","receivedAt":"2022-10-24T00:21:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Review die(), error(), warning() and output messages used in branch,\n> following the guidelines we have in Documentation/CodingGuideLines.\n> Mainly:\n> \tError messages\n>\n>  \t - Do not end error messages with a full stop.\n>\n>  \t - Do not capitalize the first word, ...\n\nOK.\n\n>\n> Beside applying this rules, some unneeded messages have been removed and\n> others reworded to normalize and simplify.\n>\n>  - Four similar messages has been merged into the most common, the last\n>    one:\n> \t- \"no such branch '%s'\"\n> \t- \"branch '%s' does not exist\"\n> \t- \"invalid branch name: '%s'\"\n> \t+ \"no branch named '%s'\"\n\nThe third one could be more appropriate than the fourth one when the\nerror is about failing to create a new branch due to invalid name.\nBut all the other three seem to refer to a branch that ought to\nexist but doesn't, and the one you picked seems reasonable one among\nthe three (and it is also the shortest).\n\n>  - Two specific messages has been reworded and merged:\n> \t- \"could not set upstream of HEAD to %s when\n> \t\tit does not point to any branch.\"\n> \t- \"could not unset upstream of HEAD when it\n> \t\tdoes not point to any branch.\"\n> \t+ \"cannot modify upstream to detached HEAD\"\n\nOK.  I have no strong opinion between \"modify\" and \"set or unset\"\nbut the latter may be closer to the spirit of the former.  I agree\nthat \"HEAD when it does not ...\" is quite a mouthful and to those\nwho understand what a 'detached HEAD' is, the updated phrasing may\nbe easier to read.\n\n>  - An error message reworded\n> \t- \"options '%s' and '%s' cannot be used together\",\n> \t\t\"--column\", \"--verbose\"\n> \t+ \"options --column and --verbose used together\"\n\nThis is arguably a regression.  The original ensures that l10n/i18n\nfolks cannot frob the option names that must be literally spelled,\nbut the \"reworded\" one lacks the protection (it also loses the quotes\naround dashed options).\n\n>  - An error message reworded\n> \t- \"The -a, and -r, options to 'git branch' do not take a branch name.\\n\"\n> \t\t\"Did you mean to use: -a|-r --list <pattern>?\"));\n> \t+ \"options -a and -r do not take a branch name\\n\"\n> \t\t\"Did you mean to use: -a|-r --list <pattern>?\"));\n\nOK.\n\n>  - Two error messages not needed, removed:\n> \t- \"cannot copy the current branch while not on any.\"\n> \t- \"cannot rename the current branch while not on any.\"\n\nOK (assuming that these are unreachable code).\n\n>  - A deprecation message has been reworded, note the '\\n':\n> \t- \"the '--set-upstream' option is no longer supported.\"\n> \t\t\"Please use '--track' or '--set-upstream-to' instead.\"\n> \t+ \"option --set-upstream is no longer supported\\n\"\n> \t\t\"Please use --track or --set-upstream-to instead\"\n\nLoss of quotes around option names, and the definite article \"the\"?\n\n>  - \"%s\" and \"'%s'\" was used to format a branch name in different\n>    messages.  \"'%s'\" has been used to normalize as it's the more\n>    frequently used in this file and very common in the rest of the\n>    codebase.  The opposite has been done for options: \"-a\" used vs\n>    \"'-a'\".\n\nAs the way to display the exact literal string, I think it is good\nto have quotes around both of them, the user-chosen names like\nbranch names, and the system-chosen names like option names.\n\n> A new informative, non-error, message has been introduced for\n> create_branch:\n> \t+ \"New branch '%s' created from '%s'\\n\"\n\nOK.\n\n> The tests for the modified messages have been updated and a new test for\n> the create_branch message has been added.\n>\n> Finally, let's change the return code on error for --edit-description,\n> from -1 to 1.\n\nOK.  That last one may be better to be a separate patch, as these\nwording changes are subject to discussion and bikeshedding.\n\n> -\tif ((head_rev != reference_rev) &&\n> -\t    in_merge_bases(rev, head_rev) != merged) {\n> -\t\tif (merged)\n> -\t\t\twarning(_(\"deleting branch '%s' that has been merged to\\n\"\n> -\t\t\t\t\"         '%s', but not yet merged to HEAD.\"),\n> -\t\t\t\tname, reference_name);\n> -\t\telse\n> -\t\t\twarning(_(\"not deleting branch '%s' that is not yet merged to\\n\"\n> +\tif ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)\n> +\t\twarning(merged\n> +\t\t\t? _(\"deleting branch '%s' that has been merged to\\n\"\n> +\t\t\t\t\"         '%s', but not yet merged to HEAD.\")\n> +\t\t\t: _(\"not deleting branch '%s' that is not yet merged to\\n\"\n>  \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n>  \t\t\t\tname, reference_name);\n> -\t}\n\nThis does not fall into any of the categories the proposed log\nmessage discussed.  Rather, it looks more like \"the code\nsubjectively looks better this way\".  It happens to much my\nsubjective taste, but that does not change the fact that we\nshouldn't distract reviewers with such an unrelated change in the\nsame patch.\n\n> @@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>  \tconst char *interpreted_newname = NULL;\n>  \tint recovery = 0;\n>  \n> -\tif (!oldname) {\n> -\t\tif (copy)\n> -\t\t\tdie(_(\"cannot copy the current branch while not on any.\"));\n> -\t\telse\n> -\t\t\tdie(_(\"cannot rename the current branch while not on any.\"));\n> -\t}\n> -\n\nSomething like\n\n    copy_or_rename_branch() never gets NULL in oldname, because its\n    only callers in cmd_branch() calls it with either the end-user's\n    command line argument or the result of resolve_refdup(\"HEAD\"),\n    and neither can ever be NULL because ...\n\nneeds to go into the proposed log message to explain why it is safe\nto remove these two messages.\n\nHaving said that, the messages may actually be correct and it is the\nlogic that makes it unreachable that is wrong in this case, I think.\nIt looks like the code expects that oldname is NULL when we are on\ndetached HEAD (it could be the old caller did have NULL in head when\nthis code was written, and it is possible that we regressed over the\ncourse of history).  I.e. Isn't it trying to protect us against\n\n   $ git checkout --detach HEAD^0\n   $ git branch -m foo\t\t;# rename .git/HEAD to .git/foo???\n\n(or \"-c\" for copy)?  The current code happens to catch it a bit\nlater in this function, when it notices \"HEAD\" is not a refname in\n\"refs/heads/\" hierarchy, but the user never attempted to rename\nrefs/heads/HEAD to refs/heads/foo, and \"Invalid branch name: 'HEAD'\"\nis us being wrong, insulting and confusing to the users.\n\nI suspect that this needs to be fixed at the caller's level, just\nlike the caller reacts to \"edit_description\" by checking the\nfilter.detached bit and rejects the request, the caller should\nnotice that we are on a detached HEAD and die with one of the above\ntwo messages without calling this function.\n\nAnd that should be a separate patch, that can be reviewed and\napplied regardless of the rest of \"error messages cleanup\" topic.\n\nThe hunks before this point that I omitted from quoting and\ncommenting looked all OK.\n\nThe remainder of the patch I didn't look at.  There may be similar\nissues like this \"oops, the messages are dead for a wrong reason and\ncode needs to be fixed\" or \"let's leave subjective improvements out\nof this topic that has clearly described rules\" to be discovered, so\nI may come back to them later, or others may find them before I do.\n\nThanks for working on this one.\n\n\n"},{"id":"465671","messageId":"faf7a985-f6ef-f20a-3857-031396124d60@gmail.com","threadId":"58679","inReplyTo":"xmqq5ygaul5k.fsf@gitster.g","subject":"Re: [PATCH] branch: error and informative messages","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-24T23:39:02Z","receivedAt":"2022-10-25T00:55:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 24/10/22 2:20, Junio C Hamano wrote:\n\n>> Beside applying this rules, some unneeded messages have been removed and\n>> others reworded to normalize and simplify.\n>>\n>>  - Four similar messages has been merged into the most common, the last\n>>    one:\n>> \t- \"no such branch '%s'\"\n>> \t- \"branch '%s' does not exist\"\n>> \t- \"invalid branch name: '%s'\"\n>> \t+ \"no branch named '%s'\"\n> \n> The third one could be more appropriate than the fourth one when the\n> error is about failing to create a new branch due to invalid name.\n> But all the other three seem to refer to a branch that ought to\n> exist but doesn't, and the one you picked seems reasonable one among\n> the three (and it is also the shortest).\n\nYes, I had doubts with the third.  The error is referring to the source branch\nin the copy/rename operation, so I think it makes sense to say that the branch\ndoesn't exist, even if it couldn't.\n\n>>  - Two specific messages has been reworded and merged:\n>> \t- \"could not set upstream of HEAD to %s when\n>> \t\tit does not point to any branch.\"\n>> \t- \"could not unset upstream of HEAD when it\n>> \t\tdoes not point to any branch.\"\n>> \t+ \"cannot modify upstream to detached HEAD\"\n> \n> OK.  I have no strong opinion between \"modify\" and \"set or unset\"\n> but the latter may be closer to the spirit of the former.  I agree\n> that \"HEAD when it does not ...\" is quite a mouthful and to those\n> who understand what a 'detached HEAD' is, the updated phrasing may\n> be easier to read.\n\nI prefer a single term like 'modify' as I find it less confuse than 'set or\nunset'.\n\n>>  - An error message reworded\n>> \t- \"options '%s' and '%s' cannot be used together\",\n>> \t\t\"--column\", \"--verbose\"\n>> \t+ \"options --column and --verbose used together\"\n> \n> This is arguably a regression.  The original ensures that l10n/i18n\n> folks cannot frob the option names that must be literally spelled,\n> but the \"reworded\" one lacks the protection (it also loses the quotes\n> around dashed options).\n\nOK. I didn't think about that, I'll revert to the parametrized form.  About the\nquotes, I did that on purpose.  As I said in a comment below, for branches the\nquotes are commonly used and make sense as it is a variable value and the\nquotes helps to delimit, but with options the literal is well-known and we use\nthe form with dashes, ie --verbose, so it is easily identifiable.  Also, we do\nnot use quotes with option literals in other places, like the documentation,\n--help,..\n\n>>  - Two error messages not needed, removed:\n>> \t- \"cannot copy the current branch while not on any.\"\n>> \t- \"cannot rename the current branch while not on any.\"\n> \n> OK (assuming that these are unreachable code).\n \nIt is, you reviewed it below.  I'll comment there.\n\n>>  - A deprecation message has been reworded, note the '\\n':\n>> \t- \"the '--set-upstream' option is no longer supported.\"\n>> \t\t\"Please use '--track' or '--set-upstream-to' instead.\"\n>> \t+ \"option --set-upstream is no longer supported\\n\"\n>> \t\t\"Please use --track or --set-upstream-to instead\"\n> \n> Loss of quotes around option names, and the definite article \"the\"?\n \nThe loss of the article, it sounds good to me and in line with the previous\n\"options --column ...\".  Dunno.\n\n>>  - \"%s\" and \"'%s'\" was used to format a branch name in different\n>>    messages.  \"'%s'\" has been used to normalize as it's the more\n>>    frequently used in this file and very common in the rest of the\n>>    codebase.  The opposite has been done for options: \"-a\" used vs\n>>    \"'-a'\".\n> \n> As the way to display the exact literal string, I think it is good\n> to have quotes around both of them, the user-chosen names like\n> branch names, and the system-chosen names like option names.\n \nSame reasoning as above.  It is a system-chosen term, but the message\nhas not a placeholder to put a value, we're using a literal.\n\n>> A new informative, non-error, message has been introduced for\n>> create_branch:\n>> \t+ \"New branch '%s' created from '%s'\\n\"\n> \n> OK.\n> \n>> The tests for the modified messages have been updated and a new test for\n>> the create_branch message has been added.\n>>\n>> Finally, let's change the return code on error for --edit-description,\n>> from -1 to 1.\n> \n> OK.  That last one may be better to be a separate patch, as these\n> wording changes are subject to discussion and bikeshedding.\n> \n\nMmm, I thought about that.  This change is one that we've been delaying because\nit might break something due to a change in the way we report errors.  We're\nspecifically changing this here and the change is small, so I found appropriate\nto do it here.\n\n>> -\tif ((head_rev != reference_rev) &&\n>> -\t    in_merge_bases(rev, head_rev) != merged) {\n>> -\t\tif (merged)\n>> -\t\t\twarning(_(\"deleting branch '%s' that has been merged to\\n\"\n>> -\t\t\t\t\"         '%s', but not yet merged to HEAD.\"),\n>> -\t\t\t\tname, reference_name);\n>> -\t\telse\n>> -\t\t\twarning(_(\"not deleting branch '%s' that is not yet merged to\\n\"\n>> +\tif ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)\n>> +\t\twarning(merged\n>> +\t\t\t? _(\"deleting branch '%s' that has been merged to\\n\"\n>> +\t\t\t\t\"         '%s', but not yet merged to HEAD.\")\n>> +\t\t\t: _(\"not deleting branch '%s' that is not yet merged to\\n\"\n>>  \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n>>  \t\t\t\tname, reference_name);\n>> -\t}\n> \n> This does not fall into any of the categories the proposed log\n> message discussed.  Rather, it looks more like \"the code\n> subjectively looks better this way\".  It happens to much my\n> subjective taste, but that does not change the fact that we\n> shouldn't distract reviewers with such an unrelated change in the\n> same patch.\n \nIt certainly looks subjectively better, and in less lines :).  That ternary\nform is already used in the file,  this patch is an opportunity to uniformize\nthat in the file.  I did this change in two commits, a preparatory one with the\nswitch to the ternary op and other changes, and then the rewording.  Finally I\nsquased into one.  I can go back to the two patches and/or revert to the\nif/else, but my preference is to keep the ternary.\n\nIn fact, I missed there the removal of two '.',  I'll fix that.\n\n>> @@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>>  \tconst char *interpreted_newname = NULL;\n>>  \tint recovery = 0;\n>>  \n>> -\tif (!oldname) {\n>> -\t\tif (copy)\n>> -\t\t\tdie(_(\"cannot copy the current branch while not on any.\"));\n>> -\t\telse\n>> -\t\t\tdie(_(\"cannot rename the current branch while not on any.\"));\n>> -\t}\n>> -\n> \n> Something like\n> \n>     copy_or_rename_branch() never gets NULL in oldname, because its\n>     only callers in cmd_branch() calls it with either the end-user's\n>     command line argument or the result of resolve_refdup(\"HEAD\"),\n>     and neither can ever be NULL because ...\n> \n> needs to go into the proposed log message to explain why it is safe\n> to remove these two messages.\n> \n> Having said that, the messages may actually be correct and it is the\n> logic that makes it unreachable that is wrong in this case, I think.\n> It looks like the code expects that oldname is NULL when we are on\n> detached HEAD (it could be the old caller did have NULL in head when\n> this code was written, and it is possible that we regressed over the\n> course of history).  I.e. Isn't it trying to protect us against\n> \n>    $ git checkout --detach HEAD^0\n>    $ git branch -m foo\t\t;# rename .git/HEAD to .git/foo???\n> \n> (or \"-c\" for copy)?  The current code happens to catch it a bit\n> later in this function, when it notices \"HEAD\" is not a refname in\n> \"refs/heads/\" hierarchy, but the user never attempted to rename\n> refs/heads/HEAD to refs/heads/foo, and \"Invalid branch name: 'HEAD'\"\n> is us being wrong, insulting and confusing to the users.\n> \n> I suspect that this needs to be fixed at the caller's level, just\n> like the caller reacts to \"edit_description\" by checking the\n> filter.detached bit and rejects the request, the caller should\n> notice that we are on a detached HEAD and die with one of the above\n> two messages without calling this function.\n> \n> And that should be a separate patch, that can be reviewed and\n> applied regardless of the rest of \"error messages cleanup\" topic.\n\nGood point.  I didn't think about that and it also goes in the line of\nthe previous patches in this file.  I'll review that.  Also gives a good\nopportunity to fix that repeated code /... if (copy) ... else if\n(rename)/.\n\n> The hunks before this point that I omitted from quoting and\n> commenting looked all OK.\n> \n> The remainder of the patch I didn't look at.  There may be similar\n> issues like this \"oops, the messages are dead for a wrong reason and\n> code needs to be fixed\" or \"let's leave subjective improvements out\n> of this topic that has clearly described rules\" to be discovered, so\n> I may come back to them later, or others may find them before I do.\n\nI'll prepare a reroll.\n\nThank you for you review.\n"},{"id":"465728","messageId":"xmqqzgdjn1ne.fsf@gitster.g","threadId":"58679","inReplyTo":"faf7a985-f6ef-f20a-3857-031396124d60@gmail.com","subject":"Re: [PATCH] branch: error and informative messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-25T19:28:37Z","receivedAt":"2022-10-25T19:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>>> \t- \"no such branch '%s'\"\n>>> \t- \"branch '%s' does not exist\"\n>>> \t- \"invalid branch name: '%s'\"\n>>> \t+ \"no branch named '%s'\"\n>\n> Yes, I had doubts with the third.  The error is referring to the source branch\n> in the copy/rename operation, so I think it makes sense to say that the branch\n> doesn't exist, even if it couldn't.\n\nOK.  As long as it refers to a branch that ought to exist, then\nusing the fourth one is perfectly fine.\n\n> I prefer a single term like 'modify' as I find it less confuse than 'set or\n> unset'.\n\nOK.\n\nSome folks find it unsure and confusing if 'modification' includes\n'deleting/unsetting', and that was why I brought up 'set or unset'.\n\n>>>  - \"%s\" and \"'%s'\" was used to format a branch name in different\n>>>    messages.  \"'%s'\" has been used to normalize as it's the more\n>>>    frequently used in this file and very common in the rest of the\n>>>    codebase.  The opposite has been done for options: \"-a\" used vs\n>>>    \"'-a'\".\n> ...\n> Same reasoning as above.  It is a system-chosen term, but the message\n> has not a placeholder to put a value, we're using a literal.\n\nI doubt that \"same reasoning\" is sensible. I'll welcome input from\nothers, but \n\n    $ git grep '\"[^\"'\\'']*'\\''--[a-z]' \\*.c\n\nlooked very reasonable, and after imagining the output with them\nlosing the single quote around the option name, I would think they\nare better with the quotes around them.\n\n>>> Finally, let's change the return code on error for --edit-description,\n>>> from -1 to 1.\n>> \n>> OK.  That last one may be better to be a separate patch, as these\n>> wording changes are subject to discussion and bikeshedding.\n>\n> Mmm, I thought about that.  This change is one that we've been delaying because\n> it might break something due to a change in the way we report errors.  We're\n> specifically changing this here and the change is small, so I found appropriate\n> to do it here.\n\nNot really.  Nobody reads error messages, but programs can react to\nexit codes.  It is more important to get the latter right.\n\n>> This does not fall into any of the categories the proposed log\n>> message discussed.  Rather, it looks more like \"the code\n>> subjectively looks better this way\".  It happens to much my\n>> subjective taste, but that does not change the fact that we\n>> shouldn't distract reviewers with such an unrelated change in the\n>> same patch.\n>  \n> It certainly looks subjectively better, and in less lines...\n\nAs I said, it does not matter.  It is outside the scope of \"improve\nerror messages\" and should be done outside the series, or at lesat\nas a separate step in the series.\n\n>> And that should be a separate patch, that can be reviewed and\n>> applied regardless of the rest of \"error messages cleanup\" topic.\n>\n> Good point.  I didn't think about that and it also goes in the line of\n> the previous patches in this file.  I'll review that.  Also gives a good\n> opportunity to fix that repeated code /... if (copy) ... else if\n> (rename)/.\n\nOK.  But again, that is outside the topic of \"improve error\nmessages\".\n\nThanks.\n\n"},{"id":"465745","messageId":"9ceaa13d-5ab3-a36f-b03d-003dbaf3f112@gmail.com","threadId":"58679","inReplyTo":"xmqqzgdjn1ne.fsf@gitster.g","subject":"Re: [PATCH] branch: error and informative messages","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-25T23:52:02Z","receivedAt":"2022-10-25T23:52:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 25/10/22 21:28, Junio C Hamano wrote:\n\n>>>>  - \"%s\" and \"'%s'\" was used to format a branch name in different\n>>>>    messages.  \"'%s'\" has been used to normalize as it's the more\n>>>>    frequently used in this file and very common in the rest of the\n>>>>    codebase.  The opposite has been done for options: \"-a\" used vs\n>>>>    \"'-a'\".\n>> ...\n>> Same reasoning as above.  It is a system-chosen term, but the message\n>> has not a placeholder to put a value, we're using a literal.\n> \n> I doubt that \"same reasoning\" is sensible. I'll welcome input from\n> others, but \n> \n>     $ git grep '\"[^\"'\\'']*'\\''--[a-z]' \\*.c\n> \n> looked very reasonable, and after imagining the output with them\n> losing the single quote around the option name, I would think they\n> are better with the quotes around them.\n\nThe reasoning I do is:\n- an option is a literal, cannot change in the message, with unexpected chars\n  for example\n- this makes it is well delimited.\n- the dashes clearly differentiate from a simple word\n- we do not use quotes for options other in places, like the documentation\n\nBut looks like we have a mix:\n\n$ git grep '\\(die\\|error\\|warning\\).*\"[^\"'\\'']*--[a-z]' \\*.c | head -12\n\napply.c:                return error(_(\"options '%s' and '%s' cannot be used together\"), \"--reject\", \"--3way\");\napply.c:                return error(_(\"'%s' outside a repository\"), \"--index\");\napply.c:                        return error(_(\"'%s' outside a repository\"), \"--cached\");\napply.c:                        error(_(\"No valid patches in input (allow with \\\"--allow-empty\\\")\"));\narchive.c:              die(_(\"Unexpected option --remote\"));\narchive.c:              die(_(\"the option '%s' requires '%s'\"), \"--exec\", \"--remote\");\narchive.c:              die(_(\"Unexpected option --output\"));\narchive.c:              die(_(\"options '%s' and '%s' cannot be used together\"), \"--add-file\", \"--remote\");\nblame.c:                die(_(\"--contents and --reverse do not blend well.\"));\nblame.c:                die(_(\"cannot use --contents with final commit object name\"));\nblame.c:                        die(_(\"--reverse and --first-parent together require specified latest commit\"));\nblame.c:                        die(_(\"--reverse --first-parent together require range along first-parent chain\"));\n\nI prefer the unquoted form, because of the previous reasons.  But I don't\nhave a strong opinion on that, beyond using the same criteria in the\nfile :-). \n\n>>>> Finally, let's change the return code on error for --edit-description,\n>>>> from -1 to 1.\n>>>\n>>> OK.  That last one may be better to be a separate patch, as these\n>>> wording changes are subject to discussion and bikeshedding.\n>>\n>> Mmm, I thought about that.  This change is one that we've been delaying because\n>> it might break something due to a change in the way we report errors.  We're\n>> specifically changing this here and the change is small, so I found appropriate\n>> to do it here.\n> \n> Not really.  Nobody reads error messages, but programs can react to\n> exit codes.  It is more important to get the latter right.\n\nOK. I just sent a patch for this.\n\n>>> This does not fall into any of the categories the proposed log\n>>> message discussed.  Rather, it looks more like \"the code\n>>> subjectively looks better this way\".  It happens to much my\n>>> subjective taste, but that does not change the fact that we\n>>> shouldn't distract reviewers with such an unrelated change in the\n>>> same patch.\n>>  \n>> It certainly looks subjectively better, and in less lines...\n> \n> As I said, it does not matter.  It is outside the scope of \"improve\n> error messages\" and should be done outside the series, or at lesat\n> as a separate step in the series.\n\nOK. I will separate this in a preparatory step.\n\n>>> And that should be a separate patch, that can be reviewed and\n>>> applied regardless of the rest of \"error messages cleanup\" topic.\n>>\n>> Good point.  I didn't think about that and it also goes in the line of\n>> the previous patches in this file.  I'll review that.  Also gives a good\n>> opportunity to fix that repeated code /... if (copy) ... else if\n>> (rename)/.\n> \n> OK.  But again, that is outside the topic of \"improve error\n> messages\".\n\nOK. I just sent a patch for this.\n\n\nI'll wait a few days before sending a new version, to give others time\nto comment.\n\nThanks.\n"},{"id":"466636","messageId":"eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com","threadId":"58679","inReplyTo":"3f63d53d-2e14-ffe0-6263-2a15f83453ad@gmail.com","subject":"[PATCH v2] branch: error and informative messages","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-11-06T21:51:53Z","receivedAt":"2022-11-06T21:52:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Review die(), error() and warning() messages used in branch, following\nthe guidelines we have in Documentation/CodingGuideLines.\n\nMainly:\n\tError messages\n \t - Do not end error messages with a full stop.\n \t - Do not capitalize the first word, ...\n\n\"%s\" and \"'%s'\" was used to format a branch name in different messages.\n\"'%s'\" has been used to normalize as it's the more frequently used in\nthis file and very common in the rest of the codebase.\n\nThe tests affected by the modified messages have been updated.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nFrom v1, many changes, non substantial for the objective of the patch,\nhas been left out.\n\nThis is a minor patch, but I don't want to leave it behind as I have\nbeen delaying this in my previous patches for branch.c, so as not to mix\nthose changes with this, but this needs to be done.\n\nSome future work might be done, maybe after some new guidelines be\ndefined.  I leave here some quick notes related that might be useful to\nthat possible discussion:\n\t- quoting related to values: branches, refs, hashes, ...\n\t- quoting related to literals: HEAD, options, commands, config, ...\n\t- informative messages going to stdin or stderr\n\t- capitalize informative or advice messages\n\t- full stop on informative messages \n\t- pool of frequent common errors (ie: no branch named...)\n\n\n--word-diff can be useful to review this patch.\n\nUn saludo.\n\n builtin/branch.c          | 71 ++++++++++++++++++++-------------------\n t/t2407-worktree-heads.sh |  2 +-\n t/t3200-branch.sh         | 12 +++----\n t/t3202-show-branch.sh    |  8 ++---\n 4 files changed, 47 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 15be0c03ef..abef1e1a66 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -163,11 +163,11 @@ static int branch_merged(int kind, const char *name,\n \t    in_merge_bases(rev, head_rev) != merged) {\n \t\tif (merged)\n \t\t\twarning(_(\"deleting branch '%s' that has been merged to\\n\"\n-\t\t\t\t\"         '%s', but not yet merged to HEAD.\"),\n+\t\t\t\t\"         '%s', but not yet merged to HEAD\"),\n \t\t\t\tname, reference_name);\n \t\telse\n \t\t\twarning(_(\"not deleting branch '%s' that is not yet merged to\\n\"\n-\t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n+\t\t\t\t\"         '%s', even though it is merged to HEAD\"),\n \t\t\t\tname, reference_name);\n \t}\n \tfree(reference_name_to_free);\n@@ -180,13 +180,13 @@ static int check_branch_commit(const char *branchname, const char *refname,\n {\n \tstruct commit *rev = lookup_commit_reference(the_repository, oid);\n \tif (!force && !rev) {\n-\t\terror(_(\"Couldn't look up commit object for '%s'\"), refname);\n+\t\terror(_(\"couldn't look up commit object for '%s'\"), refname);\n \t\treturn -1;\n \t}\n \tif (!force && !branch_merged(kinds, branchname, rev, head_rev)) {\n-\t\terror(_(\"The branch '%s' is not fully merged.\\n\"\n+\t\terror(_(\"the branch '%s' is not fully merged\\n\"\n \t\t      \"If you are sure you want to delete it, \"\n-\t\t      \"run 'git branch -D %s'.\"), branchname, branchname);\n+\t\t      \"run 'git branch -D %s'\"), branchname, branchname);\n \t\treturn -1;\n \t}\n \treturn 0;\n@@ -197,7 +197,7 @@ static void delete_branch_config(const char *branchname)\n \tstruct strbuf buf = STRBUF_INIT;\n \tstrbuf_addf(&buf, \"branch.%s\", branchname);\n \tif (git_config_rename_section(buf.buf, NULL) < 0)\n-\t\twarning(_(\"Update of config-file failed\"));\n+\t\twarning(_(\"update of config-file failed\"));\n \tstrbuf_release(&buf);\n }\n \n@@ -238,7 +238,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tif (!force) {\n \t\thead_rev = lookup_commit_reference(the_repository, &head_oid);\n \t\tif (!head_rev)\n-\t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n+\t\t\tdie(_(\"couldn't look up commit object for HEAD\"));\n \t}\n \n \tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (kinds == FILTER_REFS_BRANCHES) {\n \t\t\tconst char *path;\n \t\t\tif ((path = branch_checked_out(name))) {\n-\t\t\t\terror(_(\"Cannot delete branch '%s' \"\n+\t\t\t\terror(_(\"cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, path);\n \t\t\t\tret = 1;\n@@ -267,8 +267,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t&oid, &flags);\n \t\tif (!target) {\n \t\t\terror(remote_branch\n-\t\t\t      ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t      : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\t      ? _(\"remote-tracking branch '%s' not found\")\n+\t\t\t      : _(\"branch '%s' not found\"), bname.buf);\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n@@ -501,11 +501,11 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \t\t\tcontinue;\n \n \t\tif (is_worktree_being_rebased(wt, target))\n-\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n+\t\t\tdie(_(\"branch '%s' is being rebased at %s\"),\n \t\t\t    target, wt->path);\n \n \t\tif (is_worktree_being_bisected(wt, target))\n-\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n+\t\t\tdie(_(\"branch '%s' is being bisected at %s\"),\n \t\t\t    target, wt->path);\n \t}\n \n@@ -528,14 +528,14 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tif (ref_exists(oldref.buf))\n \t\t\trecovery = 1;\n \t\telse\n-\t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n+\t\t\tdie(_(\"invalid branch name: '%s'\"), oldname);\n \t}\n \n \tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n \t\tif (copy && !strcmp(head, oldname))\n-\t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n+\t\t\tdie(_(\"no commit on branch '%s' yet\"), oldname);\n \t\telse\n-\t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n+\t\t\tdie(_(\"no branch named '%s'\"), oldname);\n \t}\n \n \t/*\n@@ -564,22 +564,22 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tif (!copy &&\n \t    (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n \t    rename_ref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch rename failed\"));\n+\t\tdie(_(\"branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch copy failed\"));\n+\t\tdie(_(\"branch copy failed\"));\n \n \tif (recovery) {\n \t\tif (copy)\n-\t\t\twarning(_(\"Created a copy of a misnamed branch '%s'\"),\n+\t\t\twarning(_(\"created a copy of a misnamed branch '%s'\"),\n \t\t\t\tinterpreted_oldname);\n \t\telse\n-\t\t\twarning(_(\"Renamed a misnamed branch '%s' away\"),\n+\t\t\twarning(_(\"renamed a misnamed branch '%s' away\"),\n \t\t\t\tinterpreted_oldname);\n \t}\n \n \tif (!copy &&\n \t    replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n+\t\tdie(_(\"branch renamed to '%s', but HEAD is not updated\"), newname);\n \n \tstrbuf_release(&logmsg);\n \n@@ -588,9 +588,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstrbuf_addf(&newsection, \"branch.%s\", interpreted_newname);\n \tstrbuf_release(&newref);\n \tif (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)\n-\t\tdie(_(\"Branch is renamed, but update of config-file failed\"));\n+\t\tdie(_(\"branch is renamed, but update of config-file failed\"));\n \tif (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)\n-\t\tdie(_(\"Branch is copied, but update of config-file failed\"));\n+\t\tdie(_(\"branch is copied, but update of config-file failed\"));\n \tstrbuf_release(&oldsection);\n \tstrbuf_release(&newsection);\n }\n@@ -708,11 +708,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \thead = resolve_refdup(\"HEAD\", 0, &head_oid, NULL);\n \tif (!head)\n-\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\t\tdie(_(\"failed to resolve HEAD as a valid ref\"));\n \tif (!strcmp(head, \"HEAD\"))\n \t\tfilter.detached = 1;\n \telse if (!skip_prefix(head, \"refs/heads/\", &head))\n-\t\tdie(_(\"HEAD not found below refs/heads!\"));\n+\t\tdie(_(\"HEAD not found below refs/heads\"));\n \n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t     0);\n@@ -798,7 +798,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n-\t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n+\t\t\t\tdie(_(\"cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n \t\t} else if (argc == 1) {\n \t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n@@ -810,8 +810,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n \t\t\terror((!argc || !strcmp(head, branch_name))\n-\t\t\t      ? _(\"No commit on branch '%s' yet.\")\n-\t\t\t      : _(\"No branch named '%s'.\"),\n+\t\t\t      ? _(\"no commit on branch '%s' yet\")\n+\t\t\t      : _(\"no branch named '%s'\"),\n \t\t\t      branch_name);\n \t\telse if (!edit_branch_description(branch_name))\n \t\t\tret = 0; /* happy */\n@@ -824,8 +824,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n \t\telse if ((argc == 1) && filter.detached)\n-\t\t\tdie(copy? _(\"cannot copy the current branch while not on any.\")\n-\t\t\t\t: _(\"cannot rename the current branch while not on any.\"));\n+\t\t\tdie(copy? _(\"cannot copy the current branch while not on any\")\n+\t\t\t\t: _(\"cannot rename the current branch while not on any\"));\n \t\telse if (argc == 1)\n \t\t\tcopy_or_rename_branch(head, argv[0], copy, copy + rename > 1);\n \t\telse if (argc == 2)\n@@ -848,14 +848,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch) {\n \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n \t\t\t\tdie(_(\"could not set upstream of HEAD to %s when \"\n-\t\t\t\t      \"it does not point to any branch.\"),\n+\t\t\t\t      \"it does not point to any branch\"),\n \t\t\t\t    new_upstream);\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \t\t}\n \n \t\tif (!ref_exists(branch->refname)) {\n \t\t\tif (!argc || !strcmp(head, branch->name))\n-\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n+\t\t\t\tdie(_(\"no commit on branch '%s' yet\"), branch->name);\n \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n \t\t}\n \n@@ -878,12 +878,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch) {\n \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n \t\t\t\tdie(_(\"could not unset upstream of HEAD when \"\n-\t\t\t\t      \"it does not point to any branch.\"));\n+\t\t\t\t      \"it does not point to any branch\"));\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \t\t}\n \n \t\tif (!branch_has_merge_config(branch))\n-\t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n+\t\t\tdie(_(\"branch '%s' has no upstream information\"), branch->name);\n \n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n@@ -897,11 +897,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tconst char *start_name = argc == 2 ? argv[1] : head;\n \n \t\tif (filter.kind != FILTER_REFS_BRANCHES)\n-\t\t\tdie(_(\"The -a, and -r, options to 'git branch' do not take a branch name.\\n\"\n+\t\t\tdie(_(\"the -a, and -r, options to 'git branch' do not take a branch name\\n\"\n \t\t\t\t  \"Did you mean to use: -a|-r --list <pattern>?\"));\n \n \t\tif (track == BRANCH_TRACK_OVERRIDE)\n-\t\t\tdie(_(\"the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.\"));\n+\t\t\tdie(_(\"the '--set-upstream' option is no longer supported\\n\"\n+\t\t\t\t  \"Please use '--track' or '--set-upstream-to' instead\"));\n \n \t\tif (recurse_submodules) {\n \t\t\tcreate_branches_recursively(the_repository, branch_name,\ndiff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh\nindex 019a40df2c..b0992d5f91 100755\n--- a/t/t2407-worktree-heads.sh\n+++ b/t/t2407-worktree-heads.sh\n@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '\n \t\tgrep \"cannot force update the branch\" err &&\n \n \t\ttest_must_fail git branch -D wt-$i 2>err &&\n-\t\tgrep \"Cannot delete branch\" err || return 1\n+\t\tgrep \"cannot delete branch\" err || return 1\n \tdone\n '\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7f605f865b..8cfdc14b99 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -271,10 +271,10 @@ test_expect_success 'git branch -M topic topic should work when main is checked\n test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \tgit checkout HEAD^{} &&\n \ttest_when_finished git checkout - &&\n-\techo \"fatal: cannot rename the current branch while not on any.\" >expect &&\n+\techo \"fatal: cannot rename the current branch while not on any\" >expect &&\n \ttest_must_fail git branch -M must-fail 2>err &&\n \ttest_cmp expect err &&\n-\techo \"fatal: cannot copy the current branch while not on any.\" >expect &&\n+\techo \"fatal: cannot copy the current branch while not on any\" >expect &&\n \ttest_must_fail git branch -C must-fail 2>err &&\n \ttest_cmp expect err\n '\n@@ -942,7 +942,7 @@ test_expect_success '--set-upstream-to fails on multiple branches' '\n test_expect_success '--set-upstream-to fails on detached HEAD' '\n \tgit checkout HEAD^{} &&\n \ttest_when_finished git checkout - &&\n-\techo \"fatal: could not set upstream of HEAD to main when it does not point to any branch.\" >expect &&\n+\techo \"fatal: could not set upstream of HEAD to main when it does not point to any branch\" >expect &&\n \ttest_must_fail git branch --set-upstream-to main 2>err &&\n \ttest_cmp expect err\n '\n@@ -990,7 +990,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '\n '\n \n test_expect_success '--unset-upstream should fail if given a non-existent branch' '\n-\techo \"fatal: Branch '\"'\"'i-dont-exist'\"'\"' has no upstream information\" >expect &&\n+\techo \"fatal: branch '\"'\"'i-dont-exist'\"'\"' has no upstream information\" >expect &&\n \ttest_must_fail git branch --unset-upstream i-dont-exist 2>err &&\n \ttest_cmp expect err\n '\n@@ -1012,7 +1012,7 @@ test_expect_success 'test --unset-upstream on HEAD' '\n \ttest_must_fail git config branch.main.remote &&\n \ttest_must_fail git config branch.main.merge &&\n \t# fail for a branch without upstream set\n-\techo \"fatal: Branch '\"'\"'main'\"'\"' has no upstream information\" >expect &&\n+\techo \"fatal: branch '\"'\"'main'\"'\"' has no upstream information\" >expect &&\n \ttest_must_fail git branch --unset-upstream 2>err &&\n \ttest_cmp expect err\n '\n@@ -1026,7 +1026,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '\n test_expect_success '--unset-upstream should fail on detached HEAD' '\n \tgit checkout HEAD^{} &&\n \ttest_when_finished git checkout - &&\n-\techo \"fatal: could not unset upstream of HEAD when it does not point to any branch.\" >expect &&\n+\techo \"fatal: could not unset upstream of HEAD when it does not point to any branch\" >expect &&\n \ttest_must_fail git branch --unset-upstream 2>err &&\n \ttest_cmp expect err\n '\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..23cf87e550 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n test_expect_success 'error descriptions on empty repository' '\n \tcurrent=$(git branch --show-current) &&\n \tcat >expect <<-EOF &&\n-\terror: No commit on branch '\\''$current'\\'' yet.\n+\terror: no commit on branch '\\''$current'\\'' yet\n \tEOF\n \ttest_must_fail git branch --edit-description 2>actual &&\n \ttest_cmp expect actual &&\n@@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty repository' '\n test_expect_success 'fatal descriptions on empty repository' '\n \tcurrent=$(git branch --show-current) &&\n \tcat >expect <<-EOF &&\n-\tfatal: No commit on branch '\\''$current'\\'' yet.\n+\tfatal: no commit on branch '\\''$current'\\'' yet\n \tEOF\n \ttest_must_fail git branch --set-upstream-to=non-existent 2>actual &&\n \ttest_cmp expect actual &&\n@@ -199,7 +199,7 @@ EOF\n \n test_expect_success 'error descriptions on non-existent branch' '\n \tcat >expect <<-EOF &&\n-\terror: No branch named '\\''non-existent'\\'.'\n+\terror: no branch named '\\''non-existent'\\''\n \tEOF\n \ttest_must_fail git branch --edit-description non-existent 2>actual &&\n \ttest_cmp expect actual\n@@ -213,7 +213,7 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual &&\n \n \tcat >expect <<-EOF &&\n-\tfatal: No branch named '\\''non-existent'\\''.\n+\tfatal: no branch named '\\''non-existent'\\''\n \tEOF\n \ttest_must_fail git branch -c non-existent new-branch 2>actual &&\n \ttest_cmp expect actual &&\n-- \n2.38.1.432.ga6559ae16c\n"}]}