Re: [PATCH] sequencer: allow disabling conflict advice

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Junio,

Le 2024-03-03 à 17:57, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> 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.

Philippe.