On Fri, Jul 26, 2019 at 12:32 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> writes:
>
> > Make sure we do the index == head check at the beginning of the merge,
> > and error out immediately if it fails. While we're at it, fix a small
> > leak in the show-the-error codepath.
>
> As the call to repo_index_has_changes() is moved to the very
> beginning of merge_recursive() and merge_trees(), the workhorse of
> the merge machinery, merge_trees_internal(), can lose it.
Is this just a re-summarization (a perfectly good one), or a
suggestion for alternate wording for the commit message?
> > +static int merge_start(struct merge_options *opt, struct tree *head)
> > +{
> > + struct strbuf sb = STRBUF_INIT;
> > +
> > + assert(opt->branch1 && opt->branch2);
>
> This is a new assertion that did not exist in the original, isn't
> it? I do not object to new sensible assertions, and I think these
> two fields must be non-null in a freshly initialized merge_options
> structure, but shouldn't we be discussing if these two fields should
> be non-NULL, and if there are other fields in the same structure
> that we should be adding new assertions on, in a separate step on
> its own?
Good point. The only other one I saw was opt->ancestor, and while it
does have conditions it should satisfy, somewhat surprisingly the
condition is the opposite in merge_trees() vs. merge_recursive(). So
I think this is the only check that makes sense to add to
merge_start(), but I can move that out into a separate patch and add
some words about why just these two.
