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
