Miscellaneous merge fixes

While working on other things, I noticed a few miscellaneous issues in builtin/merge.c. Here's a few small fixes to address them.

Elijah Newren (2):
merge: only apply autostash when appropriate
merge: avoid searching for strategies with fewer than 0 conflicts

 builtin/merge.c  | 19 ++++++++++++++-----
 t/t7600-merge.sh |  9 +++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

base-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1331%2Fnewren%2Fmisc-merge-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1331/newren/misc-merge-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1331

gitgitgadget

From: Elijah Newren <[email protected]>

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 looking for a "better" strategy. Since it had just found a strategy with 0 conflicts though, and it is not possible to have fewer than 0 conflicts, the continuing search is guaranteed to be futile.

While searching additional strategies won't cause problems other than wasting energy, it is wasteful. Avoid searching for other strategies with fewer than 0 conflicts.

---
 builtin/merge.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

index b4253710d19..f04100ce0da 100644
@@ -1718,12 +1718,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		if (ret < 2) {
 			if (!ret) {
-				if (option_commit) {
+				if (option_commit)
 					/* Automerge succeeded. */
 					automerge_was_ok = 1;
-					break;
-				}
-				merge_was_ok = 1;
+				else
+					/* Merge good, but let user commit */
+					merge_was_ok = 1;
+				/*
+				 * This strategy worked; no point in trying
+				 * another.
+				 */
+				best_strategy = wt_strategy;
+				break;
 			}
 			cnt = (use_strategies_nr > 1) ? evaluate_result() : 0;
 			if (best_cnt <= 0 || cnt <= best_cnt) {

gitgitgadget

From: Elijah Newren <[email protected]>

If a merge failed and we are leaving conflicts in the working directory for the user to resolve, we should not attempt to apply any autostash.

Further, if we fail to apply the autostash (because either the merge failed, or the user requested --no-commit), then we should instruct the user how to apply it later.

Add a testcase verifying we have corrected this behavior.

---
 builtin/merge.c  | 5 ++++-
 t/t7600-merge.sh | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

index f7c92c0e64f..b4253710d19 100644
@@ -503,7 +503,8 @@ static void finish(struct commit *head_commit,
 	/* Run a post-merge hook */
 	run_hooks_l("post-merge", squash ? "1" : "0", NULL);
 
-	apply_autostash(git_path_merge_autostash(the_repository));
+	if (new_head)
+		apply_autostash(git_path_merge_autostash(the_repository));
 	strbuf_release(&reflog_message);
 }
 

@@ -1781,6 +1782,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			"stopped before committing as requested\n"));
 	else
 		ret = suggest_conflicts();
