On Tue, Aug 06, 2024 at 11:36:36AM -0400, Taylor Blau wrote:
> As usual, a range-diff is below, but the main changes since last time
> are as follows:
>
> - Documentation improvements to clarify what happens when both an
> incremental- and non-incremental MIDX are both present in a
> repository.
>
> - Commit message typofix on 3/19 to fix an error in one of the
> technical examples.
>
> - Dropped a custom 'local_pack_int_id' in 4/19 to make the remaining
> diff easier to read.
>
> - Minor bugfix in 7/19 where we incorrectly terminated the object
> abbreviation disambiguation step for incremental MIDXs.
>
> - Various additional bits of information in the commit message to
> explain anything that was subtle.
This all looks good to me.
I read over your responses to my previous review, but as you answered
all of my questions I didn't respond to each individually. :)
I looked over the changes in this iteration and they address all the
small points I brought up. In the bigger picture, I do think there are
probably still issues lurking around the global/local pack and objection
position ids. But where you have the series now seems like a good
cut-off point:
- I suspect pack_pos_to_midx() might need to be adjusted. But it's not
an issue that can be triggered until until we support incremental
midx bitmaps. And that should definitely go in its own series (and
preparing is kind of pointless because we don't know what the
correct interface will be until mid-way through that topic).
- I won't be surprised if there is some global/local bug that shakes
out in the long run. But I don't have a clever way of preventing it
or avoiding the need to deal with the distinction. So I think the
best path is forward, and to let the shaking commence.
The most important thing to that the new on-disk files are sane,
since those are hard to walk back later. A simple bug in the code
can always be fixed. Likewise, the changes here are unlikely to
create a bug for anybody using a single unchained midx. So even if
there is a bug, it will only be triggerable for people using the
experimental mode.
-Peff
