Re: [PATCH v3 5/9] midx: refactor permutation logic and pack sorting

[Derrick Stolee <stolee@gmail.com>]

On 1/23/2019 4:00 PM, Jonathan Tan wrote:
> Indeed, the sorting of pack_info is moved to after get_sorted_entries().
> Also, pack_perm[old number] = new number, as expected.

Thanks for chiming in with all the detail on the use of 'perm'. This is 
the most confusing part of this code path.

> I think a comment explaining why the perm is needed would be helpful -
> something explaining that the entries were generated using the old pack
> numbers, so we need this mapping to be able to write them using the new
> numbers.

I can put this comment in the struct definition. Is that the right place 
for it?


> As part of this review, I did attempt to make everything use the sorted
> pack_info order, but failed. I got as far as making get_sorted_entries()
> always use start_pack=0 and skip the pack_infos that didn't have the p
> pointer (to match the existing behavior of get_sorted_entries() that
> only operates on new pack_infos being added by add_pack_to_midx), but it
> still didn't work, and I didn't investigate further.

In the previous version, I tried to use 'perm' and the arrays as they 
existed previously, but that led to the bug in that version. Hopefully 
the new code is easier to read and understand. The delta may actually be 
hard to clearly see that it does the same work, but reading the code 
after it is applied looks clearer (to me).

Thanks,
-Stolee