midx: honor custom bases for incremental writes

t/lib-midx.sh +0 -0

index e38c609604c..b522dbdb0f4 100644

Quoted 9 lines · Taylor Blau

t/lib-midx.sh +1 -0

index e38c609604c..b522dbdb0f4 100644
@@ -34,3 +34,9 @@ compare_results_with_midx () {
 		midx_git_two_modes "cat-file --batch-all-objects --batch-check --unordered" sorted
 	'
 }
+

t/lib-midx.sh compare_results_with_midx () { · line 42

Quoted 5 lines

t/lib-midx.sh +5 -0

+nth_line() {
+	local n="$1"
+	shift
+	awk "NR==$n" "$@"
+}

It feels a bit weird to have such a general function in "lib-midx.sh", but so be it.

The 'multi-pack-index' builtin parses '--base' for incremental writes, but the normal write path does not pass that value through to write_midx_file().

As a result, something like:

    $ git multi-pack-index write --incremental --base=<base>

behaves as if no custom base had been given (unless the caller used the '--stdin-packs' path).

Thread the parsed base through write_midx_file(), and update the repack caller to pass NULL for the new argument where no custom base selection is needed.

This exposes a pre-existing problem in incremental writes with custom
bases: the writer skips packs from the full existing MIDX chain, even when the caller selected an older base or no base at all.

The affected t5334 cases fail while trying to write MIDX bitmaps. The detached layer omits packs above the selected base, and thus the resulting MIDX does not have a reachability closure, making it impossible to generate reachability bitmaps.

Mark those tests as expected failures accordingly. The following commit will fix the broken behavior and restore these tests.

builtin/multi-pack-index.c +0 -0

index 00ffb36394d..949bfa796b2 100644

midx-write.c +0 -0

index 561e9eedc0e..aa438775ebd 100644
@@ -1850,12 +1850,14 @@ static int write_midx_internal(struct write_midx_opts *opts)
 int write_midx_file(struct odb_source *source,
 		    const char *preferred_pack_name,
 		    const char *refs_snapshot,

Quoted 7 lines

Thread the parsed base through write_midx_file(), and update the repack caller to pass NULL for the new argument where no custom base selection is needed.

This exposes a pre-existing problem in incremental writes with custom
bases: the writer skips packs from the full existing MIDX chain, even when the caller selected an older base or no base at all.

So as the "normal" write path didn't honor this option at all, I assume this bug here then refers to "--stdin-packs" being broken?

t/t5334-incremental-multi-pack-index.sh test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file · line 137

Quoted 12 lines

t/t5334-incremental-multi-pack-index.sh +6 -1

@@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
 		--no-write-chain-file --base=none)" &&
 
 	test_cmp "$midx_chain.bak" "$midx_chain" &&
-	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
+
+	echo "$layer" >"$midx_chain" &&
+	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
+	test_grep "^$(git rev-parse 2.2) " midx.objects &&
+	cp "$midx_chain.bak" "$midx_chain"
 '

Would it make sense to also test for an object from the first MIDX layer to be included? Otherwise we don't really assert that all layers are included in the new MIDX.

Quoted 16 lines · Patrick Steinhardt

t/t5334-incremental-multi-pack-index.sh test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file · line 137

Quoted 12 lines

t/t5334-incremental-multi-pack-index.sh +6 -1

@@ -128,19 +128,33 @@ test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file
 		--no-write-chain-file --base=none)" &&
 
 	test_cmp "$midx_chain.bak" "$midx_chain" &&
-	test_path_is_file "$midxdir/multi-pack-index-$layer.midx"
+	test_path_is_file "$midxdir/multi-pack-index-$layer.midx" &&
+
+	echo "$layer" >"$midx_chain" &&
+	test-tool read-midx --show-objects "$objdir" "$layer" >midx.objects &&
+	test_grep "^$(git rev-parse 2.2) " midx.objects &&
+	cp "$midx_chain.bak" "$midx_chain"
 '

Would it make sense to also test for an object from the first MIDX layer to be included? Otherwise we don't really assert that all layers are included in the new MIDX.

I don't think that is necessary in this case, but let me know if I am missing something below.

The new layer is written with '--bitmap', and '--base=none' means that there is no base layer from which the bitmap can inherit objects. Since 1.2 is an ancestor of 2.2, writing a bitmap for the new layer already requires that it contain 1.2 and the rest of its reachable history. Otherwise bitmap generation would fail with the missing-closure error before we reached the assertion.

