Hi, On Sun, Apr 22, 2018 at 5:38 AM, Duy Nguyen <pclo...@gmail.com> wrote: >> - there's a better, more performant fix or there is some way to actually >> share a split_index between two independent index_state objects. > > A cleaner way of doing this would be something to the line [1] > > move_index_extensions(&o->result, o->dst_index); > > near the end of this function. This could be where we compare the > result index with the source index's shared file and see if it's worth > keeping the shared index or not. Shared index is designed to work with > huge index files though, any operations that go through all index > entries will usually not be cheap. But at least it's safer.
Yeah, it looks like move_index_extensions() currently has no logic for the split_index. Adding it sounds to me like a patch series of its own, and I'm keen to limit additional changes since my patch series already broke things pretty badly once already. >> However, with this fix, all the tests pass both normally and under >> GIT_TEST_SPLIT_INDEX=DareISayYes. Without this patch, when >> GIT_TEST_SPLIT_INDEX is set, my directory rename detection series will fail >> several tests, as reported by SZEDER. > > Yes, the change looks good. Great, thanks for looking over it. > [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? Elijah
