Re: [PATCH] sequencer: allow disabling conflict advice

[Junio C Hamano <gitster@pobox.com>]

"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()".

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