Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2706 bytes --]

On Mon, Apr 15, 2024 at 08:41:25AM +0200, Patrick Steinhardt wrote:
> In 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12)
> we have introduced multi-pack verbatim reuse of objects. This series has
> introduced a new BTMP chunk, which encodes information about bitmapped
> objects in the multi-pack index. Starting with dab60934e3 (pack-bitmap:
> pass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use
> this information to figure out objects which we can reuse from each of
> the packfiles.
> 
> One thing that we glossed over though is backwards compatibility with
> repositories that do not yet have BTMP chunks in their multi-pack index.
> In that case, `nth_bitmapped_pack()` would return an error, which causes
> us to emit a warning followed by another error message. These warnings
> are visible to users that fetch from a repository:
> 
> ```
> $ git fetch
> ...
> remote: error: MIDX does not contain the BTMP chunk
> remote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse
> remote: Enumerating objects: 40, done.
> remote: Counting objects: 100% (40/40), done.
> remote: Compressing objects: 100% (39/39), done.
> remote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)
> ...
> ```
> 
> While the fetch succeeds the user is left wondering what they did wrong.
> Furthermore, as visible both from the warning and from the reuse stats,
> pack-reuse is completely disabled in such repositories.
> 
> What is quite interesting is that this issue can even be triggered in
> case `pack.allowPackReuse=single` is set, which is the default value.
> One could have expected that in this case we fall back to the old logic,
> which is to use the preferred packfile without consulting BTMP chunks at
> all. But either we fail with the above error in case they are missing,
> or we use the first pack in the multi-pack-index. The former case
> disables pack-reuse altogether, whereas the latter case may result in
> reusing objects from a suboptimal packfile.
> 
> Fix this issue by partially reverting the logic back to what we had
> before this patch series landed. Namely, in the case where we have no
> BTMP chunks or when `pack.allowPackReuse=single` are set, we use the
> preferred pack instead of consulting the BTMP chunks.
> 
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Junio, it would be great if we could still land this fix in Git v2.45
given that it is addressing a regression in Git v2.44. This of course
assumes that the current version of this patch looks good to Taylor.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]