Re: [PATCH v2 1/4] merge-ort: barebones API of new merge strategy with empty implementation

From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Taylor Blau <me@ttaylorr.com>,
	Peter Baumann <peter.baumann@gmail.com>
Subject: Re: [PATCH v2 1/4] merge-ort: barebones API of new merge strategy with empty implementation
Date: Mon, 26 Oct 2020 15:28:24 -0700	[thread overview]
Message-ID: <CABPp-BEToXsTiF+7C12-aZFYFFxR99j1aGmMAi3VXfGUM_OrXA@mail.gmail.com> (raw)
In-Reply-To: <xmqqsga0d4f1.fsf@gitster.c.googlers.com>

On Mon, Oct 26, 2020 at 3:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:

> >> > +     /*
> >> > +      * Additional metadata used by merge_switch_to_result() or future calls
> >> > +      * to merge_inmemory_*().  Not for external use.
> >> > +      */
> >> > +     void *priv;
> >> > +     unsigned ate;
> >>
> >> I'd prefer to see this named not so cute.  Will we hang random
> >> variations of things, or would this be better to be made into a
> >> pointer to union, with an enum that tells us which kind it is in
> >> use?
> >
> > I don't understand the union suggestion.  Both fields are used.
>
> I thought "priv" shouldn't be "anything goes, so it is 'void *'" but
> is probably a "union { ... } priv;" with associated enum next to it
> that tells which one of the possibilities in the union is in effect.

I guess I'm still not following where these "possibilities" come from
or what I could have said that would have suggested possibilities.

priv is a pointer to a private data structure.  The data structure is
defined and used only in merge-ort.c and not exposed to callers.  That
data does need to be passed from merge_incore_*() to
merge_switch_to_result() (or to merge_finalize()).  Since those two
are separate function calls invoked by the caller, and since callers
shouldn't touch any of this data in priv, it's passed as an opaque
field within merge_result.  Thus void*.

> > Would you object if 'ate' was named '_'?
>
> Either is horrible name.

I'll just rip it out, for now.  It isn't relevant to early versions of
merge-ort anyway, and when I re-introduce it with yet another horrible
name, there will at least be more context for others to suggest a
better name for me.

Besides, the variable isn't at all necessary for the algorithm; it
exists solely as a way to catch a small gotcha in API usage of later
versions of merge-ort, which I otherwise didn't have a clean way of
detecting.

> >> > +/* rename-detecting three-way merge with recursive ancestor consolidation. */
> >> > +void merge_inmemory_recursive(struct merge_options *opt,
> >> > +                           struct commit_list *merge_bases,
> >> > +                           struct commit *side1,
> >> > +                           struct commit *side2,
> >> > +                           struct merge_result *result);
> >>
> >> I've seen "incore" spelled as a squashed-into-a-single-word, but not
> >> "in_memory".
> >
> > I can add an underscore.  Or switch to incore.  Preference?
>
> Anything shorter would get my vote.

Sounds good, will do.

> > Yes, your reading is correct.  We don't touch the index (or any index,
> > or any cache_entry) at all.  Among other things, data that can be used
> > to update the index are in the "priv" field.
> >
> > I'll try to add some notes to the file.
>
> Sounds good.

:-)