merge --autostash: apply autostash in more cases

This series aims to apply the stash created by 'git merge --autostash' in 2 situations that were not covered by the code:

1. If the merge is fast-forward but the fast-forward operation fails -

    PATCH 3/4

2. If the merge strategy completely fails to handle the merge (exit code 2)
- PATCH 4/4

The first 2 commits are small improvements that I noticed while implementing the other two.

I'm marking it [RFC] because I'm not 100% sure that trying to apply the autostash in 3/4 and 4/4 is actually the best course of action (or if it would be better to call 'save_autostash' instead). That's because:

For 3/4 (fast-forward fails): I'm not sure if 'unpack_trees' (called by 'checkout_fast_forward') is guaranteed to fail atomically, or it might fail mid-way and leave the worktree unclean, in which case it might be better not to apply the autostash, but just save it instead (and tell the user). In the test case I'm adding, it does fail before starting to update the working tree, but I'm not sure if it's always the case.

For 4/4 (merge strategy fails): Same reasoning: I'm not sure if all strategies (and even less user-supplied ones, which are not documented but kind of supported) are guaranteed to 'exit 2' before messing up the working tree.

Philippe Blain (4):
merge: add missing word "strategy" to a message
Documentation: define 'MERGE_AUTOSTASH'
merge: apply autostash if fast-forward fails
merge: apply autostash if merge strategy fails

 Documentation/merge-options.txt |  3 ++-
 builtin/merge.c                 |  4 +++-
 t/t7600-merge.sh                | 19 +++++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

base-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1002%2Fphil-blain%2Fmerge-autostash-ff-fails-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1002/phil-blain/merge-autostash-ff-fails-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1002

gitgitgadget

From: Philippe Blain <[email protected]>

The variable 'best_strategy' holds the name of the merge strategy that resulted in fewer conflicts, if several strategies were tried. When that's the case but the best strategy was not the first one tried, we inform the user which strategy was the "best" one before recreating the merge and leaving the conflicted files in the tree.

This informational message is missing the word "strategy", so it shows something like:

Using the recursive to prepare resolving by hand.

Fix that.

---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a8a843b1f54..74797b6c7a6 100644
@@ -1715,7 +1715,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	else {
 		printf(_("Rewinding the tree to pristine...\n"));
 		restore_state(&head_commit->object.oid, &stash);
-		printf(_("Using the %s to prepare resolving by hand.\n"),
+		printf(_("Using the %s strategy to prepare resolving by hand.\n"),
 			best_strategy);
 		try_merge_strategy(best_strategy, common, remoteheads,
 				   head_commit);

gitgitgadget

From: Philippe Blain <[email protected]>

The documentation for 'git merge --abort' and 'git merge --quit' both mention the special ref 'MERGE_AUTOSTASH', but this ref is not formally defined anywhere. Mention it in the description of the '--autostash' option for 'git merge'.

---
 Documentation/merge-options.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index eb0aabd396f..52565014c15 100644
@@ -154,7 +154,8 @@ endif::git-pull[]
 --autostash::
 --no-autostash::
 	Automatically create a temporary stash entry before the operation
-	begins, and apply it after the operation ends.  This means
+	begins, record it in the special ref `MERGE_AUTOSTASH`
+	and apply it after the operation ends.  This means
 	that you can run the operation on a dirty worktree.  However, use
 	with care: the final stash application after a successful
 	merge might result in non-trivial conflicts.

gitgitgadget

From: Philippe Blain <[email protected]>

Since 'git merge' learned '--autostash' in a03b55530a (merge: teach --autostash option, 2020-04-07), 'cmd_merge', in the fast-forward case, calls 'create_autostash' before calling 'checkout_fast_forward' if '--autostash' is given.

However, if 'checkout_fast_forward' fails, the autostash is not applied to the working tree, nor saved in the stash list, since the code simply calls 'goto done'.

Be more helpful to the user by applying the autostash in that case.

An easy way to test a failing fast-forward is when we are merging a branch that has a tracked file that conflicts with an untracked file in the working tree.

---
 builtin/merge.c  |  1 +
 t/t7600-merge.sh | 11 +++++++++++
 2 files changed, 12 insertions(+)

index 74797b6c7a6..788a6b0cd55 100644
@@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 					  &head_commit->object.oid,
 					  &commit->object.oid,
 					  overwrite_ignore)) {
+			apply_autostash(git_path_merge_autostash(the_repository));
 			ret = 1;
 			goto done;
 		}

