merge: make restore_state() restore staged state too

There are multiple issues at play here: 1) If `git merge` is invoked with staged changes, it should abort without doing any merging, and the user's working tree and index should be the same as before merge was invoked. 2) Merge strategies are responsible for enforcing the index == HEAD requirement. (See 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17) for some history around this.) 3) Merge strategies can bail saying they are not an appropriate handler for the merge in question (possibly allowing other strategies to be used instead). 4) Merge strategies can make changes to the index and working tree, and have no expectation to clean up after themselves, *even* if they bail out and say they are not an appropriate handler for the merge in question. (The `octopus` merge strategy does this, for example.) 5) Because of (3) and (4), builtin/merge.c stashes state before trying merge strategies and restores it afterward. Unfortunately, if users had staged changes before calling `git merge`, builtin/merge.c could do the following: * stash the changes, in order to clean up after the strategies * try all the merge strategies in turn, each of which report they cannot function due to the index not matching HEAD * restore the changes via "git stash apply" But that last step would have the net effect of unstaging the user's changes. Fix this by adding the "--index" option to "git stash apply". While at it, also squelch the stash apply output; we already report "Rewinding the tree to pristine..." and don't need a detailed `git status` report afterwards. Also while at it, switch to using strvec so folks don't have to count the arguments to ensure we avoided an off-by-one error, and so it's easier to add additional arguments to the command. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Jul 23, 2022 at 01:53 UTC aa77ce88ed3a9cfdf70ead9e7385f2a3418d0947
2 files changed +11 -4
builtin/merge.c
+5 -3
@@ -383,20 +383,22 @@ static void reset_hard(const struct object_id *oid, int verbose)
383 static void restore_state(const struct object_id *head,
384 const struct object_id *stash)
385 {
386 - const char *args[] = { "stash", "apply", NULL, NULL };
386 + struct strvec args = STRVEC_INIT;
387
388 if (is_null_oid(stash))
389 return;
390
391 reset_hard(head, 1);
392
393 - args[2] = oid_to_hex(stash);
393 + strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
394 + strvec_push(&args, oid_to_hex(stash));
395
396 /*
397 * It is OK to ignore error here, for example when there was
398 * nothing to restore.
399 */
399 - run_command_v_opt(args, RUN_GIT_CMD);
400 + run_command_v_opt(args.v, RUN_GIT_CMD);
401 + strvec_clear(&args);
402
403 refresh_cache(REFRESH_QUIET);
404 }
t/t6424-merge-unrelated-index-changes.sh
+6 -1
@@ -292,6 +292,7 @@ test_expect_success 'with multiple strategies, recursive or ort failure do not e
292
293 test_seq 0 10 >a &&
294 git add a &&
295 + git rev-parse :a >expect &&
296
297 sane_unset GIT_TEST_MERGE_ALGORITHM &&
298 test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
@@ -299,7 +300,11 @@ test_expect_success 'with multiple strategies, recursive or ort failure do not e
300 grep "Trying merge strategy recursive..." output &&
301 grep "Trying merge strategy ort..." output &&
302 grep "Trying merge strategy octopus..." output &&
302 - grep "No merge strategy handled the merge." output
303 + grep "No merge strategy handled the merge." output &&
304 +
305 + # Changes to "a" should remain staged
306 + git rev-parse :a >actual &&
307 + test_cmp expect actual
308 '
309
310 test_done