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

[Elijah Newren <newren@gmail.com>]

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