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 <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/merge.c b/builtin/merge.c
index f7c92c0e64ffa32866f53d90793f9d4b4d11be25..3e401de5cbecefc34d406e6d096cc2556f914219 100644 (file)
--- 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);