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

>
> 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/