threads / patch / 60389

patchbranch.c: adjust error messages to coding guidelines

Subject: [Outreachy][PATCH] branch.c: adjust error messages to coding guidelines

## tl;dr

12 messages between Oct 18, 2023 and Oct 21, 2023. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Isoken June Ibizugbe· Oct 18, 2023, 05:12 UTC · lore
Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
 builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)
Show changes to builtin/branch.c +33 −33
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..938c40bfaa 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
 	    (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != 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);
@@ -190,13 +190,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;
@@ -207,7 +207,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);
 }
 
@@ -260,7 +260,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' "
 					"used by worktree at '%s'"),
 				      bname.buf, path);
 				ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 					&oid, &flags);
 		if (!target) {
 			if (remote_branch) {
-				error(_("remote-tracking branch '%s' not found."), bname.buf);
+				error(_("remote-tracking branch '%s' not found"), bname.buf);
 			} else {
 				char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
 				char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 						"Did you forget --remote?"),
 						bname.buf);
 				else
-					error(_("branch '%s' not found."), bname.buf);
+					error(_("branch '%s' not found"), bname.buf);
 				FREE_AND_NULL(virtual_target);
 			}
 			ret = 1;
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
 			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);
 	}
 }
@@ -578,7 +578,7 @@ 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);
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
 		if (oldref_usage & IS_HEAD)
-			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);
 	}
 
 	/*
@@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	if (!copy && !(oldref_usage & IS_ORPHAN) &&
 	    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 && (oldref_usage & IS_HEAD) &&
 	    replace_each_worktree_head_symref(worktrees, 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);
 
 	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
 	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
 	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(interpreted_oldname, interpreted_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(&oldref);
 	strbuf_release(&newref);
 	strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ 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))
@@ -866,7 +866,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);
@@ -878,8 +878,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 || branch_checked_out(branch_ref.buf))
-			      ? _("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 */
@@ -892,8 +892,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)
@@ -916,14 +916,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 || branch_checked_out(branch->refname))
-				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);
 		}
 
@@ -946,12 +946,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);
@@ -965,11 +965,11 @@ 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. Please use '--track' or '--set-upstream-to' instead"));
 
 		if (recurse_submodules) {
 			create_branches_recursively(the_repository, branch_name,
-- 
2.42.0.346.g24618a8a3e.dirty
Rubén Justo· Oct 18, 2023, 18:19 UTC · re: Isoken June Ibizugbe · lore

Re: [Outreachy][PATCH] branch.c: adjust error messages to coding guidelines

On 18-oct-2023 06:12:22, Isoken June Ibizugbe wrote:
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
>  builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
Only builtin/branch.c is touched.

The changes in this patch break some tests, therefore this patch must also include the fixes for those tests.

Show 7 quoted lines
> @@ -965,11 +965,11 @@ 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>?"));

OK. The initial 'T' is fixed, but as Junio explained [1], the full stop must stay.

Thanks.
 [1] https://lore.kernel.org/git/xmqqttqxkmaq.fsf@gitster.g/
Junio C Hamano· Oct 18, 2023, 18:44 UTC · re: Rubén Justo · lore

Re: [Outreachy][PATCH] branch.c: adjust error messages to coding guidelines

Rubén Justo <rjusto@gmail.com> writes:
Show 11 quoted lines
> On 18-oct-2023 06:12:22, Isoken June Ibizugbe wrote:
>
>> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
>> ---
>>  builtin/branch.c | 66 ++++++++++++++++++++++++------------------------
>>  1 file changed, 33 insertions(+), 33 deletions(-)
>
> Only builtin/branch.c is touched.
>
> The changes in this patch break some tests, therefore this patch must
> also include the fixes for those tests.

Good point. I also notice the lack of body in the proposed commit message. It should say which part(s) of "coding guidelines" this change wants to make the code adhere to (maybe later we might change the guideline, and those who find this commit via "git blame" needs to understand what exact rule this change was aiming to follow, before they are confident that the change they plan to make is good).

Show 10 quoted lines
>> @@ -965,11 +965,11 @@ 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>?"));
>
> OK.  The initial 'T' is fixed, but as Junio explained [1], the full stop
> must stay.

Thanks for pointing out that it will help mentorship applicants to learn from reviews the other applicants are receiving, as they tend to make similar mistakes.

>
> Thanks.
>
>  [1] https://lore.kernel.org/git/xmqqttqxkmaq.fsf@gitster.g/
Isoken June Ibizugbe· Oct 19, 2023, 08:40 UTC · re: Rubén Justo · lore

[PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

As per the CodingGuidelines document, it is recommended that a single-line message provided to error messages such as die(), error() and warning(), should start with a lowercase letter and should not end with a period. Also this patch fixes the tests broken by the changes.

Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
 builtin/branch.c          | 66 +++++++++++++++++++--------------------
 t/t2407-worktree-heads.sh |  2 +-
 t/t3200-branch.sh         | 16 +++++-----
 t/t3202-show-branch.sh    | 10 +++---
 4 files changed, 47 insertions(+), 47 deletions(-)
Show changes to 4 files +47 −47

builtin/branch.c, t/t2407-worktree-heads.sh, t/t3200-branch.sh, t/t3202-show-branch.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..e7ee9bd0f1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
 	    (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != 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);
@@ -190,13 +190,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;
@@ -207,7 +207,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);
 }
 
@@ -260,7 +260,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' "
 					"used by worktree at '%s'"),
 				      bname.buf, path);
 				ret = 1;
