diff mbox series

[v2,4/6] merge: make restore_state() restore staged state too

Message ID 4a8b7c9e06df36444b94b929b2558f40e3f72e81.1655621424.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix merge restore state | expand

Commit Message

Elijah Newren June 19, 2022, 6:50 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

merge can be invoked with uncommitted changes, including staged changes.
merge is responsible for restoring this state if some of the merge
strategies make changes.  However, it was not restoring staged changes
due to the lack of the "--index" option to "git stash apply".  Add the
option to fix this shortcoming.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

ZheNing Hu July 17, 2022, 4:37 p.m. UTC | #1 
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2022年6月19日周日 14:50写道：
>
> From: Elijah Newren <newren@gmail.com>
>
> merge can be invoked with uncommitted changes, including staged changes.
> merge is responsible for restoring this state if some of the merge
> strategies make changes.  However, it was not restoring staged changes
> due to the lack of the "--index" option to "git stash apply".  Add the
> option to fix this shortcoming.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 8ce4336dd3f..2dc56fab70b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -383,14 +383,14 @@ 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", NULL, NULL };
>
>         if (is_null_oid(stash))
>                 return;
>
>         reset_hard(head, 1);
>
> -       args[2] = oid_to_hex(stash);
> +       args[3] = oid_to_hex(stash);
>
>         /*
>          * It is OK to ignore error here, for example when there was
> --
> gitgitgadget
>

Now git merge (all strategies) can learn to restore origin index state.
LGTM.

ZheNing Hu
Junio C Hamano July 19, 2022, 11:14 p.m. UTC | #2 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> merge can be invoked with uncommitted changes, including staged changes.
> merge is responsible for restoring this state if some of the merge
> strategies make changes.  However, it was not restoring staged changes
> due to the lack of the "--index" option to "git stash apply".  Add the
> option to fix this shortcoming.

Shouldn't this be testable?

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 8ce4336dd3f..2dc56fab70b 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -383,14 +383,14 @@ 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", NULL, NULL };
>  
>  	if (is_null_oid(stash))
>  		return;
>  
>  	reset_hard(head, 1);
>  
> -	args[2] = oid_to_hex(stash);
> +	args[3] = oid_to_hex(stash);
>  
>  	/*
>  	 * It is OK to ignore error here, for example when there was

OK.
Junio C Hamano July 19, 2022, 11:28 p.m. UTC | #3 
Junio C Hamano <gitster@pobox.com> writes:

> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Elijah Newren <newren@gmail.com>
>>
>> merge can be invoked with uncommitted changes, including staged changes.
>> merge is responsible for restoring this state if some of the merge
>> strategies make changes.  However, it was not restoring staged changes
>> due to the lack of the "--index" option to "git stash apply".  Add the
>> option to fix this shortcoming.
>
> Shouldn't this be testable?

I actually take this part (which implied that the change is a good
idea) back.  I think we have clearly documented for the past 17
years that you can have local changes but your index must match the
HEAD before you start your merge.

If "stash apply" vs "stash apply --index" makes any difference,
there is something wrong.  We should be aborting the "git merge"
even before we even start mucking with the working tree and the
index with strategies, no?  I think it is the bug, if this change
makes any difference, to be fixed---we shouldn't be proceeding to
even create a stash with index changes to begin with.

>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>>  builtin/merge.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> index 8ce4336dd3f..2dc56fab70b 100644
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -383,14 +383,14 @@ 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", NULL, NULL };
>>  
>>  	if (is_null_oid(stash))
>>  		return;
>>  
>>  	reset_hard(head, 1);
>>  
>> -	args[2] = oid_to_hex(stash);
>> +	args[3] = oid_to_hex(stash);
>>  
>>  	/*
>>  	 * It is OK to ignore error here, for example when there was
>
> OK.
Elijah Newren July 21, 2022, 1:37 a.m. UTC | #4 
On Tue, Jul 19, 2022 at 4:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> From: Elijah Newren <newren@gmail.com>
> >>
> >> merge can be invoked with uncommitted changes, including staged changes.
> >> merge is responsible for restoring this state if some of the merge
> >> strategies make changes.  However, it was not restoring staged changes
> >> due to the lack of the "--index" option to "git stash apply".  Add the
> >> option to fix this shortcoming.
> >
> > Shouldn't this be testable?

Yes, I will add a test.

> I actually take this part (which implied that the change is a good
> idea) back. I think we have clearly documented for the past 17
> years that you can have local changes but your index must match the
> HEAD before you start your merge.

Actually, we don't enforce that the index must match HEAD in all
cases, as noted in commit 55f39cf755 ("merge: fix misleading pre-merge
check documentation", 2018-06-30).  That commit also pointed out how
the documentation was a bit unclear in this area.

We also apparently fail to enforce the condition in at least two cases
that weren't a valid exception, which I just found while working on a
testcase for this patch.  (Thus, we have one more sordid tale to add
to the saga in commit 9822175d2b ("Ensure index matches head before
invoking merge machinery, round N", 2019-08-17))

However, the failed enforcement and the "valid" special exceptions
aren't too relevant here, so...

> If "stash apply" vs "stash apply --index" makes any difference,
> there is something wrong.  We should be aborting the "git merge"
> even before we even start mucking with the working tree and the
> index with strategies, no?  I think it is the bug, if this change
> makes any difference, to be fixed---we shouldn't be proceeding to
> even create a stash with index changes to begin with.

I agree with you that generally if the index does not match HEAD, then
(A) we should abort the merge, and (B) the working tree and index need
to be left intact when the merge aborts.

But I don't think your conclusion follows from those two items,
because of the last sentence of this comment:

   /*
    * At this point, we need a real merge.  No matter what strategy
    * we use, it would operate on the index, possibly affecting the
    * working tree, and when resolved cleanly, have the desired
    * tree in the index -- this means that the index must be in
    * sync with the head commit.  The strategies are responsible
    * to ensure this.
    */

Due to this requirement, if a user has staged changes before starting
the merge, builtin/merge.c will:

   * stash the changes
   * try all the merge strategies in turn, each of which report they
cannot function due to index not matching HEAD
   * restore the changes via "git stash apply"

This sequence has the net effect of not quite cleanly aborting the
merge -- it also unstashes the user's changes.

One way to fix this problem is the simple patch I proposed.  An
alternative fix would be to rip out the extra code from all the merge
strategies that enforces the index matches HEAD requirement, and then
adding enforcement of that condition early in builtin/merge.c.  That
alternative fix probably would have saved us from a lot of the
headache detailed in commit 9822175d2b above, but it may also make
recursive and ort a bit slower (which had relied on unpack-trees to do
some of this checking, and thus they'd have some redundant checks).
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index 8ce4336dd3f..2dc56fab70b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -383,14 +383,14 @@  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", NULL, NULL };
 
 	if (is_null_oid(stash))
 		return;
 
 	reset_hard(head, 1);
 
-	args[2] = oid_to_hex(stash);
+	args[3] = oid_to_hex(stash);
 
 	/*
 	 * It is OK to ignore error here, for example when there was