diff mbox series

[v3,5/7] merge: make restore_state() restore staged state too

Message ID 91c495c770e3f7f91ae655a503bdd1cd99935e40.1658391391.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix merge restore state | expand

Commit Message

Elijah Newren July 21, 2022, 8:16 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge.c                          | 5 +++--
 t/t6424-merge-unrelated-index-changes.sh | 7 ++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Junio C Hamano July 21, 2022, 4:16 p.m. UTC | #1 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>   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.)

I personally consider this is a bug in `octopus` (and I can do so
without offending anybody, as `octopus` was what I did), but because
the point of having pluggable merge strategies is to allow end users
and third parties write their own.  So save-and-restore dance would
be a prudent approach to the issue than forbidding this "buggy"
behaviour.

> 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"

>  	test_seq 0 10 >a &&
>  	git add a &&
> +	git rev-parse :a >expect &&
> ...
> +	# Changes to "a" should remain staged
> +	git rev-parse :a >actual &&
> +	test_cmp expect actual

Makes sense.

Thanks.
Junio C Hamano July 21, 2022, 4:24 p.m. UTC | #2 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>    * 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"

A tangent that does not make much difference in the end, but I am
finding these lines curious and somewhat annoying.  Why do we have
&nbsp; sandwitching the plain whitespace only on lines that begin
with an asterisk?
Elijah Newren July 21, 2022, 7:52 p.m. UTC | #3 
On Thu, Jul 21, 2022 at 9:24 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >    * 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"
>
> A tangent that does not make much difference in the end, but I am
> finding these lines curious and somewhat annoying.  Why do we have
> &nbsp; sandwitching the plain whitespace only on lines that begin
> with an asterisk?

Eek!  That's nasty.  I was switching back and forth between responding
to emails and coding up fixes, and copy-pasted part of my email into
the commit message.  So, apparently gmail hates me, and I just didn't
notice.

I'll clean these up; I'm rerolling for the "error()" cleanup you
mentioned anyway.
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index 4170c30317e..f807bf335bd 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -383,14 +383,15 @@  static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	const char *args[] = { "stash", "apply", NULL, NULL };
+	const char *args[] = { "stash", "apply", "--index", "--quiet",
+			       NULL, NULL };
 
 	if (is_null_oid(stash))
 		return;
 
 	reset_hard(head, 1);
 
-	args[2] = oid_to_hex(stash);
+	args[4] = oid_to_hex(stash);
 
 	/*
 	 * It is OK to ignore error here, for example when there was
diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
index 3019d030e07..c96649448fa 100755
--- a/t/t6424-merge-unrelated-index-changes.sh
+++ b/t/t6424-merge-unrelated-index-changes.sh
@@ -285,6 +285,7 @@  test_expect_success 'resolve && recursive && ort' '
 
 	test_seq 0 10 >a &&
 	git add a &&
+	git rev-parse :a >expect &&
 
 	sane_unset GIT_TEST_MERGE_ALGORITHM &&
 	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
@@ -292,7 +293,11 @@  test_expect_success 'resolve && recursive && ort' '
 	grep "Trying merge strategy resolve..." output &&
 	grep "Trying merge strategy recursive..." output &&
 	grep "Trying merge strategy ort..." output &&
-	grep "No merge strategy handled the merge." output
+	grep "No merge strategy handled the merge." output &&
+
+	# Changes to "a" should remain staged
+	git rev-parse :a >actual &&
+	test_cmp expect actual
 '
 
 test_done