On Mon, Apr 23, 2018 at 10:37 AM, Duy Nguyen [off-list ref] wrote:

quoted

quoted

[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@gmail.com/