Re: [PATCH 04/19] merge-recursive: exit early if index != head

>
> Hi Elijah,
>
> On Thu, 25 Jul 2019, Elijah Newren wrote:
>
> > We had a rule to enforce that the index matches head, but it was found
> > at the beginning of merge_trees() and would only trigger when
> > opt->call_depth was 0.  Since merge_recursive() doesn't call
> > merge_trees() until after returning from recursing, this meant that the
> > check wasn't triggered by merge_recursive() until it had first finished
> > all the intermediate merges to create virtual merge bases.  That is a
> > potentially huge amount of computation (and writing of intermediate
> > merge results into the .git/objects directory) before it errors out and
> > says, in effect, "Sorry, I can't do any merging because you have some
> > local changes that would be overwritten."
> >
> > Further, not enforcing this requirement earlier allowed other bugs (such
> > as an unintentional unconditional dropping and reloading of the index in
> > merge_recursive() even when no recursion was necessary), to mask bugs in
> > other callers (which were fixed in the commit prior to this one).
> >
> > Make sure we do the index == head check at the beginning of the merge,
> > and error out immediately if it fails.
>
> Very clear commit message.
>
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 37bb94fb4d..b762ecd7bd 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -3381,21 +3381,14 @@ static int process_entry(struct merge_options *opt,
> >       return clean_merge;
> >  }
> >
> > -int merge_trees(struct merge_options *opt,
> > -             struct tree *head,
> > -             struct tree *merge,
> > -             struct tree *common,
> > -             struct tree **result)
> > +static int merge_trees_internal(struct merge_options *opt,
>
> In other, similar cases, we seem to use the suffix `_1`. Not sure
> whether you want to change that here.

> > +{
> > +     struct strbuf sb = STRBUF_INIT;
> > +
> > +     assert(opt->branch1 && opt->branch2);
> > +
> > +     if (repo_index_has_changes(opt->repo, head, &sb)) {
> > +             err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> > +                 sb.buf);
>
> I know that you did not introduce this leak, but maybe we could slip an
> `strbuf_release(&sb);` in at this point?

> > +             return -1;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void merge_finalize(struct merge_options *opt)
> > +{
> > +     /* Common code for wrapping up merges will be added here later */
> > +}
>
> I was about to comment that this complicates this here diff and that we
> should do this when we need it, but I just peeked into the next patch,
> and it uses it, so I leave this here paragraph only to show that I
> actually reviewed this part, too.
>
> And of course, if we have a `merge_finalize()`, then a `merge_start()`
> does not sound all that bad, either.
>
> The rest looks good to me.