On Wed, May 18 2022, Taylor Blau wrote:
> When using cruft packs, the following race can occur when a geometric
> repack that writes a MIDX bitmap takes place afterwords:
>
> - First, create an unreachable object and do an all-into-one cruft
> repack which stores that object in the repository's cruft pack.
> - Then make that object reachable.
> - Finally, do a geometric repack and write a MIDX bitmap.
>
> Assuming that we are sufficiently unlucky as to select a commit from the
> MIDX which reaches that object for bitmapping, then the `git
> multi-pack-index` process will complain that that object is missing.
>
> The reason is because we don't include cruft packs in the MIDX when
> doing a geometric repack. Since the "make that object reachable" doesn't
> necessarily mean that we'll create a new copy of that object in one of
> the packs that will get rolled up as part of a geometric repack, it's
> possible that the MIDX won't see any copies of that now-reachable
> object.
>
> Of course, it's desirable to avoid including cruft packs in the MIDX
> because it causes the MIDX to store a bunch of objects which are likely
> to get thrown away. But excluding that pack does open us up to the above
> race.
>
> This patch demonstrates the bug, and resolves it by including cruft
> packs in the MIDX even when doing a geometric repack.
>
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> ---
> builtin/repack.c | 19 +++++++++++++++++--
> t/t5329-pack-objects-cruft.sh | 26 ++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 36d1f03671..e9e3a2b4e3 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -23,6 +23,7 @@
> #define PACK_CRUFT 4
>
> #define DELETE_PACK 1
> +#define CRUFT_PACK 2
>
> static int pack_everything;
> static int delta_base_offset = 1;
> @@ -161,8 +162,11 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
> if ((extra_keep->nr > 0 && i < extra_keep->nr) ||
> (file_exists(mkpath("%s/%s.keep", packdir, fname))))
> string_list_append_nodup(fname_kept_list, fname);
> - else
> - string_list_append_nodup(fname_nonkept_list, fname);
> + else {
> + struct string_list_item *item = string_list_append_nodup(fname_nonkept_list, fname);
Nit: very long line, and we end up with {} just on the else, not the if.
