@@ -167,6 +167,21 @@ static void pack_mark_in_midx(struct string_list_item *item)
item->util = (void*)((uintptr_t)item->util | PACK_IN_MIDX);
}
+static int pack_is_in_midx(struct string_list_item *item)
+{
+ return (uintptr_t)item->util & PACK_IN_MIDX;
+}
+
+static int existing_has_cruft_in_midx(struct existing_packs *existing)
+{
+ struct string_list_item *item;
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ if (pack_is_in_midx(item))
+ return 1;
+ }
+ return 0;
+}
+
static void mark_packs_for_deletion_1(struct string_list *names,
struct string_list *list)
{
@@ -821,26 +836,52 @@ static void midx_included_packs(struct string_list *include,
}
}
- for_each_string_list_item(item, &existing->cruft_packs) {
+ if (existing_has_cruft_in_midx(existing)) {
/*
- * When doing a --geometric repack, there is no need to check
- * for deleted packs, since we're by definition not doing an
- * ALL_INTO_ONE repack (hence no packs will be deleted).
- * Otherwise we must check for and exclude any packs which are
- * enqueued for deletion.
+ * If we had one or more cruft pack(s) present in the
+ * MIDX before the repack, keep them as they may be
+ * required to form a reachability closure if the MIDX
+ * is bitmapped.
*
- * So we could omit the conditional below in the --geometric
- * case, but doing so is unnecessary since no packs are marked
- * as pending deletion (since we only call
- * `mark_packs_for_deletion()` when doing an all-into-one
- * repack).
+ * A cruft pack can be required to form a reachability
+ * closure if the MIDX is bitmapped and one or more of
+ * its selected commits reaches a once-cruft object that
+ * was later made reachable.
*/
- if (pack_is_marked_for_deletion(item))
- continue;
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ /*
+ * When doing a --geometric repack, there is no
+ * need to check for deleted packs, since we're
+ * by definition not doing an ALL_INTO_ONE
+ * repack (hence no packs will be deleted).
+ * Otherwise we must check for and exclude any
+ * packs which are enqueued for deletion.
+ *
+ * So we could omit the conditional below in the
+ * --geometric case, but doing so is unnecessary
+ * since no packs are marked as pending
+ * deletion (since we only call
+ * `mark_packs_for_deletion()` when doing an
+ * all-into-one repack).
+ */
+ if (pack_is_marked_for_deletion(item))
+ continue;
- strbuf_reset(&buf);
- strbuf_addf(&buf, "%s.idx", item->string);
- string_list_insert(include, buf.buf);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "%s.idx", item->string);
+ string_list_insert(include, buf.buf);
+ }
+ } else {
+ /*
+ * Modern versions of Git will write new copies of
+ * once-cruft objects when doing a --geometric repack.
+ *
+ * If the MIDX has no cruft pack, new packs written
+ * during a --geometric repack will not rely on the
+ * cruft pack to form a reachability closure, so we can
+ * avoid including them in the MIDX in that case.
+ */
+ ;
}
strbuf_release(&buf);
@@ -1369,7 +1410,10 @@ int cmd_repack(int argc,
!(pack_everything & PACK_CRUFT))
strvec_push(&cmd.args, "--pack-loose-unreachable");
} else if (geometry.split_factor) {
- strvec_push(&cmd.args, "--stdin-packs");
+ if (existing_has_cruft_in_midx(&existing))
+ strvec_push(&cmd.args, "--stdin-packs");
+ else
+ strvec_push(&cmd.args, "--stdin-packs=follow");
strvec_push(&cmd.args, "--unpacked");
} else {
strvec_push(&cmd.args, "--unpacked");
@@ -724,4 +724,74 @@ test_expect_success 'cruft repack respects --quiet' '
)
'
+test_expect_success 'repack --write-midx excludes cruft where possible' '
+ git init exclude-cruft-when-possible &&
+ (
+ cd exclude-cruft-when-possible &&
+
+ test_commit one &&
+
+ test_commit --no-tag two &&
+ two="$(git rev-parse HEAD)" &&
+ test_commit --no-tag three &&
+ three="$(git rev-parse HEAD)" &&
+ git reset --hard one &&
+
+ git reflog expire --all --expire=all &&
+
+ git repack --cruft -d &&
+ ls $packdir/pack-*.idx | sort >packs.before &&
+
+ git merge $two &&
+ test_commit four &&
+ git repack -d --geometric=2 --write-midx --write-bitmap-index &&
+ ls $packdir/pack-*.idx | sort >packs.after &&
+
+ comm -13 packs.before packs.after >packs.new &&
+ test_line_count = 1 packs.new &&
+
+ git rev-list --objects --no-object-names one..four >expect.raw &&
+ sort expect.raw >expect &&
+
+ git show-index <$(cat packs.new) >actual.raw &&
+ cut -d" " -f2 actual.raw | sort >actual &&
+
+ test_cmp expect actual &&
+
+ test-tool read-midx --show-objects $objdir >actual.raw &&
+ grep "\.pack$" actual.raw | cut -d" " -f1 | sort >actual.objects &&
+ git rev-list --objects --no-object-names HEAD >expect.raw &&
+ sort expect.raw >expect.objects &&
+
+ test_cmp expect.objects actual.objects &&
+
+ cruft="$(basename $(ls $packdir/*.mtimes))" &&
+ grep "^pack-" actual.raw >actual.packs &&
+ ! test_grep "${cruft%.mtimes}.idx" actual.packs
+ )
+'
+
+test_expect_success 'repack --write-midx includes cruft when necessary' '
+ (
+ cd exclude-cruft-when-possible &&
+
+ ls $packdir/pack-*.idx | sort >packs.all &&
+ grep -o "pack-.*\.idx$" packs.all >in &&
+
+ git multi-pack-index write --stdin-packs --bitmap <in &&
+
+ test_commit five &&
+ git repack -d --geometric=2 --write-midx --write-bitmap-index &&
+
+ test-tool read-midx --show-objects $objdir >actual.raw &&
+ grep "\.pack$" actual.raw | cut -d" " -f1 | sort >actual.objects &&
+ git cat-file --batch-all-objects --batch-check="%(objectname)" \
+ >expect.objects &&
+ test_cmp expect.objects actual.objects &&
+
+ grep "^pack-" actual.raw >actual.packs &&
+ test_line_count = "$(($(wc -l <packs.all) + 1))" actual.packs
+ )
+'
+
test_done
In ddee3703b3 (builtin/repack.c: add cruft packs to MIDX during geometric repack, 2022-05-20), repack began adding cruft pack(s) to the MIDX with '--write-midx' to ensure that the resulting MIDX was always closed under reachability in order to generate reachability bitmaps. Suppose you have a once-unreachable object packed in a cruft pack, which later on becomes reachable from one or more objects in a geometrically repacked pack. That once-unreachable object *won't* appear in the new pack, since the cruft pack was specified as neither included nor excluded to 'pack-objects --stdin-packs'. If the bitmap selection process picks one or more commits which reach the once-unreachable objects, commit ddee3703b3 ensures that the MIDX will be closed under reachability. Without it, we would fail to generate a MIDX bitmap. ddee3703b3 alludes to the fact that this is sub-optimal by saying [...] 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. , which is true, but hides an even larger problem. If repositories rarely prune their unreachable objects and/or have many of them, the MIDX must keep track of a large number of objects which bloats the MIDX and slows down object lookup. This is doubly unfortunate because the vast majority of objects in cruft pack(s) are unlikely to be read, but object reads that go through the MIDX have to search through them anyway. This patch causes geometrically-repacked packs to contain a copy of any once-unreachable object(s) with 'git pack-objects --stdin-packs=follow', allowing us to avoid including any cruft packs in the MIDX. This is because a sequence of geometrically-repacked packs that were all generated with '--stdin-packs=follow' are guaranteed to have their union be closed under reachability. Note that you cannot guarantee that a collection of packs is closed under reachability if not all of them were generated with following as above. One tell-tale sign that not all geometrically-repacked packs in the MIDX were generated with following is to see if there is a cruft pack already in the MIDX. If there is, then starting to generate packs with following during geometric repacking won't work, since it's open to the same race as described above. But if you're starting from scratch (e.g., building the first MIDX after an all-into-one '--cruft' repack), then you can guarantee that the union of subsequently generated packs from geometric repacking *is* closed under reachability. Detect when this is the case and avoid including cruft packs in the MIDX where possible. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 78 ++++++++++++++++++++++++++++++++--------- t/t7704-repack-cruft.sh | 70 ++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+), 17 deletions(-)