Checking 2.2 confirms that an object from the old tip was pulled into the new layer; the successful bitmap write already establishes that its objects from the earlier layer were pulled in, too.

midx-write.c +1 -0

index aa438775ebd..c50fdb5c6d1 100644
@@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
 static int should_include_pack(const struct write_midx_context *ctx,
 			       const char *file_name)
 {
+	struct multi_pack_index *m = ctx->m;

midx-write.c +0 -0

 	else if (ctx->to_include &&
 		 !string_list_has_string(ctx->to_include, file_name))

t/t5334-incremental-multi-pack-index.sh +1 -1

index 69e96bf8d93..84ff6120978 100755
@@ -119,7 +119,7 @@ test_expect_success 'write MIDX layer with --base without --no-write-chain-file'
 	test_grep "cannot use --base without --no-write-chain-file" err
 '
 
-test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file' '
+test_expect_success 'write MIDX layer with --base=none and --no-write-chain-file' '
 	test_commit base-none &&
 	git repack -d &&
 

Quoted 14 lines · Taylor Blau

midx-write.c +4 -1

index aa438775ebd..c50fdb5c6d1 100644
@@ -133,8 +133,17 @@ static uint32_t midx_pack_perm(struct write_midx_context *ctx,
 static int should_include_pack(const struct write_midx_context *ctx,
 			       const char *file_name)
 {
+	struct multi_pack_index *m = ctx->m;
 	/*
-	 * Note that at most one of ctx->m and ctx->to_include are set,
+	 * When writing incrementally, ctx->m may contain layers above
+	 * the selected base MIDX, which must be included in the new
+	 * layer.

midx-write.c static uint32_t midx_pack_perm(struct write_midx_context *ctx, · line 146

Quoted 5 lines

midx-write.c +5 -0

+	 */
+	if (ctx->incremental)
+		m = ctx->base_midx;
+	/*
+	 * Note that at most one of m and ctx->to_include are set,

Is that true? With "--stdin-packs --incremental --base=<foo>" I'd expect that we have both set now.

Okay, previously we were always checking against ctx->m, so we would exclude packs that are contained in the current MIDX. And that includes the case where parts of the current MIDX are supposed to be thrown away because we want to write a new layer that excludes all layers starting at the base.

This is fixed by instead always comparing against the base MIDX in case "--incremental" was passed. When the user passes "--base=none" we don't have any base, and consequently we'd include all packs. Otherwise, we'll exclude all packs that are already covered by our base, but include all the other ones.

That feels sensible to me.

ctx->to_include can indeed be non-NULL.
...
The filtering still does the right thing: packs covered by the selected base are excluded, and the remaining packs are checked against the stdin list. But the comment is wrong, so I'll fix it.

Quoted 5 lines · Taylor Blau

ctx->to_include can indeed be non-NULL.
...
The filtering still does the right thing: packs covered by the selected base are excluded, and the remaining packs are checked against the stdin list. But the comment is wrong, so I'll fix it.

Has anything happened since we saw this comment on Aug 13th?

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Taylor Blau

ctx->to_include can indeed be non-NULL.
...
The filtering still does the right thing: packs covered by the selected base are excluded, and the remaining packs are checked against the stdin list. But the comment is wrong, so I'll fix it.

Has anything happened since we saw this comment on Aug 13th?

Not until you sent this message ;-).

I had a small reroll prepped that I had meant to send a couple of weeks ago but never got around to doing so. When I looked at it just now, I found that I wasn't quite satisfied with the range-diff in that the resulting block comment was somewhat confusing.

Instead of sending a new round immediately, let me instead share the comment that I wrote instead. Patrick (or others): does this comment seem clear, or do you think there are ways to tighten it up further?

--- 8< ---

midx-write.c +0 -2

index 66da608370..ff94076104 100644
@@ -143,15 +143,31 @@ static int should_include_pack(const struct write_midx_context *ctx,
 		m = ctx->base_midx;
 
 	/*
-	 * Note that m and ctx->to_include may both be set,
-	 * so we are testing midx_contains_pack() and

midx-write.c +11 -0

+	 * When ctx->m is NULL we are writing a new MIDX without reusing
+	 * any packs from the previous layer(s). In that case, we care
+	 * that both:
+	 *
+	 *   - the new layer's base MIDX (ctx->base_midx) does not
+	 *     already contain the pack we are considering, or the new
+	 *     layer has no base (i.e., it is a non-incremental MIDX)
+	 *
+	 *   - the pack appears in ctx->to_include, or ctx->to_include
+	 *     is NULL, meaning that we can include any pack provided
+	 *     the above condition is met.