@@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 					&oid, &flags);
 		if (!target) {
 			if (remote_branch) {
-				error(_("remote-tracking branch '%s' not found."), bname.buf);
+				error(_("remote-tracking branch '%s' not found"), bname.buf);
 			} else {
 				char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
 				char *virtual_target = resolve_refdup(virtual_name,
@@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 						"Did you forget --remote?"),
 						bname.buf);
 				else
-					error(_("branch '%s' not found."), bname.buf);
+					error(_("branch '%s' not found"), bname.buf);
 				FREE_AND_NULL(virtual_target);
 			}
 			ret = 1;
@@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
 			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);
 	}
 }
@@ -578,7 +578,7 @@ 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);
 	}
 
 	for (int i = 0; worktrees[i]; i++) {
@@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
 		if (oldref_usage & IS_HEAD)
-			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);
 	}
 
 	/*
@@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 
 	if (!copy && !(oldref_usage & IS_ORPHAN) &&
 	    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 && (oldref_usage & IS_HEAD) &&
 	    replace_each_worktree_head_symref(worktrees, 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);
 
 	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
 	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
 	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(interpreted_oldname, interpreted_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(&oldref);
 	strbuf_release(&newref);
 	strbuf_release(&oldsection);
@@ -773,7 +773,7 @@ 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))
@@ -866,7 +866,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);
@@ -878,8 +878,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 || branch_checked_out(branch_ref.buf))
-			      ? _("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 */
@@ -892,8 +892,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)
@@ -916,14 +916,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 || branch_checked_out(branch->refname))
-				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);
 		}
 
@@ -946,12 +946,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);
@@ -965,11 +965,11 @@ 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. 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 469443d8ae..f6835c91dc 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 080e4f24a6..3182abde27 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -291,10 +291,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
 '
@@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
 	git worktree add -b my7 my7 &&
 	test_must_fail git -C my7 branch -d my7 &&
 	test_must_fail git branch -d my7 2>actual &&
-	grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
 	rm -r my7 &&
 	git worktree prune
 '
@@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
 	git -C my7 bisect start HEAD HEAD~2 &&
 	test_must_fail git -C my7 branch -d my7 &&
 	test_must_fail git branch -d my7 2>actual &&
-	grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
+	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
 	rm -r my7 &&
 	git worktree prune
 '
@@ -1024,7 +1024,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
 '
@@ -1072,7 +1072,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
 '
@@ -1094,7 +1094,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
 '
@@ -1108,7 +1108,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 b17f388f56..2cdb834b37 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 &&
@@ -224,7 +224,7 @@ done
 
 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
