Re: [PATCH v2 2/2] builtin/am: allow disabling conflict advice
- From
- phillip.wood123@gmail.com <phillip.wood123@gmail.com>
- Date
- Mar 11, 2024, 10:54 UTC
- Message-ID
- <f06dcfad-e4b8-4cb7-8728-f5fb018f7be0@gmail.com>
- In-Reply-To
- <3235542cc6f77779cca1aeff65236e16b0a15d76.1710100261.git.gitgitgadget@gmail.com>
Hi Philippe
On 10/03/2024 19:51, Philippe Blain via GitGitGadget wrote:
Show 18 quoted lines
> 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
Show 53 quoted lines
> 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