+	if (autostash)
+		printf(_("When finished, apply stashed changes with `git stash pop`\n"));
 
 done:
 	if (!automerge_was_ok) {

index f0f6fda150b..6a39bc72122 100755
@@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '
 	test_cmp expect actual
 '
 
+test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '
+	git reset --hard c3 &&
+	>unrelated &&
+	git add unrelated &&
+	test_must_fail git merge --squash c7 --autostash >out 2>err &&
+	! grep "Applying autostash resulted in conflicts." err &&
+	grep "When finished, apply stashed changes with \`git stash pop\`" out
+'
+
 test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '
 	git config commit.cleanup scissors &&
 	git reset --hard c3 &&

gitgitgadget

Yep; will fix.

Let's imagine "git merge -s ours -s ort X", both of which resolve the merge cleanly but differently.

The "best_cnt" thing tries to find the last strategy with the lowest score from evaluate_result(), so strictly speaking I think this changes the behaviour. Am I mistaken?

When two strategies 'ours' and 'ort' resolved a given merge cleanly (but differently), we would have taken the result from 'ort' in the original code, but now we stop at seeing that 'ours' resolved the merge cleanly and use its result.

Yes, you are.

Though, to be fair, my commit message is wrong too.

I'll explain below...

No, the original code would not have taken 'ort'. You have overlooked the part of the code immediately above these two lines:

    if (ret < 2) {
        if (!ret) {
            if (option_commit) {
                /* Automerge succeeded. */
                automerge_was_ok = 1;
                break;
            }
        merge_was_ok = 1;
        }

In particular, the "break" is key. If the first strategy succeeds (and the user did not specify --no-commit), then the loop will hit that break statement and bail out of the loop, preventing any other merge strategy from being tried. In contrast, if the user did specify --no-commit and the previous strategy succeeded, then we will continue the loop. That seems rather inconsistent, since --no-commit should not affect the choice of strategy.

However, I missed two things in my reading. You are correct that I missed the "<=" as opposed to "<" when I wrote my commit message, though that turns out to not matter much due to the second thing. The second thing I missed was part of the code at the beginning of the loop:

    for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {

In particular, that "!merge_was_ok" check means that the code would bail early whenever ret was 0, regardless of whether --no-commit was passed. So, my code turns out to not only leave behavior alone, but also doesn't count as an optimization either -- it's simply a half-baked cleanup. If I also get rid of the now-redundant "!merge_was_ok" check in the for loop and fix my commit message, then I think it'd be a fully-baked code cleanup.

I'll submit an update.

I noticed that much but then would "git merge --no-commit -s A -s B" have the issue?

Yeah, exactly.

Ahhhh, that explains it. We leave as soon as we find merge_was_ok so this patch is not necessary. There is nothing to fix. The original was fine as-is.

Thanks.

[...]

Right, there was no bug or even optimization opportunity in the original, it was just confusing...as evidenced by the fact that we both misread the code.

The fact that we both misread the code, though, suggests there is something to fix: code readability.

While working on other things, I noticed a few miscellaneous issues in builtin/merge.c. Here's a few small fixes to address them.

Elijah Newren (3):
merge: only apply autostash when appropriate
merge: cleanup confusing logic for handling successful merges
merge: small code readability improvement

 builtin/merge.c  | 25 +++++++++++++++----------
 t/t7600-merge.sh |  9 +++++++++
 2 files changed, 24 insertions(+), 10 deletions(-)

base-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1331%2Fnewren%2Fmisc-merge-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1331/newren/misc-merge-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1331

Range-diff vs v1:

1: 92840bf6378 ! 1: 610b8d089db merge: only apply autostash when appropriate

     @@ t/t7600-merge.sh: test_expect_success 'merge --squash c3 with c7' '
       	test_cmp expect actual
       '
       
     -+test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '
     ++test_expect_success 'merge --squash --autostash conflict does not attempt to apply autostash' '
      +	git reset --hard c3 &&
      +	>unrelated &&
      +	git add unrelated &&

2: 5657a05e763 ! 2: 9817d4b19bc merge: avoid searching for strategies with fewer than 0 conflicts
@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- merge: avoid searching for strategies with fewer than 0 conflicts
+ 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.
+ 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 looking for a "better" strategy. Since it had just
- found a strategy with 0 conflicts though, and it is not possible to
- have fewer than 0 conflicts, the continuing search is guaranteed to be
- futile.
+ 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.

- While searching additional strategies won't cause problems other than
- wasting energy, it is wasteful. Avoid searching for other strategies
- with fewer than 0 conflicts.
+ Restructure the code slightly to make it clear that the loop will
+ immediately exit whenever we find a merge strategy that is successful.

## builtin/merge.c ##
+@@ builtin/merge.c: 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"));
@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
*/
if (ret < 2) {
if (!ret) {
- if (option_commit) {
-+ if (option_commit)
- /* Automerge succeeded. */
- automerge_was_ok = 1;
+- /* Automerge succeeded. */
+- automerge_was_ok = 1;
- break;
- }
-- merge_was_ok = 1;
-+ else
-+ /* Merge good, but let user commit */
-+ merge_was_ok = 1;
+ /*
+ * This strategy worked; no point in trying
+ * another.
+ */
-+ best_strategy = wt_strategy;
+ 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) {
+@@ builtin/merge.c: 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);
-: ----------- > 3: 88173eba0b9 merge: small code readability improvement

gitgitgadget

From: Elijah Newren <[email protected]>

If a merge failed and we are leaving conflicts in the working directory for the user to resolve, we should not attempt to apply any autostash.

Further, if we fail to apply the autostash (because either the merge failed, or the user requested --no-commit), then we should instruct the user how to apply it later.

Add a testcase verifying we have corrected this behavior.

---
 builtin/merge.c  | 5 ++++-
 t/t7600-merge.sh | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

index f7c92c0e64f..b4253710d19 100644
@@ -503,7 +503,8 @@ static void finish(struct commit *head_commit,
 	/* Run a post-merge hook */
 	run_hooks_l("post-merge", squash ? "1" : "0", NULL);
 
-	apply_autostash(git_path_merge_autostash(the_repository));
+	if (new_head)
+		apply_autostash(git_path_merge_autostash(the_repository));
 	strbuf_release(&reflog_message);
 }
 

@@ -1781,6 +1782,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			"stopped before committing as requested\n"));
 	else
 		ret = suggest_conflicts();
+	if (autostash)
+		printf(_("When finished, apply stashed changes with `git stash pop`\n"));
 
 done:
 	if (!automerge_was_ok) {

index f0f6fda150b..7c3f6ed9943 100755
@@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '
 	test_cmp expect actual
 '
 
+test_expect_success 'merge --squash --autostash conflict does not attempt to apply autostash' '
+	git reset --hard c3 &&
+	>unrelated &&
+	git add unrelated &&
+	test_must_fail git merge --squash c7 --autostash >out 2>err &&
+	! grep "Applying autostash resulted in conflicts." err &&
+	grep "When finished, apply stashed changes with \`git stash pop\`" out
+'
+
 test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '
 	git config commit.cleanup scissors &&
 	git reset --hard c3 &&

gitgitgadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/merge.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

index b4253710d19..abf0567b20f 100644
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 builtin/merge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index abf0567b20f..5900b81729d 100644
@@ -1708,7 +1708,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);
 		/*

@@ -1723,12 +1723,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;
 			}
 		}

gitgitgadget

Sorry, forgot to include a high level summary:

Changes since v1:
* Fix play/apply typo, spotted by Eric
* Almost completely rewrote patch 2; it's now merely a "code cleanup"
* Added a third patch, just a very minor code simplification

I was skeptical about patch 2 before I saw it, but removal of !merge_was_ok in the loop condition alone is very well worth it.

As to patch 3, I do not think it makes huge difference either way, but I've queued it anyway.

Thanks.