@@ -238,7 +238,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 &&
@@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
 	test_branch_op_in_wt() {
 		test_orphan_error() {
 			test_must_fail git $* 2>actual &&
-			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+			test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
 		} &&
 		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
 		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
-- 
2.42.0.346.g24618a8a3e.dirty
Rubén Justo· Oct 19, 2023, 19:20 UTC · re: Isoken June Ibizugbe · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
> As per the CodingGuidelines document, it is recommended that a single-line
> message provided to error messages such as die(), error() and warning(),
This is confusing; some multi-line messages are fixed in this series.
> should start with a lowercase letter and should not end with a period.
> Also this patch fixes the tests broken by the changes.
Well done, describing why the tests are touched.
Show 8 quoted lines
> 
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
>  builtin/branch.c          | 66 +++++++++++++++++++--------------------
>  t/t2407-worktree-heads.sh |  2 +-
>  t/t3200-branch.sh         | 16 +++++-----
>  t/t3202-show-branch.sh    | 10 +++---
>  4 files changed, 47 insertions(+), 47 deletions(-)
Looking good.
Show 224 quoted lines
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..e7ee9bd0f1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
>  	    (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != 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);
> @@ -190,13 +190,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;
> @@ -207,7 +207,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);
>  }
>  
> @@ -260,7 +260,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' "
>  					"used by worktree at '%s'"),
>  				      bname.buf, path);
>  				ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  					&oid, &flags);
>  		if (!target) {
>  			if (remote_branch) {
> -				error(_("remote-tracking branch '%s' not found."), bname.buf);
> +				error(_("remote-tracking branch '%s' not found"), bname.buf);
>  			} else {
>  				char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
>  				char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  						"Did you forget --remote?"),
>  						bname.buf);
>  				else
> -					error(_("branch '%s' not found."), bname.buf);
> +					error(_("branch '%s' not found"), bname.buf);
>  				FREE_AND_NULL(virtual_target);
>  			}
>  			ret = 1;
> @@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
>  			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);
>  	}
>  }
> @@ -578,7 +578,7 @@ 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);
>  	}
>  
>  	for (int i = 0; worktrees[i]; i++) {
> @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  
>  	if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
>  		if (oldref_usage & IS_HEAD)
> -			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);
>  	}
>  
>  	/*
> @@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  
>  	if (!copy && !(oldref_usage & IS_ORPHAN) &&
>  	    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 && (oldref_usage & IS_HEAD) &&
>  	    replace_each_worktree_head_symref(worktrees, 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);
>  
>  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>  	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(interpreted_oldname, interpreted_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(&oldref);
>  	strbuf_release(&newref);
>  	strbuf_release(&oldsection);
> @@ -773,7 +773,7 @@ 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))
> @@ -866,7 +866,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);
> @@ -878,8 +878,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 || branch_checked_out(branch_ref.buf))
> -			      ? _("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 */
> @@ -892,8 +892,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)
> @@ -916,14 +916,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 || branch_checked_out(branch->refname))
> -				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);
>  		}
>  
> @@ -946,12 +946,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);
> @@ -965,11 +965,11 @@ 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"
OK.
Show 144 quoted lines
>  				  "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. 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 469443d8ae..f6835c91dc 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 080e4f24a6..3182abde27 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -291,10 +291,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
>  '
> @@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
>  	git worktree add -b my7 my7 &&
>  	test_must_fail git -C my7 branch -d my7 &&
>  	test_must_fail git branch -d my7 2>actual &&
> -	grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> +	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>  	rm -r my7 &&
>  	git worktree prune
>  '
> @@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
>  	git -C my7 bisect start HEAD HEAD~2 &&
>  	test_must_fail git -C my7 branch -d my7 &&
>  	test_must_fail git branch -d my7 2>actual &&
> -	grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> +	grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>  	rm -r my7 &&
>  	git worktree prune
>  '
> @@ -1024,7 +1024,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
>  '
> @@ -1072,7 +1072,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
>  '
> @@ -1094,7 +1094,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
>  '
> @@ -1108,7 +1108,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 b17f388f56..2cdb834b37 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 &&
> @@ -224,7 +224,7 @@ done
>  
>  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
> @@ -238,7 +238,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 &&
> @@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
>  	test_branch_op_in_wt() {
>  		test_orphan_error() {
>  			test_must_fail git $* 2>actual &&
> -			test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
> +			test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
>  		} &&
>  		test_orphan_error -C wt branch $1 $2 &&                # implicit branch
>  		test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
> -- 
> 2.42.0.346.g24618a8a3e.dirty
> 

All error messages in builtin/branch.c are fixed and I've locally run the tests with your changes, and all passes.

So, aside from the confusing message, this iteration looks good to me.
Thank you.
Junio C Hamano· Oct 20, 2023, 17:31 UTC · re: Rubén Justo · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

Rubén Justo <rjusto@gmail.com> writes:
Show 9 quoted lines
> On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
>
>> As per the CodingGuidelines document, it is recommended that a single-line
>> message provided to error messages such as die(), error() and warning(),
>
> This is confusing; some multi-line messages are fixed in this series.
>
>> should start with a lowercase letter and should not end with a period.
>> Also this patch fixes the tests broken by the changes.

"Also this patch fixes the tests broken by the changes" -> "Adjust tests to match updated messages".

> Well done, describing why the tests are touched.

Nicely reviewed. But it is unclear to me how we want to mention updates to multi-line messages. Do we have a good reference in the guidelines we can copy?

The general desire is for a single-liner to look like so:
    error: the branch 'foo' is not fully merged

and an untold assumption is that we strongly prefer a single sentence in a single-liner error message---a full-stop to separate multiple sentences can be omitted for brevity.

But we have follow-up sentences that are not strictly "errors" in some messages, e.g.,

Show 5 quoted lines
>> -		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);

