[PATCH v2 2/3] merge: cleanup confusing logic for handling successful merges
- From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Aug 23, 2022, 02:42 UTC
- Message-ID
- <9817d4b19bc21e1b1581af24c7d89fe33b61638d.1661222541.git.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1331.v2.git.1661222541.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>
builtin/merge.c has a loop over the specified strategies, where if they all fail with conflicts, it picks the one with the least number of conflicts.
In the codepath that finds a successful merge, if an automatic commit was wanted, the code breaks out of the above loop, which makes sense. However, if the user requested there be no automatic commit, the loop would continue. That seems weird; --no-commit should not affect the choice of merge strategy, but the code as written makes one think it does. However, since the loop itself embeds "!merge_was_ok" as a condition on continuing to loop, it actually would also exit early if --no-commit was specified, it just exited from a different location.
Restructure the code slightly to make it clear that the loop will immediately exit whenever we find a merge strategy that is successful.
Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/merge.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c index b4253710d19..abf0567b20f 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1693,7 +1693,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (save_state(&stash)) oidclr(&stash); - for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) { + for (i = 0; i < use_strategies_nr; i++) { int ret, cnt; if (i) { printf(_("Rewinding the tree to pristine...\n")); @@ -1718,12 +1718,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) */ if (ret < 2) { if (!ret) { - if (option_commit) { - /* Automerge succeeded. */ - automerge_was_ok = 1; - break; - } + /* + * This strategy worked; no point in trying + * another. + */ merge_was_ok = 1; + best_strategy = use_strategies[i]->name; + break; } cnt = (use_strategies_nr > 1) ? evaluate_result() : 0; if (best_cnt <= 0 || cnt <= best_cnt) { @@ -1737,7 +1738,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * If we have a resulting tree, that means the strategy module * auto resolved the merge cleanly. */ - if (automerge_was_ok) { + if (merge_was_ok && option_commit) { + automerge_was_ok = 1; ret = finish_automerge(head_commit, head_subsumed, common, remoteheads, &result_tree, wt_strategy);
-- gitgitgadget