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

[Elijah Newren <newren@gmail.com>]

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.