sequencer: allow disabling conflict advice

Commit Message

Philippe Blain March 2, 2024, 4:18 p.m. UTC

From: Philippe Blain <[e-mail]>

Allow disabling the advice shown when a squencer operation results in a
merge conflict through a new config 'advice.sequencerConflict'.

Update the tests accordingly. Note that the body of the second test in
t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
escape them in the added line.

Signed-off-by: Philippe Blain <[e-mail]>
---
    sequencer: allow disabling conflict advice
    
    CC: Elijah Newren [e-mail] CC: Phillip Wood
    [e-mail] CC: Johannes Schindelin
    [e-mail] CC: ZheNing Hu [e-mail]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1682

 Documentation/config/advice.txt |  3 +++
 advice.c                        |  1 +
 advice.h                        |  1 +
 sequencer.c                     | 33 ++++++++++++++++++---------------
 t/t3501-revert-cherry-pick.sh   |  1 +
 t/t3507-cherry-pick-conflict.sh |  2 ++
 6 files changed, 26 insertions(+), 15 deletions(-)


base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

Comments

Philippe Blain March 2, 2024, 4:32 p.m. UTC | #1

Hi, 

Le 2024-03-02 à 11:18, Philippe Blain via GitGitGadget a écrit :
> From: Philippe Blain <[e-mail]>
> 
> Allow disabling the advice shown when a squencer operation results in a
> merge conflict through a new config 'advice.sequencerConflict'.
> 
> Update the tests accordingly. Note that the body of the second test in
> t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
> escape them in the added line.
> 
> Signed-off-by: Philippe Blain <[e-mail]>

I meant to CC your addresses in https://lore.kernel.org/git/[e-mail]/
which I'm responding to, but the CC's did not get through somehow.

Cheers,

Philippe.

Junio C Hamano March 3, 2024, 10:57 p.m. UTC | #2

"Philippe Blain via GitGitGadget" <[e-mail]> 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()".

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index aeab689a98d..bc7c878b236 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh

All changes to the file look good.

Phillip Wood March 4, 2024, 10:12 a.m. UTC | #3

Hi Philippe

On 02/03/2024 16:18, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <[e-mail]>
> 
> Allow disabling the advice shown when a squencer operation results in a
> merge conflict through a new config 'advice.sequencerConflict'.

We already have "advice.resolveConflict" to suppress conflict advice. 
Can we extend that to these conflict messages rather than introducing a 
new category? As far as the user is concerned they are all messages 
about resolving conflicts - I don't really see why they'd want to 
suppress the messages from "git merge" separately to "git rebase" (and 
if they do then why is it ok to suppress the messages from "git merge", 
"git rebase" and "git cherry-pick" with a single setting). It would also 
be good to update the "rebase --apply" implementation to respect this 
advice config to be consistent with "rebase --merge".

Best Wishes

Phillip

