* [PATCH] branch: error and informative messages
@ 2022-10-23 22:29 Rubén Justo
2022-10-24 0:20 ` Junio C Hamano
2022-11-06 21:51 ` [PATCH v2] " Rubén Justo
0 siblings, 2 replies; 6+ messages in thread
From: Rubén Justo @ 2022-10-23 22:29 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Review die(), error(), warning() and output messages used in branch,
following the guidelines we have in Documentation/CodingGuideLines.
Mainly:
Error messages
- Do not end error messages with a full stop.
- Do not capitalize the first word, ...
Beside applying this rules, some unneeded messages have been removed and
others reworded to normalize and simplify.
- Four similar messages has been merged into the most common, the last
one:
- "no such branch '%s'"
- "branch '%s' does not exist"
- "invalid branch name: '%s'"
+ "no branch named '%s'"
- Four specific messages has been reworded and merged:
- "too many branches for a copy operation"
- "too many arguments for a rename operation"
- "too many arguments to set new upstream"
- "too many arguments to unset upstream"
+ "too many arguments"
- Two specific messages has been reworded and merged:
- "could not set upstream of HEAD to %s when
it does not point to any branch."
- "could not unset upstream of HEAD when it
does not point to any branch."
+ "cannot modify upstream to detached HEAD"
- An error message reworded
- "options '%s' and '%s' cannot be used together",
"--column", "--verbose"
+ "options --column and --verbose used together"
- An error message reworded
- "The -a, and -r, options to 'git branch' do not take a branch name.\n"
"Did you mean to use: -a|-r --list <pattern>?"));
+ "options -a and -r do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
- Two error messages not needed, removed:
- "cannot copy the current branch while not on any."
- "cannot rename the current branch while not on any."
- A deprecation message has been reworded, note the '\n':
- "the '--set-upstream' option is no longer supported."
"Please use '--track' or '--set-upstream-to' instead."
+ "option --set-upstream is no longer supported\n"
"Please use --track or --set-upstream-to instead"
- "%s" and "'%s'" was used to format a branch name in different
messages. "'%s'" has been used to normalize as it's the more
frequently used in this file and very common in the rest of the
codebase. The opposite has been done for options: "-a" used vs
"'-a'".
A new informative, non-error, message has been introduced for
create_branch:
+ "New branch '%s' created from '%s'\n"
The tests for the modified messages have been updated and a new test for
the create_branch message has been added.
Finally, let's change the return code on error for --edit-description,
from -1 to 1.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
builtin/branch.c | 163 ++++++++++++++++----------------------
t/t1430-bad-ref-name.sh | 10 +--
t/t2407-worktree-heads.sh | 2 +-
t/t3200-branch.sh | 27 ++++---
t/t3202-show-branch.sh | 25 ++----
5 files changed, 96 insertions(+), 131 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 407517ba68..ca472436f0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -159,17 +159,13 @@ static int branch_merged(int kind, const char *name,
* any of the following code, but during the transition period,
* a gentle reminder is in order.
*/
- if ((head_rev != reference_rev) &&
- in_merge_bases(rev, head_rev) != merged) {
- if (merged)
- warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
- name, reference_name);
- else
- warning(_("not deleting branch '%s' that is not yet merged to\n"
+ if ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)
+ warning(merged
+ ? _("deleting branch '%s' that has been merged to\n"
+ " '%s', but not yet merged to HEAD.")
+ : _("not deleting branch '%s' that is not yet merged to\n"
" '%s', even though it is merged to HEAD."),
name, reference_name);
- }
free(reference_name_to_free);
return merged;
}
@@ -179,16 +175,12 @@ static int check_branch_commit(const char *branchname, const char *refname,
int kinds, int force)
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
- if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
- return -1;
- }
- if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ if (!force && !rev)
+ return error(_("couldn't look up commit object for '%s'"), refname);
+ if (!force && !branch_merged(kinds, branchname, rev, head_rev))
+ return error(_("the branch '%s' is not fully merged\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
- return -1;
- }
+ "run 'git branch -D %s'"), branchname, branchname);
return 0;
}
@@ -197,7 +189,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -238,7 +230,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (!force) {
head_rev = lookup_commit_reference(the_repository, &head_oid);
if (!head_rev)
- die(_("Couldn't look up commit object for HEAD"));
+ die(_("couldn't look up commit object for HEAD"));
}
for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
@@ -252,7 +244,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"checked out at '%s'"),
bname.buf, path);
ret = 1;
@@ -267,8 +259,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
error(remote_branch
- ? _("remote-tracking branch '%s' not found.")
- : _("branch '%s' not found."), bname.buf);
+ ? _("remote-tracking branch '%s' not found")
+ : _("branch '%s' not found"), bname.buf);
ret = 1;
continue;
}
@@ -299,8 +291,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
char *refname = name + branch_name_pos;
if (!quiet)
printf(remote_branch
- ? _("Deleted remote-tracking branch %s (was %s).\n")
- : _("Deleted branch %s (was %s).\n"),
+ ? _("Deleted remote-tracking branch '%s' (was %s)\n")
+ : _("Deleted branch '%s' (was %s)\n"),
name + branch_name_pos, describe_ref);
delete_branch_config(refname);
@@ -501,11 +493,11 @@ static void reject_rebase_or_bisect_branch(const char *target)
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch '%s' is being rebased at '%s'"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch '%s' is being bisected at '%s'"),
target, wt->path);
}
@@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
const char *interpreted_newname = NULL;
int recovery = 0;
- if (!oldname) {
- if (copy)
- die(_("cannot copy the current branch while not on any."));
- else
- die(_("cannot rename the current branch while not on any."));
- }
-
if (strbuf_check_branch_ref(&oldref, oldname)) {
/*
* Bad name --- this could be an attempt to rename a
@@ -535,15 +520,13 @@ 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);
+ die(_("no branch named '%s'"), oldname);
}
- if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
- if (copy && !strcmp(head, oldname))
- die(_("No commit on branch '%s' yet."), oldname);
- else
- die(_("No branch named '%s'."), oldname);
- }
+ if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf))
+ die(copy && !strcmp(head, oldname)
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"), oldname);
/*
* A command like "git branch -M currentbranch currentbranch" cannot
@@ -561,32 +544,25 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
BUG("expected prefix missing for refs");
}
- if (copy)
- strbuf_addf(&logmsg, "Branch: copied %s to %s",
- oldref.buf, newref.buf);
- else
- strbuf_addf(&logmsg, "Branch: renamed %s to %s",
- oldref.buf, newref.buf);
+ strbuf_addf(&logmsg, copy
+ ? "Branch: copied %s to %s"
+ : "Branch: renamed %s to %s", oldref.buf, newref.buf);
if (!copy &&
(!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
- if (recovery) {
- if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ if (recovery)
+ warning(copy ? _("created a copy of a misnamed branch '%s'")
+ : _("renamed a misnamed branch '%s' away"),
interpreted_oldname);
- else
- warning(_("Renamed a misnamed branch '%s' away"),
- interpreted_oldname);
- }
if (!copy &&
replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to '%s', but HEAD is not updated"), newname);
strbuf_release(&logmsg);
@@ -595,9 +571,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
strbuf_release(&newref);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldsection);
strbuf_release(&newsection);
}
@@ -715,11 +691,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
- die(_("HEAD not found below refs/heads!"));
+ die(_("HEAD not found below refs/heads"));
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
0);
@@ -740,9 +716,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (recurse_submodules_explicit) {
if (!submodule_propagate_branches)
- die(_("branch with --recurse-submodules can only be used if submodule.propagateBranches is enabled"));
+ die(_("branch with --recurse-submodules can only be "
+ "used if submodule.propagateBranches is enabled"));
if (noncreate_actions)
- die(_("--recurse-submodules can only be used to create branches"));
+ die(_("--recurse-submodules can only be used to "
+ "create branches"));
}
recurse_submodules =
@@ -756,7 +734,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
finalize_colopts(&colopts, -1);
if (filter.verbose) {
if (explicitly_enable_column(colopts))
- die(_("options '%s' and '%s' cannot be used together"), "--column", "--verbose");
+ die(_("options --column and --verbose used together"));
colopts = 0;
}
@@ -805,7 +783,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -816,9 +794,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
- ret = error((!argc || !strcmp(head, branch_name))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ error((!argc || !strcmp(head, branch_name))
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -835,7 +813,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
else if (argc == 2)
copy_or_rename_branch(argv[0], argv[1], 1, copy > 1);
else
- die(_("too many branches for a copy operation"));
+ die(_("too many arguments"));
} else if (rename) {
if (!argc)
die(_("branch name required"));
@@ -844,7 +822,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
else if (argc == 2)
copy_or_rename_branch(argv[0], argv[1], 0, rename > 1);
else
- die(_("too many arguments for a rename operation"));
+ die(_("too many arguments"));
} else if (new_upstream) {
struct branch *branch;
struct strbuf buf = STRBUF_INIT;
@@ -855,21 +833,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
branch = branch_get(buf.buf);
} else
- die(_("too many arguments to set new upstream"));
+ die(_("too many arguments"));
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
- die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
- new_upstream);
- die(_("no such branch '%s'"), argv[0]);
+ die(_("cannot modify upstream to detached HEAD"));
+ die(_("no branch named '%s'"), argv[0]);
}
- if (!ref_exists(branch->refname)) {
- if (!argc || !strcmp(head, branch->name))
- die(_("No commit on branch '%s' yet."), branch->name);
- die(_("branch '%s' does not exist"), branch->name);
- }
+ if (!ref_exists(branch->refname))
+ die(!argc || !strcmp(head, branch->name)
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"), branch->name);
dwim_and_setup_tracking(the_repository, branch->name,
new_upstream, BRANCH_TRACK_OVERRIDE,
@@ -885,17 +860,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
branch = branch_get(buf.buf);
} else
- die(_("too many arguments to unset upstream"));
+ die(_("too many arguments"));
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
- die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
- die(_("no such branch '%s'"), argv[0]);
+ die(_("cannot modify upstream to detached HEAD"));
+ die(_("no branch named '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -909,20 +883,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
- "Did you mean to use: -a|-r --list <pattern>?"));
+ die(_("options -a and -r do not take a branch name\n"
+ "Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("option --set-upstream is no longer supported\n"
+ "Please use --track or --set-upstream-to instead"));
- if (recurse_submodules) {
+ if (recurse_submodules)
create_branches_recursively(the_repository, branch_name,
start_name, NULL, force,
reflog, quiet, track, 0);
- return 0;
- }
- create_branch(the_repository, branch_name, start_name, force, 0,
- reflog, quiet, track, 0);
+ else
+ create_branch(the_repository, branch_name, start_name,
+ force, 0, reflog, quiet, track, 0);
+
+ printf(_("New branch '%s' created from '%s'\n"),
+ branch_name, start_name);
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index ff1c967d55..de3249eb9d 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -192,7 +192,7 @@ test_expect_success 'branch -d can delete broken name' '
test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID REF_SKIP_REFNAME_VERIFICATION &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
git branch -d broken...ref >output 2>error &&
- test_i18ngrep "Deleted branch broken...ref (was broken)" output &&
+ test_i18ngrep "Deleted branch '"'"'broken...ref'"'"' (was broken)" output &&
test_must_be_empty error &&
git branch >output 2>error &&
! grep -e "broken\.\.\.ref" error &&
@@ -218,7 +218,7 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
git branch -d badname >output 2>error &&
test_path_is_missing .git/refs/heads/badname &&
- test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
+ test_i18ngrep "Deleted branch '"'"'badname'"'"' (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -236,7 +236,7 @@ test_expect_success 'branch -d can delete dangling symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
git branch -d badname >output 2>error &&
test_path_is_missing .git/refs/heads/badname &&
- test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
+ test_i18ngrep "Deleted branch '"'"'badname'"'"' (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -265,7 +265,7 @@ test_expect_success 'branch -d can delete symref with broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
git branch -d broken...symref >output 2>error &&
test_path_is_missing .git/refs/heads/broken...symref &&
- test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
+ test_i18ngrep "Deleted branch '"'"'broken...symref'"'"' (was refs/heads/main)" output &&
test_must_be_empty error
'
@@ -283,7 +283,7 @@ test_expect_success 'branch -d can delete dangling symref with broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
git branch -d broken...symref >output 2>error &&
test_path_is_missing .git/refs/heads/broken...symref &&
- test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
+ test_i18ngrep "Deleted branch '"'"'broken...symref'"'"' (was refs/heads/idonotexist)" output &&
test_must_be_empty error
'
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 019a40df2c..b0992d5f91 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
grep "cannot force update the branch" err &&
test_must_fail git branch -D wt-$i 2>err &&
- grep "Cannot delete branch" err || return 1
+ grep "cannot delete branch" err || return 1
done
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7d8edff9c3..326c505c29 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -39,7 +39,10 @@ test_expect_success 'branch -h in broken repository' '
'
test_expect_success 'git branch abc should create a branch' '
- git branch abc && test_path_is_file .git/refs/heads/abc
+ git branch abc >actual &&
+ echo "New branch '"'"'abc'"'"' created from '"'"'main'"'"'" >expect &&
+ test_cmp expect actual &&
+ test_path_is_file .git/refs/heads/abc
'
test_expect_success 'git branch abc should fail when abc exists' '
@@ -716,7 +719,7 @@ EOF
test_expect_success 'deleting a symref' '
git branch target &&
git symbolic-ref refs/heads/symref refs/heads/target &&
- echo "Deleted branch symref (was refs/heads/target)." >expect &&
+ echo "Deleted branch '"'"'symref'"'"' (was refs/heads/target)" >expect &&
git branch -d symref >actual &&
test_path_is_file .git/refs/heads/target &&
test_path_is_missing .git/refs/heads/symref &&
@@ -726,7 +729,7 @@ test_expect_success 'deleting a symref' '
test_expect_success 'deleting a dangling symref' '
git symbolic-ref refs/heads/dangling-symref nowhere &&
test_path_is_file .git/refs/heads/dangling-symref &&
- echo "Deleted branch dangling-symref (was nowhere)." >expect &&
+ echo "Deleted branch '"'"'dangling-symref'"'"' (was nowhere)" >expect &&
git branch -d dangling-symref >actual &&
test_path_is_missing .git/refs/heads/dangling-symref &&
test_cmp expect actual
@@ -735,7 +738,7 @@ test_expect_success 'deleting a dangling symref' '
test_expect_success 'deleting a self-referential symref' '
git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
test_path_is_file .git/refs/heads/self-reference &&
- echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
+ echo "Deleted branch '"'"'self-reference'"'"' (was refs/heads/self-reference)" >expect &&
git branch -d self-reference >actual &&
test_path_is_missing .git/refs/heads/self-reference &&
test_cmp expect actual
@@ -853,7 +856,7 @@ test_expect_success 'test deleting branch deletes branch config' '
test_expect_success 'test deleting branch without config' '
git branch my7 s &&
sha1=$(git rev-parse my7 | cut -c 1-7) &&
- echo "Deleted branch my7 (was $sha1)." >expect &&
+ echo "Deleted branch '"'"'my7'"'"' (was $sha1)" >expect &&
git branch -d my7 >actual 2>&1 &&
test_cmp expect actual
'
@@ -923,7 +926,7 @@ test_expect_success 'simple tracking skips when remote ref is not a branch' '
'
test_expect_success '--set-upstream-to fails on multiple branches' '
- echo "fatal: too many arguments to set new upstream" >expect &&
+ echo "fatal: too many arguments" >expect &&
test_must_fail git branch --set-upstream-to main a b c 2>err &&
test_cmp expect err
'
@@ -931,13 +934,13 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
test_expect_success '--set-upstream-to fails on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not set upstream of HEAD to main when it does not point to any branch." >expect &&
+ echo "fatal: cannot modify upstream to detached HEAD" >expect &&
test_must_fail git branch --set-upstream-to main 2>err &&
test_cmp expect err
'
test_expect_success '--set-upstream-to fails on a missing dst branch' '
- echo "fatal: branch '"'"'does-not-exist'"'"' does not exist" >expect &&
+ echo "fatal: no branch named '"'"'does-not-exist'"'"'" >expect &&
test_must_fail git branch --set-upstream-to main does-not-exist 2>err &&
test_cmp expect err
'
@@ -979,7 +982,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
'
test_expect_success '--unset-upstream should fail if given a non-existent branch' '
- echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
test_cmp expect err
'
@@ -1001,13 +1004,13 @@ test_expect_success 'test --unset-upstream on HEAD' '
test_must_fail git config branch.main.remote &&
test_must_fail git config branch.main.merge &&
# fail for a branch without upstream set
- echo "fatal: Branch '"'"'main'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'main'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
test_expect_success '--unset-upstream should fail on multiple branches' '
- echo "fatal: too many arguments to unset upstream" >expect &&
+ echo "fatal: too many arguments" >expect &&
test_must_fail git branch --unset-upstream a b c 2>err &&
test_cmp expect err
'
@@ -1015,7 +1018,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
test_expect_success '--unset-upstream should fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not unset upstream of HEAD when it does not point to any branch." >expect &&
+ echo "fatal: cannot modify upstream to detached HEAD" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..6b4be35e4c 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -9,9 +9,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
test_expect_success 'error descriptions on empty repository' '
current=$(git branch --show-current) &&
- cat >expect <<-EOF &&
- error: No commit on branch '\''$current'\'' yet.
- EOF
+ echo "error: no commit on branch '"'"'$current'"'"' yet" > expect &&
test_must_fail git branch --edit-description 2>actual &&
test_cmp expect actual &&
test_must_fail git branch --edit-description $current 2>actual &&
@@ -20,9 +18,7 @@ test_expect_success 'error descriptions on empty repository' '
test_expect_success 'fatal descriptions on empty repository' '
current=$(git branch --show-current) &&
- cat >expect <<-EOF &&
- fatal: No commit on branch '\''$current'\'' yet.
- EOF
+ echo "fatal: no commit on branch '"'"'$current'"'"' yet" > expect &&
test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
test_cmp expect actual &&
test_must_fail git branch -c new-branch 2>actual &&
@@ -198,23 +194,12 @@ done <<\EOF
EOF
test_expect_success 'error descriptions on non-existent branch' '
- cat >expect <<-EOF &&
- error: No branch named '\''non-existent'\'.'
- EOF
+ echo "error: no branch named '"'"'non-existent'"'"'" > expect &&
test_must_fail git branch --edit-description non-existent 2>actual &&
- test_cmp expect actual
-'
-
-test_expect_success 'fatal descriptions on non-existent branch' '
- cat >expect <<-EOF &&
- fatal: branch '\''non-existent'\'' does not exist
- EOF
+ test_cmp expect actual &&
+ echo "fatal: no branch named '"'"'non-existent'"'"'" > expect &&
test_must_fail git branch --set-upstream-to=non-existent non-existent 2>actual &&
test_cmp expect actual &&
-
- cat >expect <<-EOF &&
- fatal: No branch named '\''non-existent'\''.
- EOF
test_must_fail git branch -c non-existent new-branch 2>actual &&
test_cmp expect actual &&
test_must_fail git branch -m non-existent new-branch 2>actual &&
--
2.36.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] branch: error and informative messages
2022-10-23 22:29 [PATCH] branch: error and informative messages Rubén Justo
@ 2022-10-24 0:20 ` Junio C Hamano
2022-10-24 23:39 ` Rubén Justo
2022-11-06 21:51 ` [PATCH v2] " Rubén Justo
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-10-24 0:20 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
Rubén Justo <rjusto@gmail.com> writes:
> Review die(), error(), warning() and output messages used in branch,
> following the guidelines we have in Documentation/CodingGuideLines.
> Mainly:
> Error messages
>
> - Do not end error messages with a full stop.
>
> - Do not capitalize the first word, ...
OK.
>
> Beside applying this rules, some unneeded messages have been removed and
> others reworded to normalize and simplify.
>
> - Four similar messages has been merged into the most common, the last
> one:
> - "no such branch '%s'"
> - "branch '%s' does not exist"
> - "invalid branch name: '%s'"
> + "no branch named '%s'"
The third one could be more appropriate than the fourth one when the
error is about failing to create a new branch due to invalid name.
But all the other three seem to refer to a branch that ought to
exist but doesn't, and the one you picked seems reasonable one among
the three (and it is also the shortest).
> - Two specific messages has been reworded and merged:
> - "could not set upstream of HEAD to %s when
> it does not point to any branch."
> - "could not unset upstream of HEAD when it
> does not point to any branch."
> + "cannot modify upstream to detached HEAD"
OK. I have no strong opinion between "modify" and "set or unset"
but the latter may be closer to the spirit of the former. I agree
that "HEAD when it does not ..." is quite a mouthful and to those
who understand what a 'detached HEAD' is, the updated phrasing may
be easier to read.
> - An error message reworded
> - "options '%s' and '%s' cannot be used together",
> "--column", "--verbose"
> + "options --column and --verbose used together"
This is arguably a regression. The original ensures that l10n/i18n
folks cannot frob the option names that must be literally spelled,
but the "reworded" one lacks the protection (it also loses the quotes
around dashed options).
> - An error message reworded
> - "The -a, and -r, options to 'git branch' do not take a branch name.\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
> + "options -a and -r do not take a branch name\n"
> "Did you mean to use: -a|-r --list <pattern>?"));
OK.
> - Two error messages not needed, removed:
> - "cannot copy the current branch while not on any."
> - "cannot rename the current branch while not on any."
OK (assuming that these are unreachable code).
> - A deprecation message has been reworded, note the '\n':
> - "the '--set-upstream' option is no longer supported."
> "Please use '--track' or '--set-upstream-to' instead."
> + "option --set-upstream is no longer supported\n"
> "Please use --track or --set-upstream-to instead"
Loss of quotes around option names, and the definite article "the"?
> - "%s" and "'%s'" was used to format a branch name in different
> messages. "'%s'" has been used to normalize as it's the more
> frequently used in this file and very common in the rest of the
> codebase. The opposite has been done for options: "-a" used vs
> "'-a'".
As the way to display the exact literal string, I think it is good
to have quotes around both of them, the user-chosen names like
branch names, and the system-chosen names like option names.
> A new informative, non-error, message has been introduced for
> create_branch:
> + "New branch '%s' created from '%s'\n"
OK.
> The tests for the modified messages have been updated and a new test for
> the create_branch message has been added.
>
> Finally, let's change the return code on error for --edit-description,
> from -1 to 1.
OK. That last one may be better to be a separate patch, as these
wording changes are subject to discussion and bikeshedding.
> - if ((head_rev != reference_rev) &&
> - in_merge_bases(rev, head_rev) != merged) {
> - if (merged)
> - warning(_("deleting branch '%s' that has been merged to\n"
> - " '%s', but not yet merged to HEAD."),
> - name, reference_name);
> - else
> - warning(_("not deleting branch '%s' that is not yet merged to\n"
> + if ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)
> + warning(merged
> + ? _("deleting branch '%s' that has been merged to\n"
> + " '%s', but not yet merged to HEAD.")
> + : _("not deleting branch '%s' that is not yet merged to\n"
> " '%s', even though it is merged to HEAD."),
> name, reference_name);
> - }
This does not fall into any of the categories the proposed log
message discussed. Rather, it looks more like "the code
subjectively looks better this way". It happens to much my
subjective taste, but that does not change the fact that we
shouldn't distract reviewers with such an unrelated change in the
same patch.
> @@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> const char *interpreted_newname = NULL;
> int recovery = 0;
>
> - if (!oldname) {
> - if (copy)
> - die(_("cannot copy the current branch while not on any."));
> - else
> - die(_("cannot rename the current branch while not on any."));
> - }
> -
Something like
copy_or_rename_branch() never gets NULL in oldname, because its
only callers in cmd_branch() calls it with either the end-user's
command line argument or the result of resolve_refdup("HEAD"),
and neither can ever be NULL because ...
needs to go into the proposed log message to explain why it is safe
to remove these two messages.
Having said that, the messages may actually be correct and it is the
logic that makes it unreachable that is wrong in this case, I think.
It looks like the code expects that oldname is NULL when we are on
detached HEAD (it could be the old caller did have NULL in head when
this code was written, and it is possible that we regressed over the
course of history). I.e. Isn't it trying to protect us against
$ git checkout --detach HEAD^0
$ git branch -m foo ;# rename .git/HEAD to .git/foo???
(or "-c" for copy)? The current code happens to catch it a bit
later in this function, when it notices "HEAD" is not a refname in
"refs/heads/" hierarchy, but the user never attempted to rename
refs/heads/HEAD to refs/heads/foo, and "Invalid branch name: 'HEAD'"
is us being wrong, insulting and confusing to the users.
I suspect that this needs to be fixed at the caller's level, just
like the caller reacts to "edit_description" by checking the
filter.detached bit and rejects the request, the caller should
notice that we are on a detached HEAD and die with one of the above
two messages without calling this function.
And that should be a separate patch, that can be reviewed and
applied regardless of the rest of "error messages cleanup" topic.
The hunks before this point that I omitted from quoting and
commenting looked all OK.
The remainder of the patch I didn't look at. There may be similar
issues like this "oops, the messages are dead for a wrong reason and
code needs to be fixed" or "let's leave subjective improvements out
of this topic that has clearly described rules" to be discovered, so
I may come back to them later, or others may find them before I do.
Thanks for working on this one.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] branch: error and informative messages
2022-10-24 0:20 ` Junio C Hamano
@ 2022-10-24 23:39 ` Rubén Justo
2022-10-25 19:28 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Rubén Justo @ 2022-10-24 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On 24/10/22 2:20, Junio C Hamano wrote:
>> Beside applying this rules, some unneeded messages have been removed and
>> others reworded to normalize and simplify.
>>
>> - Four similar messages has been merged into the most common, the last
>> one:
>> - "no such branch '%s'"
>> - "branch '%s' does not exist"
>> - "invalid branch name: '%s'"
>> + "no branch named '%s'"
>
> The third one could be more appropriate than the fourth one when the
> error is about failing to create a new branch due to invalid name.
> But all the other three seem to refer to a branch that ought to
> exist but doesn't, and the one you picked seems reasonable one among
> the three (and it is also the shortest).
Yes, I had doubts with the third. The error is referring to the source branch
in the copy/rename operation, so I think it makes sense to say that the branch
doesn't exist, even if it couldn't.
>> - Two specific messages has been reworded and merged:
>> - "could not set upstream of HEAD to %s when
>> it does not point to any branch."
>> - "could not unset upstream of HEAD when it
>> does not point to any branch."
>> + "cannot modify upstream to detached HEAD"
>
> OK. I have no strong opinion between "modify" and "set or unset"
> but the latter may be closer to the spirit of the former. I agree
> that "HEAD when it does not ..." is quite a mouthful and to those
> who understand what a 'detached HEAD' is, the updated phrasing may
> be easier to read.
I prefer a single term like 'modify' as I find it less confuse than 'set or
unset'.
>> - An error message reworded
>> - "options '%s' and '%s' cannot be used together",
>> "--column", "--verbose"
>> + "options --column and --verbose used together"
>
> This is arguably a regression. The original ensures that l10n/i18n
> folks cannot frob the option names that must be literally spelled,
> but the "reworded" one lacks the protection (it also loses the quotes
> around dashed options).
OK. I didn't think about that, I'll revert to the parametrized form. About the
quotes, I did that on purpose. As I said in a comment below, for branches the
quotes are commonly used and make sense as it is a variable value and the
quotes helps to delimit, but with options the literal is well-known and we use
the form with dashes, ie --verbose, so it is easily identifiable. Also, we do
not use quotes with option literals in other places, like the documentation,
--help,..
>> - Two error messages not needed, removed:
>> - "cannot copy the current branch while not on any."
>> - "cannot rename the current branch while not on any."
>
> OK (assuming that these are unreachable code).
It is, you reviewed it below. I'll comment there.
>> - A deprecation message has been reworded, note the '\n':
>> - "the '--set-upstream' option is no longer supported."
>> "Please use '--track' or '--set-upstream-to' instead."
>> + "option --set-upstream is no longer supported\n"
>> "Please use --track or --set-upstream-to instead"
>
> Loss of quotes around option names, and the definite article "the"?
The loss of the article, it sounds good to me and in line with the previous
"options --column ...". Dunno.
>> - "%s" and "'%s'" was used to format a branch name in different
>> messages. "'%s'" has been used to normalize as it's the more
>> frequently used in this file and very common in the rest of the
>> codebase. The opposite has been done for options: "-a" used vs
>> "'-a'".
>
> As the way to display the exact literal string, I think it is good
> to have quotes around both of them, the user-chosen names like
> branch names, and the system-chosen names like option names.
Same reasoning as above. It is a system-chosen term, but the message
has not a placeholder to put a value, we're using a literal.
>> A new informative, non-error, message has been introduced for
>> create_branch:
>> + "New branch '%s' created from '%s'\n"
>
> OK.
>
>> The tests for the modified messages have been updated and a new test for
>> the create_branch message has been added.
>>
>> Finally, let's change the return code on error for --edit-description,
>> from -1 to 1.
>
> OK. That last one may be better to be a separate patch, as these
> wording changes are subject to discussion and bikeshedding.
>
Mmm, I thought about that. This change is one that we've been delaying because
it might break something due to a change in the way we report errors. We're
specifically changing this here and the change is small, so I found appropriate
to do it here.
>> - if ((head_rev != reference_rev) &&
>> - in_merge_bases(rev, head_rev) != merged) {
>> - if (merged)
>> - warning(_("deleting branch '%s' that has been merged to\n"
>> - " '%s', but not yet merged to HEAD."),
>> - name, reference_name);
>> - else
>> - warning(_("not deleting branch '%s' that is not yet merged to\n"
>> + if ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)
>> + warning(merged
>> + ? _("deleting branch '%s' that has been merged to\n"
>> + " '%s', but not yet merged to HEAD.")
>> + : _("not deleting branch '%s' that is not yet merged to\n"
>> " '%s', even though it is merged to HEAD."),
>> name, reference_name);
>> - }
>
> This does not fall into any of the categories the proposed log
> message discussed. Rather, it looks more like "the code
> subjectively looks better this way". It happens to much my
> subjective taste, but that does not change the fact that we
> shouldn't distract reviewers with such an unrelated change in the
> same patch.
It certainly looks subjectively better, and in less lines :). That ternary
form is already used in the file, this patch is an opportunity to uniformize
that in the file. I did this change in two commits, a preparatory one with the
switch to the ternary op and other changes, and then the rewording. Finally I
squased into one. I can go back to the two patches and/or revert to the
if/else, but my preference is to keep the ternary.
In fact, I missed there the removal of two '.', I'll fix that.
>> @@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>> const char *interpreted_newname = NULL;
>> int recovery = 0;
>>
>> - if (!oldname) {
>> - if (copy)
>> - die(_("cannot copy the current branch while not on any."));
>> - else
>> - die(_("cannot rename the current branch while not on any."));
>> - }
>> -
>
> Something like
>
> copy_or_rename_branch() never gets NULL in oldname, because its
> only callers in cmd_branch() calls it with either the end-user's
> command line argument or the result of resolve_refdup("HEAD"),
> and neither can ever be NULL because ...
>
> needs to go into the proposed log message to explain why it is safe
> to remove these two messages.
>
> Having said that, the messages may actually be correct and it is the
> logic that makes it unreachable that is wrong in this case, I think.
> It looks like the code expects that oldname is NULL when we are on
> detached HEAD (it could be the old caller did have NULL in head when
> this code was written, and it is possible that we regressed over the
> course of history). I.e. Isn't it trying to protect us against
>
> $ git checkout --detach HEAD^0
> $ git branch -m foo ;# rename .git/HEAD to .git/foo???
>
> (or "-c" for copy)? The current code happens to catch it a bit
> later in this function, when it notices "HEAD" is not a refname in
> "refs/heads/" hierarchy, but the user never attempted to rename
> refs/heads/HEAD to refs/heads/foo, and "Invalid branch name: 'HEAD'"
> is us being wrong, insulting and confusing to the users.
>
> I suspect that this needs to be fixed at the caller's level, just
> like the caller reacts to "edit_description" by checking the
> filter.detached bit and rejects the request, the caller should
> notice that we are on a detached HEAD and die with one of the above
> two messages without calling this function.
>
> And that should be a separate patch, that can be reviewed and
> applied regardless of the rest of "error messages cleanup" topic.
Good point. I didn't think about that and it also goes in the line of
the previous patches in this file. I'll review that. Also gives a good
opportunity to fix that repeated code /... if (copy) ... else if
(rename)/.
> The hunks before this point that I omitted from quoting and
> commenting looked all OK.
>
> The remainder of the patch I didn't look at. There may be similar
> issues like this "oops, the messages are dead for a wrong reason and
> code needs to be fixed" or "let's leave subjective improvements out
> of this topic that has clearly described rules" to be discovered, so
> I may come back to them later, or others may find them before I do.
I'll prepare a reroll.
Thank you for you review.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] branch: error and informative messages
2022-10-24 23:39 ` Rubén Justo
@ 2022-10-25 19:28 ` Junio C Hamano
2022-10-25 23:52 ` Rubén Justo
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-10-25 19:28 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
Rubén Justo <rjusto@gmail.com> writes:
>>> - "no such branch '%s'"
>>> - "branch '%s' does not exist"
>>> - "invalid branch name: '%s'"
>>> + "no branch named '%s'"
>
> Yes, I had doubts with the third. The error is referring to the source branch
> in the copy/rename operation, so I think it makes sense to say that the branch
> doesn't exist, even if it couldn't.
OK. As long as it refers to a branch that ought to exist, then
using the fourth one is perfectly fine.
> I prefer a single term like 'modify' as I find it less confuse than 'set or
> unset'.
OK.
Some folks find it unsure and confusing if 'modification' includes
'deleting/unsetting', and that was why I brought up 'set or unset'.
>>> - "%s" and "'%s'" was used to format a branch name in different
>>> messages. "'%s'" has been used to normalize as it's the more
>>> frequently used in this file and very common in the rest of the
>>> codebase. The opposite has been done for options: "-a" used vs
>>> "'-a'".
> ...
> Same reasoning as above. It is a system-chosen term, but the message
> has not a placeholder to put a value, we're using a literal.
I doubt that "same reasoning" is sensible. I'll welcome input from
others, but
$ git grep '"[^"'\'']*'\''--[a-z]' \*.c
looked very reasonable, and after imagining the output with them
losing the single quote around the option name, I would think they
are better with the quotes around them.
>>> Finally, let's change the return code on error for --edit-description,
>>> from -1 to 1.
>>
>> OK. That last one may be better to be a separate patch, as these
>> wording changes are subject to discussion and bikeshedding.
>
> Mmm, I thought about that. This change is one that we've been delaying because
> it might break something due to a change in the way we report errors. We're
> specifically changing this here and the change is small, so I found appropriate
> to do it here.
Not really. Nobody reads error messages, but programs can react to
exit codes. It is more important to get the latter right.
>> This does not fall into any of the categories the proposed log
>> message discussed. Rather, it looks more like "the code
>> subjectively looks better this way". It happens to much my
>> subjective taste, but that does not change the fact that we
>> shouldn't distract reviewers with such an unrelated change in the
>> same patch.
>
> It certainly looks subjectively better, and in less lines...
As I said, it does not matter. It is outside the scope of "improve
error messages" and should be done outside the series, or at lesat
as a separate step in the series.
>> And that should be a separate patch, that can be reviewed and
>> applied regardless of the rest of "error messages cleanup" topic.
>
> Good point. I didn't think about that and it also goes in the line of
> the previous patches in this file. I'll review that. Also gives a good
> opportunity to fix that repeated code /... if (copy) ... else if
> (rename)/.
OK. But again, that is outside the topic of "improve error
messages".
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] branch: error and informative messages
2022-10-25 19:28 ` Junio C Hamano
@ 2022-10-25 23:52 ` Rubén Justo
0 siblings, 0 replies; 6+ messages in thread
From: Rubén Justo @ 2022-10-25 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On 25/10/22 21:28, Junio C Hamano wrote:
>>>> - "%s" and "'%s'" was used to format a branch name in different
>>>> messages. "'%s'" has been used to normalize as it's the more
>>>> frequently used in this file and very common in the rest of the
>>>> codebase. The opposite has been done for options: "-a" used vs
>>>> "'-a'".
>> ...
>> Same reasoning as above. It is a system-chosen term, but the message
>> has not a placeholder to put a value, we're using a literal.
>
> I doubt that "same reasoning" is sensible. I'll welcome input from
> others, but
>
> $ git grep '"[^"'\'']*'\''--[a-z]' \*.c
>
> looked very reasonable, and after imagining the output with them
> losing the single quote around the option name, I would think they
> are better with the quotes around them.
The reasoning I do is:
- an option is a literal, cannot change in the message, with unexpected chars
for example
- this makes it is well delimited.
- the dashes clearly differentiate from a simple word
- we do not use quotes for options other in places, like the documentation
But looks like we have a mix:
$ git grep '\(die\|error\|warning\).*"[^"'\'']*--[a-z]' \*.c | head -12
apply.c: return error(_("options '%s' and '%s' cannot be used together"), "--reject", "--3way");
apply.c: return error(_("'%s' outside a repository"), "--index");
apply.c: return error(_("'%s' outside a repository"), "--cached");
apply.c: error(_("No valid patches in input (allow with \"--allow-empty\")"));
archive.c: die(_("Unexpected option --remote"));
archive.c: die(_("the option '%s' requires '%s'"), "--exec", "--remote");
archive.c: die(_("Unexpected option --output"));
archive.c: die(_("options '%s' and '%s' cannot be used together"), "--add-file", "--remote");
blame.c: die(_("--contents and --reverse do not blend well."));
blame.c: die(_("cannot use --contents with final commit object name"));
blame.c: die(_("--reverse and --first-parent together require specified latest commit"));
blame.c: die(_("--reverse --first-parent together require range along first-parent chain"));
I prefer the unquoted form, because of the previous reasons. But I don't
have a strong opinion on that, beyond using the same criteria in the
file :-).
>>>> Finally, let's change the return code on error for --edit-description,
>>>> from -1 to 1.
>>>
>>> OK. That last one may be better to be a separate patch, as these
>>> wording changes are subject to discussion and bikeshedding.
>>
>> Mmm, I thought about that. This change is one that we've been delaying because
>> it might break something due to a change in the way we report errors. We're
>> specifically changing this here and the change is small, so I found appropriate
>> to do it here.
>
> Not really. Nobody reads error messages, but programs can react to
> exit codes. It is more important to get the latter right.
OK. I just sent a patch for this.
>>> This does not fall into any of the categories the proposed log
>>> message discussed. Rather, it looks more like "the code
>>> subjectively looks better this way". It happens to much my
>>> subjective taste, but that does not change the fact that we
>>> shouldn't distract reviewers with such an unrelated change in the
>>> same patch.
>>
>> It certainly looks subjectively better, and in less lines...
>
> As I said, it does not matter. It is outside the scope of "improve
> error messages" and should be done outside the series, or at lesat
> as a separate step in the series.
OK. I will separate this in a preparatory step.
>>> And that should be a separate patch, that can be reviewed and
>>> applied regardless of the rest of "error messages cleanup" topic.
>>
>> Good point. I didn't think about that and it also goes in the line of
>> the previous patches in this file. I'll review that. Also gives a good
>> opportunity to fix that repeated code /... if (copy) ... else if
>> (rename)/.
>
> OK. But again, that is outside the topic of "improve error
> messages".
OK. I just sent a patch for this.
I'll wait a few days before sending a new version, to give others time
to comment.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] branch: error and informative messages
2022-10-23 22:29 [PATCH] branch: error and informative messages Rubén Justo
2022-10-24 0:20 ` Junio C Hamano
@ 2022-11-06 21:51 ` Rubén Justo
1 sibling, 0 replies; 6+ messages in thread
From: Rubén Justo @ 2022-11-06 21:51 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Review die(), error() and warning() messages used in branch, following
the guidelines we have in Documentation/CodingGuideLines.
Mainly:
Error messages
- Do not end error messages with a full stop.
- Do not capitalize the first word, ...
"%s" and "'%s'" was used to format a branch name in different messages.
"'%s'" has been used to normalize as it's the more frequently used in
this file and very common in the rest of the codebase.
The tests affected by the modified messages have been updated.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
From v1, many changes, non substantial for the objective of the patch,
has been left out.
This is a minor patch, but I don't want to leave it behind as I have
been delaying this in my previous patches for branch.c, so as not to mix
those changes with this, but this needs to be done.
Some future work might be done, maybe after some new guidelines be
defined. I leave here some quick notes related that might be useful to
that possible discussion:
- quoting related to values: branches, refs, hashes, ...
- quoting related to literals: HEAD, options, commands, config, ...
- informative messages going to stdin or stderr
- capitalize informative or advice messages
- full stop on informative messages
- pool of frequent common errors (ie: no branch named...)
--word-diff can be useful to review this patch.
Un saludo.
builtin/branch.c | 71 ++++++++++++++++++++-------------------
t/t2407-worktree-heads.sh | 2 +-
t/t3200-branch.sh | 12 +++----
t/t3202-show-branch.sh | 8 ++---
4 files changed, 47 insertions(+), 46 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 15be0c03ef..abef1e1a66 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -163,11 +163,11 @@ static int branch_merged(int kind, const char *name,
in_merge_bases(rev, head_rev) != merged) {
if (merged)
warning(_("deleting branch '%s' that has been merged to\n"
- " '%s', but not yet merged to HEAD."),
+ " '%s', but not yet merged to HEAD"),
name, reference_name);
else
warning(_("not deleting branch '%s' that is not yet merged to\n"
- " '%s', even though it is merged to HEAD."),
+ " '%s', even though it is merged to HEAD"),
name, reference_name);
}
free(reference_name_to_free);
@@ -180,13 +180,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
{
struct commit *rev = lookup_commit_reference(the_repository, oid);
if (!force && !rev) {
- error(_("Couldn't look up commit object for '%s'"), refname);
+ error(_("couldn't look up commit object for '%s'"), refname);
return -1;
}
if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
+ error(_("the branch '%s' is not fully merged\n"
"If you are sure you want to delete it, "
- "run 'git branch -D %s'."), branchname, branchname);
+ "run 'git branch -D %s'"), branchname, branchname);
return -1;
}
return 0;
@@ -197,7 +197,7 @@ static void delete_branch_config(const char *branchname)
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "branch.%s", branchname);
if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
+ warning(_("update of config-file failed"));
strbuf_release(&buf);
}
@@ -238,7 +238,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (!force) {
head_rev = lookup_commit_reference(the_repository, &head_oid);
if (!head_rev)
- die(_("Couldn't look up commit object for HEAD"));
+ die(_("couldn't look up commit object for HEAD"));
}
for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("Cannot delete branch '%s' "
+ error(_("cannot delete branch '%s' "
"checked out at '%s'"),
bname.buf, path);
ret = 1;
@@ -267,8 +267,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
&oid, &flags);
if (!target) {
error(remote_branch
- ? _("remote-tracking branch '%s' not found.")
- : _("branch '%s' not found."), bname.buf);
+ ? _("remote-tracking branch '%s' not found")
+ : _("branch '%s' not found"), bname.buf);
ret = 1;
continue;
}
@@ -501,11 +501,11 @@ static void reject_rebase_or_bisect_branch(const char *target)
continue;
if (is_worktree_being_rebased(wt, target))
- die(_("Branch %s is being rebased at %s"),
+ die(_("branch '%s' is being rebased at %s"),
target, wt->path);
if (is_worktree_being_bisected(wt, target))
- die(_("Branch %s is being bisected at %s"),
+ die(_("branch '%s' is being bisected at %s"),
target, wt->path);
}
@@ -528,14 +528,14 @@ 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);
+ die(_("invalid branch name: '%s'"), oldname);
}
if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
if (copy && !strcmp(head, oldname))
- die(_("No commit on branch '%s' yet."), oldname);
+ die(_("no commit on branch '%s' yet"), oldname);
else
- die(_("No branch named '%s'."), oldname);
+ die(_("no branch named '%s'"), oldname);
}
/*
@@ -564,22 +564,22 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
if (!copy &&
(!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch rename failed"));
+ die(_("branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch copy failed"));
+ die(_("branch copy failed"));
if (recovery) {
if (copy)
- warning(_("Created a copy of a misnamed branch '%s'"),
+ warning(_("created a copy of a misnamed branch '%s'"),
interpreted_oldname);
else
- warning(_("Renamed a misnamed branch '%s' away"),
+ warning(_("renamed a misnamed branch '%s' away"),
interpreted_oldname);
}
if (!copy &&
replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
- die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ die(_("branch renamed to '%s', but HEAD is not updated"), newname);
strbuf_release(&logmsg);
@@ -588,9 +588,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
strbuf_addf(&newsection, "branch.%s", interpreted_newname);
strbuf_release(&newref);
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is renamed, but update of config-file failed"));
+ die(_("branch is renamed, but update of config-file failed"));
if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
- die(_("Branch is copied, but update of config-file failed"));
+ die(_("branch is copied, but update of config-file failed"));
strbuf_release(&oldsection);
strbuf_release(&newsection);
}
@@ -708,11 +708,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
head = resolve_refdup("HEAD", 0, &head_oid, NULL);
if (!head)
- die(_("Failed to resolve HEAD as a valid ref."));
+ die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
- die(_("HEAD not found below refs/heads!"));
+ die(_("HEAD not found below refs/heads"));
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
0);
@@ -798,7 +798,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc) {
if (filter.detached)
- die(_("Cannot give description to detached HEAD"));
+ die(_("cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1) {
strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
@@ -810,8 +810,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
error((!argc || !strcmp(head, branch_name))
- ? _("No commit on branch '%s' yet.")
- : _("No branch named '%s'."),
+ ? _("no commit on branch '%s' yet")
+ : _("no branch named '%s'"),
branch_name);
else if (!edit_branch_description(branch_name))
ret = 0; /* happy */
@@ -824,8 +824,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!argc)
die(_("branch name required"));
else if ((argc == 1) && filter.detached)
- die(copy? _("cannot copy the current branch while not on any.")
- : _("cannot rename the current branch while not on any."));
+ die(copy? _("cannot copy the current branch while not on any")
+ : _("cannot rename the current branch while not on any"));
else if (argc == 1)
copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
else if (argc == 2)
@@ -848,14 +848,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not set upstream of HEAD to %s when "
- "it does not point to any branch."),
+ "it does not point to any branch"),
new_upstream);
die(_("no such branch '%s'"), argv[0]);
}
if (!ref_exists(branch->refname)) {
if (!argc || !strcmp(head, branch->name))
- die(_("No commit on branch '%s' yet."), branch->name);
+ die(_("no commit on branch '%s' yet"), branch->name);
die(_("branch '%s' does not exist"), branch->name);
}
@@ -878,12 +878,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch) {
if (!argc || !strcmp(argv[0], "HEAD"))
die(_("could not unset upstream of HEAD when "
- "it does not point to any branch."));
+ "it does not point to any branch"));
die(_("no such branch '%s'"), argv[0]);
}
if (!branch_has_merge_config(branch))
- die(_("Branch '%s' has no upstream information"), branch->name);
+ die(_("branch '%s' has no upstream information"), branch->name);
strbuf_reset(&buf);
strbuf_addf(&buf, "branch.%s.remote", branch->name);
@@ -897,11 +897,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
const char *start_name = argc == 2 ? argv[1] : head;
if (filter.kind != FILTER_REFS_BRANCHES)
- die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
+ die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
"Did you mean to use: -a|-r --list <pattern>?"));
if (track == BRANCH_TRACK_OVERRIDE)
- die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
+ die(_("the '--set-upstream' option is no longer supported\n"
+ "Please use '--track' or '--set-upstream-to' instead"));
if (recurse_submodules) {
create_branches_recursively(the_repository, branch_name,
diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
index 019a40df2c..b0992d5f91 100755
--- a/t/t2407-worktree-heads.sh
+++ b/t/t2407-worktree-heads.sh
@@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
grep "cannot force update the branch" err &&
test_must_fail git branch -D wt-$i 2>err &&
- grep "Cannot delete branch" err || return 1
+ grep "cannot delete branch" err || return 1
done
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 7f605f865b..8cfdc14b99 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -271,10 +271,10 @@ test_expect_success 'git branch -M topic topic should work when main is checked
test_expect_success 'git branch -M and -C fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: cannot rename the current branch while not on any." >expect &&
+ echo "fatal: cannot rename the current branch while not on any" >expect &&
test_must_fail git branch -M must-fail 2>err &&
test_cmp expect err &&
- echo "fatal: cannot copy the current branch while not on any." >expect &&
+ echo "fatal: cannot copy the current branch while not on any" >expect &&
test_must_fail git branch -C must-fail 2>err &&
test_cmp expect err
'
@@ -942,7 +942,7 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
test_expect_success '--set-upstream-to fails on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not set upstream of HEAD to main when it does not point to any branch." >expect &&
+ echo "fatal: could not set upstream of HEAD to main when it does not point to any branch" >expect &&
test_must_fail git branch --set-upstream-to main 2>err &&
test_cmp expect err
'
@@ -990,7 +990,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
'
test_expect_success '--unset-upstream should fail if given a non-existent branch' '
- echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
test_cmp expect err
'
@@ -1012,7 +1012,7 @@ test_expect_success 'test --unset-upstream on HEAD' '
test_must_fail git config branch.main.remote &&
test_must_fail git config branch.main.merge &&
# fail for a branch without upstream set
- echo "fatal: Branch '"'"'main'"'"' has no upstream information" >expect &&
+ echo "fatal: branch '"'"'main'"'"' has no upstream information" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
@@ -1026,7 +1026,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
test_expect_success '--unset-upstream should fail on detached HEAD' '
git checkout HEAD^{} &&
test_when_finished git checkout - &&
- echo "fatal: could not unset upstream of HEAD when it does not point to any branch." >expect &&
+ echo "fatal: could not unset upstream of HEAD when it does not point to any branch" >expect &&
test_must_fail git branch --unset-upstream 2>err &&
test_cmp expect err
'
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..23cf87e550 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
test_expect_success 'error descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- error: No commit on branch '\''$current'\'' yet.
+ error: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --edit-description 2>actual &&
test_cmp expect actual &&
@@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty repository' '
test_expect_success 'fatal descriptions on empty repository' '
current=$(git branch --show-current) &&
cat >expect <<-EOF &&
- fatal: No commit on branch '\''$current'\'' yet.
+ fatal: no commit on branch '\''$current'\'' yet
EOF
test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
test_cmp expect actual &&
@@ -199,7 +199,7 @@ EOF
test_expect_success 'error descriptions on non-existent branch' '
cat >expect <<-EOF &&
- error: No branch named '\''non-existent'\'.'
+ error: no branch named '\''non-existent'\''
EOF
test_must_fail git branch --edit-description non-existent 2>actual &&
test_cmp expect actual
@@ -213,7 +213,7 @@ test_expect_success 'fatal descriptions on non-existent branch' '
test_cmp expect actual &&
cat >expect <<-EOF &&
- fatal: No branch named '\''non-existent'\''.
+ fatal: no branch named '\''non-existent'\''
EOF
test_must_fail git branch -c non-existent new-branch 2>actual &&
test_cmp expect actual &&
--
2.38.1.432.ga6559ae16c
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-06 21:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-23 22:29 [PATCH] branch: error and informative messages Rubén Justo
2022-10-24 0:20 ` Junio C Hamano
2022-10-24 23:39 ` Rubén Justo
2022-10-25 19:28 ` Junio C Hamano
2022-10-25 23:52 ` Rubén Justo
2022-11-06 21:51 ` [PATCH v2] " Rubén Justo
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).