On Mon, Oct 26, 2020 at 3:10 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Elijah Newren <newren@xxxxxxxxx> 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.
:-)
