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

From: Elijah Newren <newren@gmail.com>
Date: Oct 26, 2020, 22:28 UTC
Message-ID: <CABPp-BEToXsTiF+7C12-aZFYFFxR99j1aGmMAi3VXfGUM_OrXA@mail.gmail.com>
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:

Show 17 quoted lines

> >> > +     /*
> >> > +      * 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.

Show 13 quoted lines

> >> > +/* 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.

Show 7 quoted lines

> > 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.

:-)