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

Commit Message

Philippe Blain March 10, 2024, 7:51 p.m. UTC

From: Philippe Blain <[e-mail]>

When 'git am' or 'git rebase --apply' encounter a conflict, they show a
message instructing the user how to continue the operation. This message
can't be disabled.

Use ADVICE_MERGE_CONFLICT introduced in the previous commit to allow
disabling it. Update the tests accordingly, as the advice output is now
on stderr instead of stdout. In t4150, redirect stdout to 'out' and
stderr to 'err', since this is less confusing. In t4254, as we are
testing a specific failure mode of 'git am', simply disable the advice.

Signed-off-by: Philippe Blain <[e-mail]>
---
 builtin/am.c          | 14 +++++++++-----
 t/t4150-am.sh         |  8 ++++----
 t/t4254-am-corrupt.sh |  2 +-
 3 files changed, 14 insertions(+), 10 deletions(-)

Comments

Phillip Wood March 11, 2024, 10:54 a.m. UTC | #1

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

Junio C Hamano March 11, 2024, 5:12 p.m. UTC | #2

[e-mail] writes:

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

Good eyes.  You'll get the final "\n" but the line breaks inside the
paragraph you give to advise*() functions are your responsibility.
Even though advice.c:vadvise() handles multi-line message better
(unlike usage.c:vreportf() that is used for error() and die()) by
giving a line header for each line of the message, we do not wrap
lines at runtime.

> 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

Thanks, both, and indeed it is a bit surprising.

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

Junio C Hamano March 11, 2024, 5:49 p.m. UTC | #3

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

>> I think you need to append "\n" to the message strings here (and
>> below) to match the behavior of printf_ln().
>
> Good eyes.  You'll get the final "\n" but the line breaks inside the
> paragraph you give to advise*() functions are your responsibility.
> Even though advice.c:vadvise() handles multi-line message better
> (unlike usage.c:vreportf() that is used for error() and die()) by
> giving a line header for each line of the message, we do not wrap
> lines at runtime.

Perhaps something like this.

The overly long lines are getting a bit annoying but I do not
offhand think of a good way to shorten them.

Also, having to assemble the message in a buffer and emit them all
once with ("%s" % sb.buf) is a highly annoying pattern.  Perhaps
given enough examples, somebody will come up with a simpler API to
do the same thing, but that is not in the scope of this series.

 builtin/am.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git i/builtin/am.c w/builtin/am.c
index 0e97b827e4..227036d732 100644
--- i/builtin/am.c
+++ w/builtin/am.c
@@ -1155,14 +1155,13 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 		const char *cmdline = state->interactive ? "git am -i" : "git am";
 		struct strbuf sb = STRBUF_INIT;
 
-		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);
+		strbuf_addf(&sb, _("When you have resolved this problem, run \"%s --continue\".\n"), cmdline);
+		strbuf_addf(&sb, _("If you prefer to skip this patch, run \"%s --skip\" instead.\n"), cmdline);
 
 		if (advice_enabled(ADVICE_AM_WORK_DIR) &&
 		    is_empty_or_missing_file(am_path(state, "patch")) &&
 		    !repo_index_has_changes(the_repository, NULL, NULL))
-			strbuf_addf(&sb, _("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\".\n"), 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);

Philippe Blain March 16, 2024, 7:44 p.m. UTC | #4

Hi Phillip and Junio,

Le 2024-03-11 à 13:49, Junio C Hamano a écrit :
> Junio C Hamano <[e-mail]> writes:
> 
>>> I think you need to append "\n" to the message strings here (and
>>> below) to match the behavior of printf_ln().
>>
>> Good eyes.  You'll get the final "\n" but the line breaks inside the
>> paragraph you give to advise*() functions are your responsibility.
>> Even though advice.c:vadvise() handles multi-line message better
>> (unlike usage.c:vreportf() that is used for error() and die()) by
>> giving a line header for each line of the message, we do not wrap
>> lines at runtime.
> 
> Perhaps something like this.

Thanks Phillip for noticing, and Junio for the fix. I should have looked
at the output, apologies. I made sure that the test passed but since 
t/t4150-am.sh only checks for the "To record the empty patch as an empty commit"
string, it still passed despite the missing newlines.

Just a note if it helps anyone: I cherry-picked Junio's fixes using:

   b4 shazam -P _ '<xmqq1q8gsloz.fsf@gitster.g>'


Cheers,

Philippe.

Philippe Blain March 16, 2024, 8:01 p.m. UTC | #5

Le 2024-03-11 à 13:12, Junio C Hamano a écrit :
> [e-mail] writes:
> 
>> 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().
> 
> Good eyes.  You'll get the final "\n" but the line breaks inside the
> paragraph you give to advise*() functions are your responsibility.
> Even though advice.c:vadvise() handles multi-line message better
> (unlike usage.c:vreportf() that is used for error() and die()) by
> giving a line header for each line of the message, we do not wrap
> lines at runtime.
> 
>> 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
> 
> Thanks, both, and indeed it is a bit surprising.
> 
>> 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.

We are not testing this advice when rebasing _with the apply backend_. 
We are testing it with the merge backend (in t5520-pull.sh) but we are 
only grepping stderr for  "Resolve all conflicts manually" so I did not 
have to change anything. I'll add that to the commit message for completeness.

We were testing the apply backend through the same test before 2ac0d6273f 
(rebase: change the default backend from "am" to "merge", 2020-02-15).

Thanks,
Philippe.

Patch

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);
 
 		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);
 	}
 
 	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