In a modern codebase with facilities from advice.c, we would probably do "a single-liner error message, optionally followed by a hint message", i.e.

    error: the branch 'foo' is not fully merged
    hint: If you are sure you want to delete it,
    hint: run "git branch -D foo".

but this message apparently predates wide use of the advice_if() and friends.

But rewriting this error message to use advice is probably outside the scope of this patch. But for completeness it would probably look something like this (with necessary ADVICE_FOO defined):

	error(_("the branch '%s' is not fully merged"), branchname);
	advice_if_enabled(ADVICE_DELETE_BRANCH,
                	  _("If you are sure you want to delete it,"
			    "run 'git branch -D %s'."));
If I were doing this patch, I'd probably just add
	/* NEEDSWORK: turn the remainder ofthe message into advice */

in front of the existing error() call for this one without touching the message itself, as it will have to be touched again when such a change happens.

Thanks.
Isoken Ibizugbe· Oct 20, 2023, 17:59 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

On Fri, Oct 20, 2023 at 6:31 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 63 quoted lines
>
> Rubén Justo <rjusto@gmail.com> writes:
>
> > On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
> >
> >> As per the CodingGuidelines document, it is recommended that a single-line
> >> message provided to error messages such as die(), error() and warning(),
> >
> > This is confusing; some multi-line messages are fixed in this series.
> >
> >> should start with a lowercase letter and should not end with a period.
> >> Also this patch fixes the tests broken by the changes.
>
> "Also this patch fixes the tests broken by the changes" -> "Adjust
> tests to match updated messages".
>
> > Well done, describing why the tests are touched.
>
> Nicely reviewed.  But it is unclear to me how we want to mention
> updates to multi-line messages.  Do we have a good reference in the
> guidelines we can copy?
>
> The general desire is for a single-liner to look like so:
>
>     error: the branch 'foo' is not fully merged
>
> and an untold assumption is that we strongly prefer a single
> sentence in a single-liner error message---a full-stop to separate
> multiple sentences can be omitted for brevity.
>
> But we have follow-up sentences that are not strictly "errors" in
> some messages, e.g.,
>
> >> -            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);
>
> In a modern codebase with facilities from advice.c, we would
> probably do "a single-liner error message, optionally followed by a
> hint message", i.e.
>
>     error: the branch 'foo' is not fully merged
>     hint: If you are sure you want to delete it,
>     hint: run "git branch -D foo".
>
> but this message apparently predates wide use of the advice_if() and
> friends.
>
> But rewriting this error message to use advice is probably outside
> the scope of this patch.  But for completeness it would probably
> look something like this (with necessary ADVICE_FOO defined):
>
>         error(_("the branch '%s' is not fully merged"), branchname);
>         advice_if_enabled(ADVICE_DELETE_BRANCH,
>                           _("If you are sure you want to delete it,"
>                             "run 'git branch -D %s'."));
>
> If I were doing this patch, I'd probably just add
>
>         /* NEEDSWORK: turn the remainder ofthe message into advice */
>

