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.