index 1cbc9715a81..216113d3537 100755
@@ -122,6 +122,8 @@ test_expect_success 'setup' '
 	c0=$(git rev-parse HEAD) &&
 	cp file.1 file &&
 	git add file &&
+	cp file.1 other &&
+	git add other &&
 	test_tick &&
 	git commit -m "commit 1" &&
 	git tag c1 &&

@@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '
 	test_cmp result.1-5 file
 '
 
+test_expect_success 'failed fast-forward merge with --autostash' '
+	git reset --hard c0 &&
+	git merge-file file file.orig file.5 &&
+	cp file.5 other &&
+	test_must_fail git merge --autostash c1 2>err &&
+	test_i18ngrep "Applied autostash." err &&
+	test_cmp file.5 file
+'
+
 test_expect_success 'octopus merge with --autostash' '
 	git reset --hard c1 &&
 	git merge-file file file.orig file.3 &&

gitgitgadget

I think this one makes sense, as the case that is tested I know fast-forward (aka two-tree switching) would be atomic. If your working tree is already broken (e.g. triggers I/O errors in the middle, some directories having wrong permissions to make them unwritable by you, etc.), you would also see an error from fast-forward and you probably cannot tell these two cases apart, and trying to unstash the local changes may make things even worse, but I suspect that there isn't much you can do about it.

Thanks.

From: Philippe Blain <[email protected]>

Since 'git merge' learned '--autostash' in a03b55530a (merge: teach --autostash option, 2020-04-07), 'cmd_merge', once it is determined that we have to create a merge commit, calls 'create_autostash' if '--autostash' is given.

As explained in a03b55530a, and made more abvious by the tests added in that commit, the autostash is then applied if the merge succeeds, either directly or by committing (after conflict resolution or if '--no-commit' was given), or if the merge is aborted with 'git merge --abort'. In some other cases, like the user calling 'git reset --merge' or 'git merge --quit', the autostash is not applied, but saved in the stash list.

However, there exists a scenario that creates an autostash but does not apply nor save it to the stash list: if the chosen merge strategy completely fails to handle the merge, i.e. 'try_merge_strategy' returns 2.

Apply the autostash in that case also. An easy way to test that is to try to merge more than two commits but explicitely ask for the 'recursive' merge strategy.

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

index 788a6b0cd55..d44c14a21a3 100644
@@ -1709,6 +1709,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			fprintf(stderr, _("Merge with strategy %s failed.\n"),
 				use_strategies[0]->name);
+		apply_autostash(git_path_merge_autostash(the_repository));
 		ret = 2;
 		goto done;
 	} else if (best_strategy == wt_strategy)

index 216113d3537..2ef39d3088e 100755
@@ -732,6 +732,14 @@ test_expect_success 'octopus merge with --autostash' '
 	test_cmp result.1-3-5-9 file
 '
 
+test_expect_success 'failed merge (exit 2) with --autostash' '
+	git reset --hard c1 &&
+	git merge-file file file.orig file.5 &&
+	test_must_fail git merge -s recursive --autostash c2 c3 2>err &&
+	test_i18ngrep "Applied autostash." err &&
+	test_cmp result.1-5 file
+'
+
 test_expect_success 'conflicted merge with --autostash, --abort restores stash' '
 	git reset --hard c3 &&
 	cp file.1 file &&

gitgitgadget

OK.

We know that the recursive that barfs when asked to merge more than one "other" histories will fail before touching the index or the working tree, but I do not think the merge-strategy protocol makes any guarantee as to what half-modified state the strategy would leave the index and the working tree in.

So this change at the first glance looks risky, but at least we have a call to restore_state() before you added an apply_autostash(). The restore_state() call is the same as the one is used between application of a strategy that has failed the same way in the loop, so we should be able to trust it to bring the state back to the pristine (i.e. after autostash removed the local changes).

I think this change makes sense, too.

(I didn't look at 1/4 and 2/4)

Thanks.

I'm not familiar with unpack_trees, but sans that possibility the whole series looks fine to me.

Felipe Contreras