On Mon, Apr 23, 2018 at 10:37 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote: >>> [1] To me the second parameter should be src_index, not dst_index. >>> We're copying entries from _source_ index to "result" and we should >>> also copy extensions from the source index. That line happens to work >>> only when dst_index is the same as src_index, which is the common use >>> case so far. >> >> That makes sense; this sounds like another fix that should be >> submitted. Did you want to submit a patch making that change? Do you >> want me to? > > I did not look careful enough to make sure it was right and submit a > patch. But it sounds like it could be another regression if dst_index > is now not the same as src_index (sorry I didn't look at your whole > stories and don't if dst_index != src_index is a new thing or not). If > dst_index is new, moving extensions from that to result index is > basically no-op, in other words we fail to copy necessary extensions > over. Ah, got it, sounds like it should be included in this patch. A quick summary for you so you don't have to review my whole series: - All callers of unpack_trees() have dst_index == src_index, until my series. - My series makes merge-recursive.c call unpack_trees() with dst_index != src_index (all other callsites unchanged) - In merge-recursive.c, dst_index points to an entirely new index, so yeah we'd be dropping the extensions from the original src_index. I think all the relevant parts of my series as far as this change is concerned is the first few diff hunks at https://public-inbox.org/git/20180419175823.7946-34-newren@xxxxxxxxx/
