Code review comment for lp:~spiv/bzr/insert-stream-check-chk-root

Revision history for this message
Andrew Bennetts (spiv) wrote :

This is a partial fix for bug 406687, and an incremental step towards a full fix. It adds a check to RepositoryPackCollection._commit_write_group that verifies every revision added by that write group has a corresponding inventory, and that for every corresponding inventory plus any present parent inventories that the chk root entries are present ­— and presence in a fallback does not count. At the same time ghost parent inventories are still allowed. This fix is sufficient at least to catch some bugs we had in the past where no chk records were transferred at all in some cases.

(The full fix, which I am working on, will involve checking that all relevant chk records are present, and that the text versions named by those relevant records are also present.)

I experimented with not requiring chk root records in a stacked repository for inventories that have not changed, because in principle they aren't required (just comparing the key names is enough to show that the delta between those revs is empty), but bzr currently fails on such a repository, so allowing this would allow data incompatible with e.g. 2.0rc1, which I think is undesirable.

I am a little worried about the possible performance impact of this change. I don't think the code is particularly wasteful (except perhaps in holding collections of keys that are larger or more long-lived than strictly necessary), but it is fundamentally more work to perform on every 2a repository write.

« Back to merge proposal