On Fri, Oct 20, 2023 at 6:31 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Rubén Justo <rjusto@xxxxxxxxx> 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?
> 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.
