Le 2024-03-09 à 12:22, Philippe Blain a écrit :
> Hi Junio,
>
> Le 2024-03-03 à 17:57, Junio C Hamano a écrit :
>> "Philippe Blain via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>>
>>> if (msg) {
>>> - advise("%s\n", msg);
>>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT, "%s\n", msg);
>>> /*
>>> * A conflict has occurred but the porcelain
>>> * (typically rebase --interactive) wants to take care
>>
>> This hunk is good. The block removes the CHERRY_PICK_HEAD after
>> giving this advice and then returns.
>>
>>> @@ -480,22 +480,25 @@ static void print_advice(struct repository *r, int show_hint,
>>>
>>> if (show_hint) {
>>> if (opts->no_commit)
>>> - advise(_("after resolving the conflicts, mark the corrected paths\n"
>>> - "with 'git add <paths>' or 'git rm <paths>'"));
>>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>>> + _("after resolving the conflicts, mark the corrected paths\n"
>>> + "with 'git add <paths>' or 'git rm <paths>'"));
>>> else if (opts->action == REPLAY_PICK)
>>> - advise(_("After resolving the conflicts, mark them with\n"
>>> - "\"git add/rm <pathspec>\", then run\n"
>>> - "\"git cherry-pick --continue\".\n"
>>> - "You can instead skip this commit with \"git cherry-pick --skip\".\n"
>>> - "To abort and get back to the state before \"git cherry-pick\",\n"
>>> - "run \"git cherry-pick --abort\"."));
>>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>>> + _("After resolving the conflicts, mark them with\n"
>>> + "\"git add/rm <pathspec>\", then run\n"
>>> + "\"git cherry-pick --continue\".\n"
>>> + "You can instead skip this commit with \"git cherry-pick --skip\".\n"
>>> + "To abort and get back to the state before \"git cherry-pick\",\n"
>>> + "run \"git cherry-pick --abort\"."));
>>> else if (opts->action == REPLAY_REVERT)
>>> - advise(_("After resolving the conflicts, mark them with\n"
>>> - "\"git add/rm <pathspec>\", then run\n"
>>> - "\"git revert --continue\".\n"
>>> - "You can instead skip this commit with \"git revert --skip\".\n"
>>> - "To abort and get back to the state before \"git revert\",\n"
>>> - "run \"git revert --abort\"."));
>>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>>> + _("After resolving the conflicts, mark them with\n"
>>> + "\"git add/rm <pathspec>\", then run\n"
>>> + "\"git revert --continue\".\n"
>>> + "You can instead skip this commit with \"git revert --skip\".\n"
>>> + "To abort and get back to the state before \"git revert\",\n"
>>> + "run \"git revert --abort\"."));
>>> else
>>> BUG("unexpected pick action in print_advice()");
>>> }
>>
>> This hunk can be improved. If I were doing this patch, I probably
>> would have just done
>>
>> - if (show_hint) {
>> + if (show_hint && advice_enabled(ADVICE_SEQUENCER_CONFLICT)) {
>>
>> and nothing else, and doing so would keep the block easier to extend
>> and maintain in the future.
>>
>> Because the block is all about "show_hint", we have code to print
>> advice messages and nothing else in it currently, and more
>> importantly, we will not add anything other than code to print
>> advice messages in it. Because of that, skipping everything when
>> ADVICE_SEQUENCER_CONFLICT is not enabled will not cause problems
>> (unlike the earlier hunk---which will break if we added "&&
>> advice_enabled()" to "if (msg)"). That way, when somebody teaches
>> this code a new kind of opts->action, they do not have to say
>> "advice_if_enabled(ADVICE_SEQUENCER_CONFLICT()"; they can just use
>> "advise()".
>
> That's true and makes the changes simpler, thank you for the suggestion.
> I'll do that in v2.
Thinking about this more and looking at the code, using 'advice_enabled' in the condition
instead of using 'advise_if_enabled' for each message has a side effect:
the text "hint: Disable this message with "git config advice.sequencerConflict false"
will not appear, which I find less user friendly...
Philippe.
