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

[Elijah Newren <newren@gmail.com>]

Hi Ben,

On Wed, Mar 1, 2023 at 11:17 PM Ben Humphreys <behumphreys@atlassian.com> wrote:
>
> Hi Junio / Elijah,
>
> On Thu, Jul 21, 2022 at 09:31:44AM -0700, Junio C Hamano wrote:
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > diff --git a/builtin/merge.c b/builtin/merge.c
> > > index f807bf335bd..11bb4bab0a1 100644
> > > --- a/builtin/merge.c
> > > +++ b/builtin/merge.c
> > > @@ -1686,12 +1686,12 @@ 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
> > > +    * and/or recover from merge strategies bailing while leaving the
> > > +    * index and working tree polluted.
> > >      */
> >
> > Makes sense.  We may want to special-case strategies that are known
> > not to have the buggy "leave contaminated tree when bailing out"
> > behaviour to avoid waste.  I expect that more than 99.99% of the
> > time people are feeding a single other commit to ort or recursive,
> > and if these are known to be safe, a lot will be saved by not saving
> > "just in case".  But that can be left for later, after the series
> > solidifies.
>
> This may stretch your memory a bit since the above was many months ago,
> but I'm wondering if you know of any effort since to build the above
> described optimisations?
>
> We've seen when Git 2.38.0 (which introduced this change) is used with
> Bitbucket Server it results in a severe performance regression due to an
> sharp increase in disk and CPU load. Our code that tests the mergeability
> of a pull request is one such affected codepath.
>
> If there isn't any existing efforts to build the optimisations you
> mention above I will have a shot at it.

I've got bad news for you and great news for you.

The bad news: there have not yet been any efforts to build these
optimizations mentioned above.

The great news: the fact that this affects you means you are using
non-bare clones in your mergeability checks, and being forced with
every merge to first checkout the appropriate branch, and pay for the
penalty of updating both the index and the working tree both in that
checkout and during the merge (and perhaps in doing a hard reset
afterwards) in your mergeability check, despite the fact that a
mergeability check really only needs a boolean: "does it merge
cleanly?".  Doing a full worktree-tied merge like this is really
expensive, and while the above Git changes may have made it even more
expensive for you, the real savings comes from switching to a bare
clone and not writing any working tree files or the index.  That's
available via running `git merge-tree`; see the documentation for the
--write-tree option in particular.  GitHub switched over to it last
year and GitLab should be switching soon (or may have already
completed it; I haven't checked in a bit).

You are, of course, more than welcome to build the optimizations Junio
alludes to.  It'd help out various end users.  But for improving
server side operations, I think switching to `git merge-tree` would
provide you _much_ bigger benefits.


Hope that helps,
Elijah