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

From: Elijah Newren <newren@gmail.com>
Date: Jul 21, 2022, 01:37 UTC
Message-ID: <CABPp-BEfpwmEcR3TR8xu_+1TQfOvOTQceNE7-H76rsno6TmFPg@mail.gmail.com>
In-Reply-To: <xmqq7d4865t6.fsf@gitster.g>

On Tue, Jul 19, 2022 at 4:28 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

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

Show 6 quoted lines

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