Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD
- Date: Tue, 5 Jun 2018 00:14:13 -0700
- From: Elijah Newren <newren@xxxxxxxxx>
- Subject: 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@xxxxxxxxx> wrote:
> Elijah Newren <newren@xxxxxxxxx> 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@xxxxxxxxx>
>> 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.