Le 2024-03-11 à 13:12, Junio C Hamano a écrit :
> phillip.wood123@xxxxxxxxx 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.
