Re: [PATCH] sequencer: allow disabling conflict advice

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2024, 22:57 UTC
Message-ID: <xmqqwmqiudna.fsf@gitster.g>
In-Reply-To: <pull.1682.git.1709396291693.gitgitgadget@gmail.com>

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

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

Show 40 quoted lines

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