[Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message

[Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message

[Isoken June Ibizugbe]

This patch improves the consistency and clarity of error messages across Git commands. It adheres to Git's Coding Guidelines for error messages:

- Error messages no longer end with a full stop.
- Capitalization is avoided in error messages.
- Error messages lead with a description of the issue, enhancing readability.

Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>

Isoken June Ibizugbe (1):
  branch.c: ammend error messages for die()

 builtin/branch.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

-- 
2.42.0.325.g3a06386e31

[PATCH 1/1] branch.c: ammend error messages for die()

[Isoken June Ibizugbe]

Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
---
 builtin/branch.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2ec190b14a..a756543d64 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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,9 +624,9 @@ 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)
@@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 	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);
@@ -892,7 +892,7 @@ 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.")
+			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);
@@ -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.325.g3a06386e31

Re: [PATCH 1/1] branch.c: ammend error messages for die()

[Junio C Hamano]

Isoken June Ibizugbe <isokenjune@gmail.com> writes:

> Subject: Re: [PATCH 1/1] branch.c: ammend error messages for die()

"ammend" is misspelt, but more importantly, it has less information
contents than other possible phrases, e.g.,

  Subject: [PATCH 1/1] branch.c: adjust die() messages to coding guidelines

In any case, the title of a commit has insufficient space to
describe what the amendment is about, or which exact guideline
these messages violate and needs adjustment.  This space before your
sign-off is where you write it.

Other outreachy candidates have already been given pretty much the
same pieces of advice.  It may help candidates to learn from the
responses given to other candidates.  For example, I said the same
thing in https://lore.kernel.org/git/xmqqlecbzl5e.fsf@gitster.g/

> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
>  builtin/branch.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)

Not a fault of this patch at all, but it is somewhat surprising that
we do not break any existing test with this many messages changed.
Did you run the test suite before making this commit?

Make it a habit to always do "make test" before committing your
work.  I am not saying "do not commit what does not pass the tests".
What I mean is "be aware of what is still broken (when fixing a bug)
or what you broke (when adding a new feature, perhaps as an
unintended side effect), before you commit, so that you can describe
them in your commit log message".

Thanks.

Re: [PATCH 1/1] branch.c: ammend error messages for die()

[Rubén Justo]

On 11-oct-2023 16:24:20, Isoken June Ibizugbe wrote:

Hi Isoken,

> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
>  builtin/branch.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..a756543d64 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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"),

OK.

>  			    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"),

OK.

>  			    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);

OK.

>  	}
>  
>  	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);

OK. Both.

