Re: [PATCH v2 2/2] builtin/am: allow disabling conflict advice

[Philippe Blain <levraiphilippeblain@gmail.com>]

Le 2024-03-11 à 13:12, Junio C Hamano a écrit :
> phillip.wood123@gmail.com writes:
> 
>> Hi Philippe
>>
>> On 10/03/2024 19:51, Philippe Blain via GitGitGadget wrote:
>>> diff --git a/builtin/am.c b/builtin/am.c
>>> index d1990d7edcb..0e97b827e4b 100644
>>> --- a/builtin/am.c
>>> +++ b/builtin/am.c
>>> @@ -1150,19 +1150,23 @@ static const char *msgnum(const struct am_state *state)
>>>   static void NORETURN die_user_resolve(const struct am_state *state)
>>>   {
>>>   	if (state->resolvemsg) {
>>> -		printf_ln("%s", state->resolvemsg);
>>> +		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", state->resolvemsg);
>>>   	} else {
>>>   		const char *cmdline = state->interactive ? "git am -i" : "git am";
>>> +		struct strbuf sb = STRBUF_INIT;
>>>   -		printf_ln(_("When you have resolved this problem, run
>>> \"%s --continue\"."), cmdline);
>>> -		printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
>>> +		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\"."), cmdline);
>>> +		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
>>
>> I think you need to append "\n" to the message strings here (and
>> below) to match the behavior of printf_ln().
> 
> Good eyes.  You'll get the final "\n" but the line breaks inside the
> paragraph you give to advise*() functions are your responsibility.
> Even though advice.c:vadvise() handles multi-line message better
> (unlike usage.c:vreportf() that is used for error() and die()) by
> giving a line header for each line of the message, we do not wrap
> lines at runtime.
> 
>> Apart from that both patches look good to me, thanks for
>> re-rolling. It is a bit surprising that we don't need to update any
> 
> Thanks, both, and indeed it is a bit surprising.
> 
>> rebase tests. I haven't checked but I guess either we're not testing
>> this advice when rebasing or we're using a grep expression that is
>> vague enough not to be affected.

We are not testing this advice when rebasing _with the apply backend_. 
We are testing it with the merge backend (in t5520-pull.sh) but we are 
only grepping stderr for  "Resolve all conflicts manually" so I did not 
have to change anything. I'll add that to the commit message for completeness.

We were testing the apply backend through the same test before 2ac0d6273f 
(rebase: change the default backend from "am" to "merge", 2020-02-15).

Thanks,
Philippe.