Re: [PATCH v2 2/2] builtin/am: allow disabling conflict advice

[phillip.wood123@gmail.com]

Hi Philippe

On 10/03/2024 19:51, Philippe Blain via GitGitGadget wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> index d1990d7edcb..0e97b827e4b 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1150,19 +1150,23 @@ static const char *msgnum(const struct am_state *state)
>   static void NORETURN die_user_resolve(const struct am_state *state)
>   {
>   	if (state->resolvemsg) {
> -		printf_ln("%s", state->resolvemsg);
> +		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", state->resolvemsg);
>   	} else {
>   		const char *cmdline = state->interactive ? "git am -i" : "git am";
> +		struct strbuf sb = STRBUF_INIT;
>   
> -		printf_ln(_("When you have resolved this problem, run \"%s --continue\"."), cmdline);
> -		printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);
> +		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\"."), cmdline);
> +		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline);

I think you need to append "\n" to the message strings here (and below) 
to match the behavior of printf_ln().

Apart from that both patches look good to me, thanks for re-rolling. It 
is a bit surprising that we don't need to update any rebase tests. I 
haven't checked but I guess either we're not testing this advice when 
rebasing or we're using a grep expression that is vague enough not to be 
affected.

Best Wishes

Phillip

>   		if (advice_enabled(ADVICE_AM_WORK_DIR) &&
>   		    is_empty_or_missing_file(am_path(state, "patch")) &&
>   		    !repo_index_has_changes(the_repository, NULL, NULL))
> -			printf_ln(_("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
> +			strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline);
>   
> -		printf_ln(_("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);
> +		strbuf_addf(&sb, _("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline);
> +
> +		advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", sb.buf);
> +		strbuf_release(&sb);
>   	}message instructing the user how to continue the operation. This message
>   
>   	exit(128);
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 3b125762694..5e2b6c80eae 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -1224,8 +1224,8 @@ test_expect_success 'record as an empty commit when meeting e-mail message that
>   
>   test_expect_success 'skip an empty patch in the middle of an am session' '
>   	git checkout empty-commit^ &&
> -	test_must_fail git am empty-commit.patch >err &&
> -	grep "Patch is empty." err &&
> +	test_must_fail git am empty-commit.patch >out 2>err &&
> +	grep "Patch is empty." out &&
>   	grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err &&
>   	git am --skip &&
>   	test_path_is_missing .git/rebase-apply &&
> @@ -1236,8 +1236,8 @@ test_expect_success 'skip an empty patch in the middle of an am session' '
>   
>   test_expect_success 'record an empty patch as an empty commit in the middle of an am session' '
>   	git checkout empty-commit^ &&
> -	test_must_fail git am empty-commit.patch >err &&
> -	grep "Patch is empty." err &&
> +	test_must_fail git am empty-commit.patch >out 2>err &&
> +	grep "Patch is empty." out &&
>   	grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err &&
>   	git am --allow-empty >output &&
>   	grep "No changes - recorded it as an empty commit." output &&
> diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
> index 45f1d4f95e5..661feb60709 100755
> --- a/t/t4254-am-corrupt.sh
> +++ b/t/t4254-am-corrupt.sh
> @@ -59,7 +59,7 @@ test_expect_success setup '
>   # Also, it had the unwanted side-effect of deleting f.
>   test_expect_success 'try to apply corrupted patch' '
>   	test_when_finished "git am --abort" &&
> -	test_must_fail git -c advice.amWorkDir=false am bad-patch.diff 2>actual &&
> +	test_must_fail git -c advice.amWorkDir=false -c advice.mergeConflict=false am bad-patch.diff 2>actual &&
>   	echo "error: git diff header lacks filename information (line 4)" >expected &&
>   	test_path_is_file f &&
>   	test_cmp expected actual