>  	}
>  
>  	/*
> @@ -624,9 +624,9 @@ 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"));

Ditto

>  
>  	if (recovery) {
>  		if (copy)
> @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>  	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);

IMO we can go further here and also remove that final "!".  But it's OK
the way you have done it.

>  
>  	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"));

OK, both.

>  	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"));

OK.

>  	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"));

OK.

>  			branch_name = head;
>  		} else if (argc == 1) {
>  			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> @@ -892,7 +892,7 @@ 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.")
> +			die(copy? _("cannot copy the current branch while not on any")
>  				: _("cannot rename the current branch while not on any."));

OK.  But I think you want to modify also the second message, to remove
its full stop as well.

>  		else if (argc == 1)
>  			copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> @@ -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"),

OK.

>  				    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);

OK.

>  			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);

OK, both.

>  
>  		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>?"));

Good; the full stop removed and here that question mark is valuable.  And ...

>  
>  		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"));

also good.  But since we're here, maybe we can break this long message, remove
the first full stop and leave the second part of the message as is, as a
suggestion.  Like we do in the previous message, above.

For your consideration, I mean something like:

--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -969,7 +969,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
                                  "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,
> -- 
> 2.42.0.325.g3a06386e31
> 

One final comment, leaving aside my suggestions; this changes break
some tests that you need to adjust.  Try:

   $ make && (cd t; ./t3200-branch.sh -vi)

Or, as Junio has already suggested in another message:

   $ make test

I have some unfinished work that you might find useful:

https://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com/

Thank you for working on this.

Re: [PATCH 1/1] branch.c: ammend error messages for die()

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  	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);
>
> IMO we can go further here and also remove that final "!".  But it's OK
> the way you have done it.

Thanks for good eyes.  I do not think '!' is adding any value in
this case, so removing it is probably a good idea, even if it is
done outside the scope of this patch.

>>  		else if ((argc == 1) && filter.detached)
>> -			die(copy? _("cannot copy 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."));
>
> OK.  But I think you want to modify also the second message, to remove
> its full stop as well.

Nice spotting.

>> @@ -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>?"));
>
> Good; the full stop removed and here that question mark is valuable.  And ...

This one is not a single sentence, so retaining the full stop at the
end of the first sentence would actually be good, in addition to the
capitalization of the second sentence.

In a modern world, I would suspect that we would use die_message()
interface to emit the first sentence, show the "Did you mean..."
with advice_if_enabled(), and then exit with the status returned by
die_message(), similar to how branch.c::setup_tracking() does.  When
it happens, the die() message will be only the first sentence, and
what this patch did can be retained.  The second message will have
to be reworked to make it more appropriate as an advice message.

Thanks for a prompt review.

Re: [PATCH 1/1] branch.c: ammend error messages for die()

[Isoken Ibizugbe]

On Wed, Oct 11, 2023 at 7:05 PM Rubén Justo <rjusto@gmail.com> wrote:
>
> On 11-oct-2023 16:24:20, Isoken June Ibizugbe wrote:
>
> Hi Isoken,
>
> > Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> > ---
> >  builtin/branch.c | 38 +++++++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index 2ec190b14a..a756543d64 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -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"),
>
> OK.
>
> >                           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"),
>
> OK.
>
> >                           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);
>
> OK.
>
> >       }
> >
> >       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);
>
> OK. Both.
>
> >       }
> >
> >       /*
> > @@ -624,9 +624,9 @@ 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"));
>
> Ditto
>
> >
> >       if (recovery) {
> >               if (copy)
> > @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >       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);
>
> IMO we can go further here and also remove that final "!".  But it's OK
> the way you have done it.
>
> >
> >       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"));
>
> OK, both.
>
> >       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"));
>
> OK.
>
> >       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"));
>
> OK.
>
> >                       branch_name = head;
> >               } else if (argc == 1) {
> >                       strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> > @@ -892,7 +892,7 @@ 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.")
> > +                     die(copy? _("cannot copy the current branch while not on any")
> >                               : _("cannot rename the current branch while not on any."));
>
> OK.  But I think you want to modify also the second message, to remove
> its full stop as well.
>
> >               else if (argc == 1)
> >                       copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> > @@ -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"),
>
> OK.
>
> >                                   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);
>
> OK.
>
> >                       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);
>
> OK, both.
>
> >
> >               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>?"));
>
> Good; the full stop removed and here that question mark is valuable.  And ...
>
> >
> >               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"));
>
> also good.  But since we're here, maybe we can break this long message, remove
> the first full stop and leave the second part of the message as is, as a
> suggestion.  Like we do in the previous message, above.
>
> For your consideration, I mean something like:
>
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -969,7 +969,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>                                   "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,
> > --
> > 2.42.0.325.g3a06386e31
> >
>
> One final comment, leaving aside my suggestions; this changes break
> some tests that you need to adjust.  Try:
>
>    $ make && (cd t; ./t3200-branch.sh -vi)
>
> Or, as Junio has already suggested in another message:
>
>    $ make test
>
> I have some unfinished work that you might find useful:
>
> https://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com/
Thanks for the review. I'll make the requested changes and provide an
update. I'll also make sure to run the tests you suggested and make
the necessary changes.
>
> Thank you for working on this.

Re: [Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message

[Dragan Simic]

On 2023-10-11 17:24, Isoken June Ibizugbe wrote:
> This patch improves the consistency and clarity of error messages
> across Git commands. It adheres to Git's Coding Guidelines for error
> messages:
> 
> - Error messages no longer end with a full stop.
> - Capitalization is avoided in error messages.
> - Error messages lead with a description of the issue, enhancing 
> readability.
> 
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>

When there's only one patch and not a series, there's no need for a 
cover letter, and the description of the patch actually needs to go into 
the patch itself, so it can be latter pulled into the repository as part 
of the patch submission.

> Isoken June Ibizugbe (1):
>   branch.c: ammend error messages for die()
> 
>  builtin/branch.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)

Re: [Outreachy][PATCH 0/1] builtin/branch.c: ammend die() error message

[Dragan Simic]

On 2023-10-11 17:42, Isoken Ibizugbe wrote:
> Thanks for the feedback. How do I add the description of the patch.

Could you, please, use inline replying?  We've asked that at least four 
times already.

Regarding the description of the patch, just write it as the commit 
comment in your local git repository.  Using git-format-patch will pick 
the commit comment up and put it into the email message file you'll then 
send to the mailing list using git-send-email.  It's all rather simple 
and straightforward.


> On Wed, 11 Oct 2023 at 4:34 PM, Dragan Simic <dsimic@manjaro.org>
> wrote:
> 
>> On 2023-10-11 17:24, Isoken June Ibizugbe wrote:
>>> This patch improves the consistency and clarity of error messages
>>> across Git commands. It adheres to Git's Coding Guidelines for
>> error
>>> messages:
>>> 
>>> - Error messages no longer end with a full stop.
>>> - Capitalization is avoided in error messages.
>>> - Error messages lead with a description of the issue, enhancing
>>> readability.
>>> 
>>> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
>> 
>> When there's only one patch and not a series, there's no need for a
>> cover letter, and the description of the patch actually needs to go
>> into
>> the patch itself, so it can be latter pulled into the repository as
>> part
>> of the patch submission.
>> 
>>> Isoken June Ibizugbe (1):
>>> branch.c: ammend error messages for die()
>>> 
>>> builtin/branch.c | 38 +++++++++++++++++++-------------------
>>> 1 file changed, 19 insertions(+), 19 deletions(-)