On Thu, Aug 01, 2024 at 06:40:26AM -0400, Jeff King wrote: > On Wed, Jul 17, 2024 at 05:12:41PM -0400, Taylor Blau wrote: > > > Now that the MIDX machinery's internals have been taught to understand > > incremental MIDXs over the previous handful of commits, the MIDX > > machinery itself can begin reading incremental MIDXs. > > > > (Note that while the on-disk format for incremental MIDXs has been > > defined, the writing end has not been implemented. This will take place > > in the commit after next.) > > > > The core of this change involves following the order specified in the > > MIDX chain and opening up MIDXs in the chain one-by-one, adding them to > > the previous layer's `->base_midx` pointer at each step. > > This makes it sound like reading a chain file of: > > multi-pack-index-$H1.midx > multi-pack-index-$H2.midx > multi-pack-index-$H3.midx > > will have H1's base_midx pointing to H2. But the design document from > the first patch made me think it went the other way (H1 is the oldest > midx, then H2, then H3). For many things the ordering doesn't matter, > but I'd think the pseudo-pack order would go from the root of the > base_midx walk to the tip. That is, the base_midx pointers go in reverse > chronological order. > > Looking at the code, I think it's doing what I expect. Not sure if I'm > mis-reading what you wrote above, or if it's wrong. The patch message is just plain wrong here. I switched the sentence beginning with "The core of this change involves [...]" to add "in reverse" to clarify what's going on here. > > [...] > > The code itself all looked reasonable. There are a scary number of spots > where we have to do global/local position conversion. It's hard to know > if you got them all. Agreed. If you have ideas to make it less scary, do let me know ;-). Thanks, Taylor
