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

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

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