| Message ID | pull.1682.v2.git.1710100261.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Mar 10, 2024 at 07:50:59PM +0000, Philippe Blain via GitGitGadget wrote: > Range-diff vs v1: > > 1: e929d3381cf ! 1: a2ce6fd24c2 sequencer: allow disabling conflict advice [...] > ## advice.c ## > @@ advice.c: static struct { > - [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" }, > - [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" }, > - [ADVICE_RM_HINTS] = { "rmHints" }, > -+ [ADVICE_SEQUENCER_CONFLICT] = { "sequencerConflict" }, > - [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" }, > - [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" }, > - [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" }, > + [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" }, > + [ADVICE_IGNORED_HOOK] = { "ignoredHook" }, > + [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" }, > ++ [ADVICE_MERGE_CONFLICT] = { "mergeConflict" }, > + [ADVICE_NESTED_TAG] = { "nestedTag" }, > + [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" }, > + [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" }, You rename ADVICE_SEQUENCER_CONFLICT to ADVICE_MERGE_CONFLICT and place the new name in the correct position, alphabetically. Nice. > ## advice.h ## > @@ advice.h: enum advice_type { > - ADVICE_RESOLVE_CONFLICT, > - ADVICE_RM_HINTS, > - ADVICE_SEQUENCER_IN_USE, > -+ ADVICE_SEQUENCER_CONFLICT, > - ADVICE_SET_UPSTREAM_FAILURE, > - ADVICE_SKIPPED_CHERRY_PICKS, > - ADVICE_STATUS_AHEAD_BEHIND_WARNING, > + ADVICE_IGNORED_HOOK, > + ADVICE_IMPLICIT_IDENTITY, > + ADVICE_NESTED_TAG, > ++ ADVICE_MERGE_CONFLICT, > + ADVICE_OBJECT_NAME_WARNING, > + ADVICE_PUSH_ALREADY_EXISTS, > + ADVICE_PUSH_FETCH_FIRST, Here, I assume you're trying to place the new name correctly too. However, I see that it's in the wrong place. It initially caught my attention, but then I realize that the list is not sorted. So it's understandable. Maybe you want to sort the list as a preparatory patch in this series and so we'll avoid this kind of mistakes. Of course, this does not deserve a reroll. We can do it in a future series when the dust settles.
Hi Rubén, Le 2024-03-11 à 16:58, Rubén Justo a écrit : > On Sun, Mar 10, 2024 at 07:50:59PM +0000, Philippe Blain via GitGitGadget wrote: > > >> ## advice.h ## >> @@ advice.h: enum advice_type { >> - ADVICE_RESOLVE_CONFLICT, >> - ADVICE_RM_HINTS, >> - ADVICE_SEQUENCER_IN_USE, >> -+ ADVICE_SEQUENCER_CONFLICT, >> - ADVICE_SET_UPSTREAM_FAILURE, >> - ADVICE_SKIPPED_CHERRY_PICKS, >> - ADVICE_STATUS_AHEAD_BEHIND_WARNING, >> + ADVICE_IGNORED_HOOK, >> + ADVICE_IMPLICIT_IDENTITY, >> + ADVICE_NESTED_TAG, >> ++ ADVICE_MERGE_CONFLICT, >> + ADVICE_OBJECT_NAME_WARNING, >> + ADVICE_PUSH_ALREADY_EXISTS, >> + ADVICE_PUSH_FETCH_FIRST, > > Here, I assume you're trying to place the new name correctly too. > However, I see that it's in the wrong place. It initially caught my > attention, but then I realize that the list is not sorted. So it's > understandable. > > Maybe you want to sort the list as a preparatory patch in this series > and so we'll avoid this kind of mistakes. > > Of course, this does not deserve a reroll. We can do it in a future > series when the dust settles. I fixed this to put it in the correct order. Thanks, Philippe.
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. 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: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1682%2Fphil-blain%2Fsequencer-conflict-advice-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1682/phil-blain/sequencer-conflict-advice-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1682 Range-diff vs v1: 1: e929d3381cf ! 1: a2ce6fd24c2 sequencer: allow disabling conflict advice @@ Commit message sequencer: allow disabling conflict advice Allow disabling the advice shown when a squencer operation results in a - merge conflict through a new config 'advice.sequencerConflict'. + merge conflict through a new config 'advice.mergeConflict', which is + named generically such that it can be used by other commands eventually. + + 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 + disable the advice, which is more user-friendly. 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 @@ Commit message ## Documentation/config/advice.txt ## @@ Documentation/config/advice.txt: advice.*:: - rmHints:: - In case of failure in the output of linkgit:git-rm[1], - show directions on how to proceed from the current state. -+ sequencerConflict:: -+ Advice shown when a sequencer operation stops because -+ of conflicts. - sequencerInUse:: - Advice shown when a sequencer command is already in progress. - skippedCherryPicks:: + Advice on how to set your identity configuration when + your information is guessed from the system username and + domain name. ++ mergeConflict:: ++ Advice shown when various commands stop because of conflicts. + nestedTag:: + Advice shown if a user attempts to recursively tag a tag object. + pushAlreadyExists:: ## advice.c ## @@ advice.c: static struct { - [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" }, - [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" }, - [ADVICE_RM_HINTS] = { "rmHints" }, -+ [ADVICE_SEQUENCER_CONFLICT] = { "sequencerConflict" }, - [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" }, - [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" }, - [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" }, + [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" }, + [ADVICE_IGNORED_HOOK] = { "ignoredHook" }, + [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" }, ++ [ADVICE_MERGE_CONFLICT] = { "mergeConflict" }, + [ADVICE_NESTED_TAG] = { "nestedTag" }, + [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" }, + [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" }, ## advice.h ## @@ advice.h: enum advice_type { - ADVICE_RESOLVE_CONFLICT, - ADVICE_RM_HINTS, - ADVICE_SEQUENCER_IN_USE, -+ ADVICE_SEQUENCER_CONFLICT, - ADVICE_SET_UPSTREAM_FAILURE, - ADVICE_SKIPPED_CHERRY_PICKS, - ADVICE_STATUS_AHEAD_BEHIND_WARNING, + ADVICE_IGNORED_HOOK, + ADVICE_IMPLICIT_IDENTITY, + ADVICE_NESTED_TAG, ++ ADVICE_MERGE_CONFLICT, + 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_SEQUENCER_CONFLICT, "%s\n", msg); ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, "%s\n", msg); /* * A conflict has occurred but the porcelain * (typically rebase --interactive) wants to take care @@ sequencer.c: static void print_advice(struct repository *r, int 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, ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, + _("after resolving the conflicts, mark the corrected paths\n" + "with 'git add <paths>' or 'git rm <paths>'")); else if (opts->action == REPLAY_PICK) @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, - "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, ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, + _("After resolving the conflicts, mark them with\n" + "\"git add/rm <pathspec>\", then run\n" + "\"git cherry-pick --continue\".\n" @@ sequencer.c: static void print_advice(struct repository *r, int show_hint, - "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, ++ advise_if_enabled(ADVICE_MERGE_CONFLICT, + _("After resolving the conflicts, mark them with\n" + "\"git add/rm <pathspec>\", then run\n" + "\"git revert --continue\".\n" @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'advice from failed revert' ' hint: You can instead skip this commit with "git revert --skip". hint: To abort and get back to the state before "git revert", hint: run "git revert --abort". -+ hint: Disable this message with "git config advice.sequencerConflict false" ++ hint: Disable this message with "git config advice.mergeConflict false" EOF test_commit --append --no-tag "double-add dream" dream dream && test_must_fail git revert HEAD^ 2>actual && @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry- hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort". -+ hint: Disable this message with "git config advice.sequencerConflict false" ++ hint: Disable this message with "git config advice.mergeConflict false" EOF test_must_fail git cherry-pick picked 2>actual && @@ t/t3507-cherry-pick-conflict.sh: test_expect_success 'advice from failed cherry- error: could not apply \$picked... picked hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' -+ hint: Disable this message with \"git config advice.sequencerConflict false\" ++ hint: Disable this message with \"git config advice.mergeConflict false\" EOF test_must_fail git cherry-pick --no-commit picked 2>actual && -: ----------- > 2: 3235542cc6f builtin/am: allow disabling conflict advice