Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD

> On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> `git merge-recursive` does a three-way merge between user-specified trees
>>> base, head, and remote.  Since the user is allowed to specify head, we can
>>> not necesarily assume that head == HEAD.
>>>
>>> We modify index_has_changes() to take an extra argument specifying the
>>> tree to compare the index to.  If NULL, it will compare to HEAD.  We then
>>> use this from merge-recursive to make sure we compare to the
>>> user-specified head.
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>
>>> I'm really unsure where the index_has_changes() declaration should go;
>>> I stuck it in tree.h, but is there a better spot?
>>
>> I think I saw you tried to lift an assumption that we're always
>> working on the_index in a separate patch recently.  Should that
>> logic apply also to this part of the codebase?  IOW, shouldn't
>> index_has_changes() take a pointer to istate (as opposed to a
>> function that uses the implicit the_index that should be named as
>> "cache_has_changes()" or something?)
>>
>> I tend to think this function as part of the larger read-cache.c
>> family whose definitions are in cache.h and accompanied by macros
>> that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we
>> were to move it elsewhere, I'd keep the header part as-is and
>> implementation to read-cache.c to keep it together with the family,
>> but I do not see a huge issue with the current placement, either.
>
> That's good point; the goal to lift assumptions on the_index should
> probably also apply here.  I'll make the change.
> (And it was actually Duy's patch that I was reviewing, but close
> enough.)   I'll take a look at moving it to read-cache.c as well.