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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2023, 17:31 UTC
Message-ID: <xmqqwmvhqjyx.fsf@gitster.g>
In-Reply-To: <331e1ab3-2e8c-497d-a05d-ef197d664188@gmail.com>

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

Show 9 quoted lines

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

Show 5 quoted lines

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