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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2020, 22:10 UTC
Message-ID: <xmqqsga0d4f1.fsf@gitster.c.googlers.com>
In-Reply-To: <CABPp-BFtzeOHS=ptgzkrpOL3gwvE6bSaRgxLO1QJ284J08i-mA@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

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

> Would you object if 'ate' was named '_'?

Either is horrible name.

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

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