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

[Junio C Hamano <gitster@pobox.com>]

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.