> Update the tests accordingly. Note that the body of the second test in
> t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must
> escape them in the added line.
> 
> Signed-off-by: Philippe Blain <[e-mail]>
> ---
>      sequencer: allow disabling conflict advice
>      
>      CC: Elijah Newren [e-mail] CC: Phillip Wood
>      [e-mail] CC: Johannes Schindelin
>      [e-mail] CC: ZheNing Hu [e-mail]
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1682
> 
>   Documentation/config/advice.txt |  3 +++
>   advice.c                        |  1 +
>   advice.h                        |  1 +
>   sequencer.c                     | 33 ++++++++++++++++++---------------
>   t/t3501-revert-cherry-pick.sh   |  1 +
>   t/t3507-cherry-pick-conflict.sh |  2 ++
>   6 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index c7ea70f2e2e..736b88407a4 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -104,6 +104,9 @@ advice.*::
>   	rmHints::
>   		In case of failure in the output of linkgit:git-rm[1],
>   		show directions on how to proceed from the current state.
> +	sequencerConflict::
> +		Advice shown when a sequencer operation stops because
> +		of conflicts.
>   	sequencerInUse::
>   		Advice shown when a sequencer command is already in progress.
>   	skippedCherryPicks::
> diff --git a/advice.c b/advice.c
> index 6e9098ff089..23e48194e74 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -71,6 +71,7 @@ static struct {
>   	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
>   	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
>   	[ADVICE_RM_HINTS]				= { "rmHints" },
> +	[ADVICE_SEQUENCER_CONFLICT]                     = { "sequencerConflict" },
>   	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
>   	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
>   	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
> diff --git a/advice.h b/advice.h
> index 9d4f49ae38b..98966f8991d 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -40,6 +40,7 @@ enum advice_type {
>   	ADVICE_RESOLVE_CONFLICT,
>   	ADVICE_RM_HINTS,
>   	ADVICE_SEQUENCER_IN_USE,
> +	ADVICE_SEQUENCER_CONFLICT,
>   	ADVICE_SET_UPSTREAM_FAILURE,
>   	ADVICE_SKIPPED_CHERRY_PICKS,
>   	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
> diff --git a/sequencer.c b/sequencer.c
> index f49a871ac06..3e2f028ce2d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -467,7 +467,7 @@ static void print_advice(struct repository *r, int show_hint,
>   	char *msg = getenv("GIT_CHERRY_PICK_HELP");
>   
>   	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
> @@ -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()");
>   	}
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index aeab689a98d..bc7c878b236 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -170,6 +170,7 @@ test_expect_success 'advice from failed revert' '
>   	hint: You can instead skip this commit with "git revert --skip".
>   	hint: To abort and get back to the state before "git revert",
>   	hint: run "git revert --abort".
> +	hint: Disable this message with "git config advice.sequencerConflict false"
>   	EOF
>   	test_commit --append --no-tag "double-add dream" dream dream &&
>   	test_must_fail git revert HEAD^ 2>actual &&
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index c88d597b126..a643893dcbd 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -60,6 +60,7 @@ test_expect_success 'advice from failed cherry-pick' '
>   	hint: You can instead skip this commit with "git cherry-pick --skip".
>   	hint: To abort and get back to the state before "git cherry-pick",
>   	hint: run "git cherry-pick --abort".
> +	hint: Disable this message with "git config advice.sequencerConflict false"
>   	EOF
>   	test_must_fail git cherry-pick picked 2>actual &&
>   
> @@ -74,6 +75,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
>   	error: could not apply \$picked... picked
>   	hint: after resolving the conflicts, mark the corrected paths
>   	hint: with 'git add <paths>' or 'git rm <paths>'
> +	hint: Disable this message with \"git config advice.sequencerConflict false\"
>   	EOF
>   	test_must_fail git cherry-pick --no-commit picked 2>actual &&
>   
> 
> base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

Phillip Wood March 4, 2024, 10:27 a.m. UTC | #4

> We already have "advice.resolveConflict" to suppress conflict advice. 

Oh looking more closely that is doing something slightly different - it 
suppresses advice about pre-existing conflicts in the index when 
starting a merge etc. So we probably do need a new config variable but I 
think it should have a generic name - not be sequencer specific so we 
can extend its scope in the future to "git merge", "git am -3", "git 
stash" etc.

Best Wishes

Phillip

Junio C Hamano March 4, 2024, 5:56 p.m. UTC | #5

Phillip Wood <[e-mail]> writes:

> ... So we probably do need a new config variable but
> I think it should have a generic name - not be sequencer specific so
> we can extend its scope in the future to "git merge", "git am -3",
> "git stash" etc.

A very good point.  Thanks for your careful thinking.

Philippe Blain March 9, 2024, 5:22 p.m. UTC | #6

Hi Junio,

Le 2024-03-03 à 17:57, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <[e-mail]> 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.

Philippe Blain March 9, 2024, 5:53 p.m. UTC | #7

Hi Phillip and Junio,

Le 2024-03-04 à 12:56, Junio C Hamano a écrit :
> Phillip Wood <[e-mail]> writes:
> 
>> ... So we probably do need a new config variable but
>> I think it should have a generic name - not be sequencer specific so
>> we can extend its scope in the future to "git merge", "git am -3",
>> "git stash" etc.
> 
> A very good point.  Thanks for your careful thinking.

OK, I agree we can make the new advice more generic, but I'm lacking
inspiration for the name. Maybe 'advice.mergeConflicted' ? 
Or 'advice.resolveConflictedMerge' ? though this is close to the existing 
'resolveConflict'... 
Maybe just 'advice.mergeConflict' ?

Thanks, 
Philippe.

Philippe Blain March 9, 2024, 6:01 p.m. UTC | #8

Hi Phillip,

Le 2024-03-04 à 05:12, Phillip Wood a écrit :
> Hi Philippe
> 
> On 02/03/2024 16:18, Philippe Blain via GitGitGadget wrote:
> 
> It would also be good to update the "rebase --apply" implementation to respect this advice config to be consistent with "rebase --merge".
Yes, this is a good idea. This would take care of 'git am' at the same time
since both are implemented in builtin/am.c::die_user_resolve.

Thanks,
Philippe.

Philippe Blain March 9, 2024, 6:58 p.m. UTC | #9

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" <[e-mail]> 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.

Phillip Wood March 9, 2024, 7:15 p.m. UTC | #10

Hi Philippe

On 09/03/2024 17:53, Philippe Blain wrote:
> Le 2024-03-04 à 12:56, Junio C Hamano a écrit :
>> Phillip Wood <[e-mail]> writes:
>>
>>> ... So we probably do need a new config variable but
>>> I think it should have a generic name - not be sequencer specific so
>>> we can extend its scope in the future to "git merge", "git am -3",
>>> "git stash" etc.
>>
>> A very good point.  Thanks for your careful thinking.
> 
> OK, I agree we can make the new advice more generic, but I'm lacking
> inspiration for the name. Maybe 'advice.mergeConflicted' ?
> Or 'advice.resolveConflictedMerge' ? though this is close to the existing
> 'resolveConflict'...
> Maybe just 'advice.mergeConflict' ?

'advice.mergeConflict' sounds good to me

Best Wishes

Phillip

Junio C Hamano March 9, 2024, 7:55 p.m. UTC | #11

Philippe Blain <[e-mail]> writes:

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

Good eyes.

I agree that you have to do that part yourself at the end of that
"if () { ... }" block using turn_off_instructions[] string.

Junio C Hamano March 9, 2024, 7:56 p.m. UTC | #12

Phillip Wood <[e-mail]> writes:

>> Maybe just 'advice.mergeConflict' ?
>
> 'advice.mergeConflict' sounds good to me

Yup, looks good to me, too.

Junio C Hamano March 9, 2024, 11:19 p.m. UTC | #13

Junio C Hamano <[e-mail]> writes:

> Philippe Blain <[e-mail]> writes:
>
>> 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...
>
> Good eyes.
>
> I agree that you have to do that part yourself at the end of that
> "if () { ... }" block using turn_off_instructions[] string.

Forgot to add "... which is an unnecessary chore" at the end.

Thanks.

Patch

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..736b88407a4 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -104,6 +104,9 @@  advice.*::
 	rmHints::
 		In case of failure in the output of linkgit:git-rm[1],
 		show directions on how to proceed from the current state.
+	sequencerConflict::
+		Advice shown when a sequencer operation stops because
+		of conflicts.
 	sequencerInUse::
 		Advice shown when a sequencer command is already in progress.
 	skippedCherryPicks::
diff --git a/advice.c b/advice.c
index 6e9098ff089..23e48194e74 100644
--- a/advice.c
+++ b/advice.c
@@ -71,6 +71,7 @@  static struct {
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
 	[ADVICE_RM_HINTS]				= { "rmHints" },
+	[ADVICE_SEQUENCER_CONFLICT]                     = { "sequencerConflict" },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
 	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
 	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..98966f8991d 100644
--- a/advice.h
+++ b/advice.h
@@ -40,6 +40,7 @@  enum advice_type {
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,
+	ADVICE_SEQUENCER_CONFLICT,
 	ADVICE_SET_UPSTREAM_FAILURE,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..3e2f028ce2d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -467,7 +467,7 @@  static void print_advice(struct repository *r, int show_hint,
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
 
 	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
@@ -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()");
 	}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index aeab689a98d..bc7c878b236 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -170,6 +170,7 @@  test_expect_success 'advice from failed revert' '
 	hint: You can instead skip this commit with "git revert --skip".
 	hint: To abort and get back to the state before "git revert",
 	hint: run "git revert --abort".
+	hint: Disable this message with "git config advice.sequencerConflict false"
 	EOF
 	test_commit --append --no-tag "double-add dream" dream dream &&
 	test_must_fail git revert HEAD^ 2>actual &&
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index c88d597b126..a643893dcbd 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -60,6 +60,7 @@  test_expect_success 'advice from failed cherry-pick' '
 	hint: You can instead skip this commit with "git cherry-pick --skip".
 	hint: To abort and get back to the state before "git cherry-pick",
 	hint: run "git cherry-pick --abort".
+	hint: Disable this message with "git config advice.sequencerConflict false"
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
@@ -74,6 +75,7 @@  test_expect_success 'advice from failed cherry-pick --no-commit' "
 	error: could not apply \$picked... picked
 	hint: after resolving the conflicts, mark the corrected paths
 	hint: with 'git add <paths>' or 'git rm <paths>'
+	hint: Disable this message with \"git config advice.sequencerConflict false\"
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&