| Message ID | pull.1682.v3.git.1710623790.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes: > This series introduces a new config 'advice.mergeConflict' and uses it to > allow disabling the advice shown when 'git rebase', 'git cherry-pick', 'git > revert', 'git rebase --apply' and 'git am' stop because of conflicts. > > Thanks everyone for the reviews! > > Changes since v2: > > * expanded the commit messages to explain why the tests for 'git rebase' do > not need to be adjusted > * adjusted the wording of the new 'advice.mergeConflict' in the doc, as > suggested by Kristoffer for uniformity with his series which is already > merged to 'master' (b09a8839a4 (Merge branch > 'kh/branch-ref-syntax-advice', 2024-03-15)). > * checked all new output manually and consequently adjusted the code in 1/2 > to avoid a lonely 'hint: ' line. > * adjusted the addition in advice.h in 1/2 to put the new enum > alphabetically, as noticed by Rubén. > * added misssing newlines in 2/2 as noticed by Phillip and tweaked by > Junio. > * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid > conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged > series Looking good; will queue. Thanks.
Hi Philippe On 16/03/2024 21:16, Philippe Blain via GitGitGadget wrote: > Changes since v2: > > * expanded the commit messages to explain why the tests for 'git rebase' do > not need to be adjusted > * adjusted the wording of the new 'advice.mergeConflict' in the doc, as > suggested by Kristoffer for uniformity with his series which is already > merged to 'master' (b09a8839a4 (Merge branch > 'kh/branch-ref-syntax-advice', 2024-03-15)). > * checked all new output manually and consequently adjusted the code in 1/2 > to avoid a lonely 'hint: ' line. > * adjusted the addition in advice.h in 1/2 to put the new enum > alphabetically, as noticed by Rubén. > * added misssing newlines in 2/2 as noticed by Phillip and tweaked by > Junio. > * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid > conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged > series > [...] > Note that the code path where 'git rebase --apply' stops because of > conflicts is not covered by the tests but I tested it manually using this > diff: > > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index 47534f1062..34eac2e6f4 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -374,7 +374,7 @@ test_pull_autostash_fail () > echo conflicting >>seq.txt && > test_tick && > git commit -m "Create conflict" seq.txt && > - test_must_fail git pull --rebase . seq 2>err >out && > + test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out && > test_grep "Resolve all conflicts manually" err > ' Thanks for being so thorough, this version looks good to me Best Wishes Phillip > > Philippe Blain (2): > sequencer: allow disabling conflict advice > builtin/am: allow disabling conflict advice > > Documentation/config/advice.txt | 2 ++ > advice.c | 1 + > advice.h | 1 + > builtin/am.c | 14 +++++++++----- > sequencer.c | 33 ++++++++++++++++++--------------- > t/t3501-revert-cherry-pick.sh | 1 + > t/t3507-cherry-pick-conflict.sh | 2 ++ > t/t4150-am.sh | 8 ++++---- > t/t4254-am-corrupt.sh | 2 +- > 9 files changed, 39 insertions(+), 25 deletions(-) > > > base-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1682 > > Range-diff vs v2: > > 1: a2ce6fd24c2 ! 1: 6005c1e9890 sequencer: allow disabling conflict advice > @@ Commit message > merge conflict through a new config 'advice.mergeConflict', which is > named generically such that it can be used by other commands eventually. > > + Remove that final '\n' in the first hunk in sequencer.c to avoid an > + otherwise empty 'hint: ' line before the line 'hint: Disable this > + message with "git config advice.mergeConflict false"' which is > + automatically added by 'advise_if_enabled'. > + > Note that we use 'advise_if_enabled' for each message in the second hunk > in sequencer.c, instead of using 'if (show_hints && > advice_enabled(...)', because the former instructs the user how to > @@ Commit message > > Update the tests accordingly. Note that the body of the second test in > t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must > - escape them in the added line. > + escape them in the added line. Note that t5520-pull.sh, which checks > + that we display the advice for 'git rebase' (via 'git pull --rebase') > + does not have to be updated because it only greps for a specific line in > + the advice message. > > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > > ## Documentation/config/advice.txt ## > @@ Documentation/config/advice.txt: advice.*:: > - Advice on how to set your identity configuration when > - your information is guessed from the system username and > - domain name. > + Shown when the user's information is guessed from the > + system username and domain name, to tell the user how to > + set their identity configuration. > + mergeConflict:: > -+ Advice shown when various commands stop because of conflicts. > ++ Shown when various commands stop because of conflicts. > nestedTag:: > - Advice shown if a user attempts to recursively tag a tag object. > + Shown when a user attempts to recursively tag a tag object. > pushAlreadyExists:: > > ## advice.c ## > @@ advice.c: static struct { > > ## advice.h ## > @@ advice.h: enum advice_type { > + ADVICE_GRAFT_FILE_DEPRECATED, > ADVICE_IGNORED_HOOK, > ADVICE_IMPLICIT_IDENTITY, > - ADVICE_NESTED_TAG, > + ADVICE_MERGE_CONFLICT, > + ADVICE_NESTED_TAG, > ADVICE_OBJECT_NAME_WARNING, > ADVICE_PUSH_ALREADY_EXISTS, > - ADVICE_PUSH_FETCH_FIRST, > > ## sequencer.c ## > @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, > @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, > > if (msg) { > - advise("%s\n", msg); > -+ advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg); > ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", msg); > /* > * A conflict has occurred but the porcelain > * (typically rebase --interactive) wants to take care > 2: 3235542cc6f ! 2: 73d07c8b6a7 builtin/am: allow disabling conflict advice > @@ Commit message > 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. > + Note that we are not testing that this advice is shown in 'git rebase' > + for the apply backend since 2ac0d6273f (rebase: change the default > + backend from "am" to "merge", 2020-02-15). > > + Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> > + Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > > ## builtin/am.c ## > @@ builtin/am.c: static const char *msgnum(const struct am_state *state) > > - 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); > ++ 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)) > - 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); > ++ strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\".\n"), 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); >
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Philippe > > On 16/03/2024 21:16, Philippe Blain via GitGitGadget wrote: >> Changes since v2: >> * expanded the commit messages to explain why the tests for 'git >> rebase' do >> not need to be adjusted >> * adjusted the wording of the new 'advice.mergeConflict' in the doc, as >> suggested by Kristoffer for uniformity with his series which is already >> merged to 'master' (b09a8839a4 (Merge branch >> 'kh/branch-ref-syntax-advice', 2024-03-15)). >> * checked all new output manually and consequently adjusted the code in 1/2 >> to avoid a lonely 'hint: ' line. >> * adjusted the addition in advice.h in 1/2 to put the new enum >> alphabetically, as noticed by Rubén. >> * added misssing newlines in 2/2 as noticed by Phillip and tweaked by >> Junio. >> * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid >> conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged > series >> [...] Note that the code path where 'git rebase --apply' stops >> because of >> conflicts is not covered by the tests but I tested it manually using this >> diff: >> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh >> index 47534f1062..34eac2e6f4 100755 >> --- a/t/t5520-pull.sh >> +++ b/t/t5520-pull.sh >> @@ -374,7 +374,7 @@ test_pull_autostash_fail () >> echo conflicting >>seq.txt && >> test_tick && >> git commit -m "Create conflict" seq.txt && >> - test_must_fail git pull --rebase . seq 2>err >out && >> + test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out && >> test_grep "Resolve all conflicts manually" err >> ' > > Thanks for being so thorough, this version looks good to me Yup, these look good. Let's mark the topic for 'next'. Thanks, both.
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 47534f1062..34eac2e6f4 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -374,7 +374,7 @@ test_pull_autostash_fail () echo conflicting >>seq.txt && test_tick && git commit -m "Create conflict" seq.txt && - test_must_fail git pull --rebase . seq 2>err >out && + test_must_fail git -c rebase.backend=apply pull --rebase . seq 2>err >out && test_grep "Resolve all conflicts manually" err '
This series introduces a new config 'advice.mergeConflict' and uses it to allow disabling the advice shown when 'git rebase', 'git cherry-pick', 'git revert', 'git rebase --apply' and 'git am' stop because of conflicts. Thanks everyone for the reviews! Changes since v2: * expanded the commit messages to explain why the tests for 'git rebase' do not need to be adjusted * adjusted the wording of the new 'advice.mergeConflict' in the doc, as suggested by Kristoffer for uniformity with his series which is already merged to 'master' (b09a8839a4 (Merge branch 'kh/branch-ref-syntax-advice', 2024-03-15)). * checked all new output manually and consequently adjusted the code in 1/2 to avoid a lonely 'hint: ' line. * adjusted the addition in advice.h in 1/2 to put the new enum alphabetically, as noticed by Rubén. * added misssing newlines in 2/2 as noticed by Phillip and tweaked by Junio. * rebased on master (2953d95d40 (The eighth batch, 2024-03-15)), to avoid conflicts in 'Documentation/config/advice.txt' due to Kristoffer's merged series Changes since v1: * renamed the new advice to 'advice.mergeConflict' to make it non-sequencer specific * added 2/2 which uses the advice in builtin/am, which covers 'git rebase --apply' and 'git am' Note that the code path where 'git rebase --apply' stops because of conflicts is not covered by the tests but I tested it manually using this diff: Philippe Blain (2): sequencer: allow disabling conflict advice builtin/am: allow disabling conflict advice Documentation/config/advice.txt | 2 ++ advice.c | 1 + advice.h | 1 + builtin/am.c | 14 +++++++++----- sequencer.c | 33 ++++++++++++++++++--------------- t/t3501-revert-cherry-pick.sh | 1 + t/t3507-cherry-pick-conflict.sh | 2 ++ t/t4150-am.sh | 8 ++++---- t/t4254-am-corrupt.sh | 2 +- 9 files changed, 39 insertions(+), 25 deletions(-) base-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1682 Range-diff vs v2: 1: a2ce6fd24c2 ! 1: 6005c1e9890 sequencer: allow disabling conflict advice @@ Commit message merge conflict through a new config 'advice.mergeConflict', which is named generically such that it can be used by other commands eventually. + Remove that final '\n' in the first hunk in sequencer.c to avoid an + otherwise empty 'hint: ' line before the line 'hint: Disable this + message with "git config advice.mergeConflict false"' which is + automatically added by 'advise_if_enabled'. + Note that we use 'advise_if_enabled' for each message in the second hunk in sequencer.c, instead of using 'if (show_hints && advice_enabled(...)', because the former instructs the user how to @@ Commit message Update the tests accordingly. Note that the body of the second test in t3507-cherry-pick-conflict.sh is enclosed in double quotes, so we must - escape them in the added line. + escape them in the added line. Note that t5520-pull.sh, which checks + that we display the advice for 'git rebase' (via 'git pull --rebase') + does not have to be updated because it only greps for a specific line in + the advice message. Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> ## Documentation/config/advice.txt ## @@ Documentation/config/advice.txt: advice.*:: - Advice on how to set your identity configuration when - your information is guessed from the system username and - domain name. + Shown when the user's information is guessed from the + system username and domain name, to tell the user how to + set their identity configuration. + mergeConflict:: -+ Advice shown when various commands stop because of conflicts. ++ Shown when various commands stop because of conflicts. nestedTag:: - Advice shown if a user attempts to recursively tag a tag object. + Shown when a user attempts to recursively tag a tag object. pushAlreadyExists:: ## advice.c ## @@ advice.c: static struct { ## advice.h ## @@ advice.h: enum advice_type { + ADVICE_GRAFT_FILE_DEPRECATED, ADVICE_IGNORED_HOOK, ADVICE_IMPLICIT_IDENTITY, - ADVICE_NESTED_TAG, + ADVICE_MERGE_CONFLICT, + ADVICE_NESTED_TAG, ADVICE_OBJECT_NAME_WARNING, ADVICE_PUSH_ALREADY_EXISTS, - ADVICE_PUSH_FETCH_FIRST, ## sequencer.c ## @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, if (msg) { - advise("%s\n", msg); -+ advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg); ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s", msg); /* * A conflict has occurred but the porcelain * (typically rebase --interactive) wants to take care 2: 3235542cc6f ! 2: 73d07c8b6a7 builtin/am: allow disabling conflict advice @@ Commit message 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. + Note that we are not testing that this advice is shown in 'git rebase' + for the apply backend since 2ac0d6273f (rebase: change the default + backend from "am" to "merge", 2020-02-15). + Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> + Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> ## builtin/am.c ## @@ builtin/am.c: static const char *msgnum(const struct am_state *state) - 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); ++ 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)) - 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); ++ strbuf_addf(&sb, _("To record the empty patch as an empty commit, run \"%s --allow-empty\".\n"), 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);