Re: [PATCH v4 3/7] merge: do not abort early if one strategy fails to handle the merge

>
> On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > builtin/merge is setup to allow multiple strategies to be specified,
> > and it will find the "best" result and use it.  This is defeated if
> > some of the merge strategies abort early when they cannot handle the
> > merge.  Fix the logic that calls recursive and ort to not do such an
> > early abort, but instead return "2" or "unhandled" so that the next
> > strategy can try to handle the merge.
> >
> > Coming up with a testcase for this is somewhat difficult, since
> > recursive and ort both handle nearly any two-headed merge (there is
> > a separate code path that checks for non-two-headed merges and
> > already returns "2" for them).  So use a somewhat synthetic testcase
> > of having the index not match HEAD before the merge starts, since all
> > merge strategies will abort for that.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  builtin/merge.c                          |  6 ++++--
> >  t/t6402-merge-rename.sh                  |  2 +-
> >  t/t6424-merge-unrelated-index-changes.sh | 16 ++++++++++++++++
> >  t/t6439-merge-co-error-msgs.sh           |  1 +
> >  4 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 13884b8e836..dec7375bf2a 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -754,8 +754,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
> >               else
> >                       clean = merge_recursive(&o, head, remoteheads->item,
> >                                               reversed, &result);
> > -             if (clean < 0)
> > -                     exit(128);
> > +             if (clean < 0) {
> > +                     rollback_lock_file(&lock);
> > +                     return 2;
> > +             }
> >               if (write_locked_index(&the_index, &lock,
> >                                      COMMIT_LOCK | SKIP_IF_UNCHANGED))
> >                       die(_("unable to write %s"), get_index_file());
> > diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
> > index 3a32b1a45cf..772238e582c 100755
> > --- a/t/t6402-merge-rename.sh
> > +++ b/t/t6402-merge-rename.sh
> > @@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
> >       echo >>M one line addition &&
> >       cat M >M.saved &&
> >       git update-index M &&
> > -     test_expect_code 128 git pull --no-rebase . yellow &&
> > +     test_expect_code 2 git pull --no-rebase . yellow &&
> >       test_cmp M M.saved &&
> >       rm -f M.saved
> >  '
> > diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
> > index f35d3182b86..8b749e19083 100755
> > --- a/t/t6424-merge-unrelated-index-changes.sh
> > +++ b/t/t6424-merge-unrelated-index-changes.sh
> > @@ -268,4 +268,20 @@ test_expect_success 'subtree' '
> >       test_path_is_missing .git/MERGE_HEAD
> >  '
> >
> > +test_expect_success 'resolve && recursive && ort' '
> > +     git reset --hard &&
> > +     git checkout B^0 &&
> > +
> > +     test_seq 0 10 >a &&
> > +     git add a &&
> > +
> > +     sane_unset GIT_TEST_MERGE_ALGORITHM &&
> > +     test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
> > +
> > +     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
> > +'

> Ah, re my feedback on 2/7 I hadn't read ahead. This is the test I
> mentioned as failing with the code added in 2/7 if it's tweaked to be
> s/exit 2/exit 0/.
>
> So it's a bit odd to have code added in 2/7 that's tested in 3/7. I
> think this would be much easier to understand if these tests came before
> all these code changes, so then as the changes are made we can see how
> the behavior changes.