Re: [PATCH v4 15/17] builtin/repack.c: add cruft packs to MIDX during geometric repack
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-05-19 11:33:15
On Wed, May 18 2022, Taylor Blau wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.