Thanks for the feedback. Are suggesting I add this comment to error messages that are of this nature?

Show 5 quoted lines
> in front of the existing error() call for this one without touching
> the message itself, as it will have to be touched again when such a
> change happens.
>
> Thanks.
Rubén Justo· Oct 21, 2023, 10:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

On 20-oct-2023 10:31:18, Junio C Hamano wrote:
Show 8 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
> >
> >> As per the CodingGuidelines document, it is recommended that a single-line
> >> message provided to error messages such as die(), error() and warning(),
> >
> > This is confusing; some multi-line messages are fixed in this series.

What I expected to see is a re-roll with no mention about single/multi-line.

Show 5 quoted lines
> >> should start with a lowercase letter and should not end with a period.
> >> Also this patch fixes the tests broken by the changes.
> 
> "Also this patch fixes the tests broken by the changes" -> "Adjust
> tests to match updated messages".

That is a more palatable description of why the tests are being touched in this series. Thanks.

Rubén Justo· Oct 21, 2023, 10:27 UTC · re: Rubén Justo · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

On 19-oct-2023 21:20:24, Rubén Justo wrote:
> So, aside from the confusing message, this iteration looks good to me.

Reviewing again the messages in builtin/branch.c, if you finally re-roll, maybe you want to add:

Show changes to builtin/branch.c +1 −1
diff --git a/builtin/branch.c b/builtin/branch.c
index e7ee9bd0f1..31da889e95 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -777,7 +777,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
        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"));
 
But if you don't include this nit, that's fine with me.  I still think
the changes you've already made are correct.

Thank you.
Isoken Ibizugbe· Oct 20, 2023, 04:26 UTC · re: Isoken June Ibizugbe · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

On Thu, Oct 19, 2023 at 9:41 AM Isoken June Ibizugbe <isokenjune@gmail.com> wrote:

