From 5b1d30cabfcdd7cb1dc990f22980af32aa6986a9 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 23 Aug 2022 02:42:20 +0000 Subject: [PATCH 1/2] merge: cleanup confusing logic for handling successful merges 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 Signed-off-by: Junio C Hamano --- builtin/merge.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index f7c92c0e64..3e401de5cb 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1692,7 +1692,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")); @@ -1717,12 +1717,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) { @@ -1736,7 +1737,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); From ae15fd4116212d8f2168e321594ee3acc2f50a5f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 23 Aug 2022 02:42:21 +0000 Subject: [PATCH 2/2] merge: small code readability improvement After our loop through the selected strategies, we compare best_strategy to wt_strategy. This is fine, but the fact that the code setting best_strategy sets it to use_strategies[i]->name requires a little bit of extra checking to determine that at the time of setting, that's the same as wt_strategy. Just setting best_strategy to wt_strategy makes it a little easier to verify what the loop is doing, at least for this reader. Further, use_strategies[i]->name is used in a number of places, where we could just use wt_strategy. The latter takes less time for this reader to parse (one variable name instead of three), so just use wt_strategy to make the code slightly faster for human readers to parse. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/merge.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 3e401de5cb..78af390ba3 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1707,7 +1707,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) */ wt_strategy = use_strategies[i]->name; - ret = try_merge_strategy(use_strategies[i]->name, + ret = try_merge_strategy(wt_strategy, common, remoteheads, head_commit); /* @@ -1722,12 +1722,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * another. */ merge_was_ok = 1; - best_strategy = use_strategies[i]->name; + best_strategy = wt_strategy; break; } cnt = (use_strategies_nr > 1) ? evaluate_result() : 0; if (best_cnt <= 0 || cnt <= best_cnt) { - best_strategy = use_strategies[i]->name; + best_strategy = wt_strategy; best_cnt = cnt; } }