Re: [PATCH v2 5/6] merge: ensure we can actually restore pre-merge state

[Elijah Newren <newren@gmail.com>]

On Tue, Jul 19, 2022 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Merge strategies can fail -- not just have conflicts, but give up and
> > say that they are unable to handle the current type of merge.  However,
> > they can also make changes to the index and working tree before giving
> > up; merge-octopus does this, for example.  Currently, we do not expect
> > the individual strategies to clean up after themselves, but instead
> > expect builtin/merge.c to do so.  For it to be able to, it needs to save
> > the state before trying the merge strategy so it can have something to
> > restore to.  Therefore, remove the shortcut bypassing the save_state()
> > call.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  builtin/merge.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 2dc56fab70b..aaee8f6a553 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> >        * tree in the index -- this means that the index must be in
> >        * sync with the head commit.  The strategies are responsible
> >        * to ensure this.
> > +      *
> > +      * Stash away the local changes so that we can try more than one.
> >        */
>
> The comment explains why we limited the save_state() to avoid wasted
> cycles and SSD wear and tear by looking at the number of strategies.
> But because we are removing the restriction (which I am not 100%
> sure is a good idea), "so that we can try more than one" no longer
> applies as the reason why we run save_state() here.

I should probably change it to "Stash away the local changes so that
we can try more than one and/or recover from merge strategies
bailing".

In regards to the "good idea" side, I don't really like it either, but:

  1. Merge strategies are allowed to make whatever changes they want
to the index and working tree.  They have no requirement to clean up
after themselves.
  2. Merge strategies are allowed to bail and say, "Nevermind, not
only can I not successfully merge this, I can't even leave conflicts
for the users to resolve; I just can't handle it at all.  Try some
other strategy."  (See the "exit 2" codepath of commit 98efc8f3d8
("octopus: allow manual resolve on the last round.", 2006-01-13), for
example)
  3. Merge strategies can bail with the "try some other strategy"
response _after_ mucking with the index and working tree.  octopus
does (again, see the commit mentioned above)
  4. builtin/merge.c previously would _only_ cleanup for merge
strategies if there was more than 1.

This combination is clearly broken.  We need to fix either item 1 or
item 4 (or maybe item 3?).  Since 1 might run into issues with
user-custom merge strategies which may have been written to mimic
octopus and thus rely on the assurances in items 1-3 above, I figured
fixing item 4 was the easiest.