Show 13 quoted lines
>
> As per the CodingGuidelines document, it is recommended that a single-line
> message provided to error messages such as die(), error() and warning(),
> should start with a lowercase letter and should not end with a period.
> Also this patch fixes the tests broken by the changes.
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
>  builtin/branch.c          | 66 +++++++++++++++++++--------------------
>  t/t2407-worktree-heads.sh |  2 +-
>  t/t3200-branch.sh         | 16 +++++-----
>  t/t3202-show-branch.sh    | 10 +++---
>  4 files changed, 47 insertions(+), 47 deletions(-)
Hello Junio, I would appreciate your comment on this.
Show 369 quoted lines
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..e7ee9bd0f1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
>             (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != 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);
> @@ -190,13 +190,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;
> @@ -207,7 +207,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);
>  }
>
> @@ -260,7 +260,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' "
>                                         "used by worktree at '%s'"),
>                                       bname.buf, path);
>                                 ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                                         &oid, &flags);
>                 if (!target) {
>                         if (remote_branch) {
> -                               error(_("remote-tracking branch '%s' not found."), bname.buf);
> +                               error(_("remote-tracking branch '%s' not found"), bname.buf);
>                         } else {
>                                 char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
>                                 char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>                                                 "Did you forget --remote?"),
>                                                 bname.buf);
>                                 else
> -                                       error(_("branch '%s' not found."), bname.buf);
> +                                       error(_("branch '%s' not found"), bname.buf);
>                                 FREE_AND_NULL(virtual_target);
>                         }
>                         ret = 1;
> @@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
>                         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);
>         }
>  }
> @@ -578,7 +578,7 @@ 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);
>         }
>
>         for (int i = 0; worktrees[i]; i++) {
> @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
>         if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
>                 if (oldref_usage & IS_HEAD)
> -                       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);
>         }
>
>         /*
> @@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
>         if (!copy && !(oldref_usage & IS_ORPHAN) &&
>             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 && (oldref_usage & IS_HEAD) &&
>             replace_each_worktree_head_symref(worktrees, 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);
>
>         strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
>         strbuf_addf(&newsection, "branch.%s", interpreted_newname);
>         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(interpreted_oldname, interpreted_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(&oldref);
>         strbuf_release(&newref);
>         strbuf_release(&oldsection);
> @@ -773,7 +773,7 @@ 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))
> @@ -866,7 +866,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);
> @@ -878,8 +878,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 || branch_checked_out(branch_ref.buf))
> -                             ? _("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 */
> @@ -892,8 +892,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)
> @@ -916,14 +916,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 || branch_checked_out(branch->refname))
> -                               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);
>                 }
>
> @@ -946,12 +946,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);
> @@ -965,11 +965,11 @@ 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. 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 469443d8ae..f6835c91dc 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 080e4f24a6..3182abde27 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -291,10 +291,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
>  '
> @@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
>         git worktree add -b my7 my7 &&
>         test_must_fail git -C my7 branch -d my7 &&
>         test_must_fail git branch -d my7 2>actual &&
> -       grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> +       grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>         rm -r my7 &&
>         git worktree prune
>  '
> @@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
>         git -C my7 bisect start HEAD HEAD~2 &&
>         test_must_fail git -C my7 branch -d my7 &&
>         test_must_fail git branch -d my7 2>actual &&
> -       grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> +       grep "^error: cannot delete branch .my7. used by worktree at " actual &&
>         rm -r my7 &&
>         git worktree prune
>  '
> @@ -1024,7 +1024,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
>  '
> @@ -1072,7 +1072,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
>  '
> @@ -1094,7 +1094,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
>  '
> @@ -1108,7 +1108,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 b17f388f56..2cdb834b37 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 &&
> @@ -224,7 +224,7 @@ done
>
>  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
> @@ -238,7 +238,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 &&
> @@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
>         test_branch_op_in_wt() {
>                 test_orphan_error() {
>                         test_must_fail git $* 2>actual &&
> -                       test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
> +                       test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
>                 } &&
>                 test_orphan_error -C wt branch $1 $2 &&                # implicit branch
>                 test_orphan_error -C wt branch $1 orphan-branch $2 &&  # explicit branch
> --
> 2.42.0.346.g24618a8a3e.dirty
>
Christian Couder· Oct 20, 2023, 09:41 UTC · re: Isoken Ibizugbe · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

Hi Isoken,
On Fri, Oct 20, 2023 at 6:28 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
Show 17 quoted lines
>
> On Thu, Oct 19, 2023 at 9:41 AM Isoken June Ibizugbe
> <isokenjune@gmail.com> wrote:
> >
> > As per the CodingGuidelines document, it is recommended that a single-line
> > message provided to error messages such as die(), error() and warning(),
> > should start with a lowercase letter and should not end with a period.
> > Also this patch fixes the tests broken by the changes.
> >
> > Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> > ---
> >  builtin/branch.c          | 66 +++++++++++++++++++--------------------
> >  t/t2407-worktree-heads.sh |  2 +-
> >  t/t3200-branch.sh         | 16 +++++-----
> >  t/t3202-show-branch.sh    | 10 +++---
> >  4 files changed, 47 insertions(+), 47 deletions(-)
> Hello Junio, I would appreciate your comment on this.

Please don't expect Junio to give direct feedback on all the patches sent to the mailing list. This patch has already been reviewed by Rubén. If Junio doesn't review it and if this patch is not mentioned in the next "What's cooking in git.git" email from Junio, it likely means that you should send a version 3 addressing Rubén's feedback about the commit message.

Thanks, Christian.

Junio C Hamano· Oct 20, 2023, 17:10 UTC · re: Christian Couder · lore

Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines

Christian Couder <christian.couder@gmail.com> writes:
Show 5 quoted lines
>> Hello Junio, I would appreciate your comment on this.
>
> Please don't expect Junio to give direct feedback on all the patches
> sent to the mailing list. This patch has already been reviewed by
> Rubén.
Correct.
> If Junio doesn't review it and if this patch is not mentioned
> in the next "What's cooking in git.git" email from Junio, it likely
> means that you should send a version 3 addressing Rubén's feedback
> about the commit message.

While the above may be a good advice, I try to be easier to read than the above tealeaf-reading, especially to new contributors ;-)

At least I should comment on Rubén's comment, but I haven't got around to it yet.

Thanks.

← back to recent threads