midx-write: include packs above custom incremental base

The previous commit made '--base' take effect on the normal incremental
write path, which exposed an existing assumption in our helper function
`should_include_pack()`, which is that any pack already present in
`ctx->m` was skipped.

That is only correct for non-incremental writes. For incremental writes,
`ctx->base_midx` is the boundary that should be excluded from the new
layer. If the caller selects an older base, or no base at all, then
packs from layers above that base have to be included in the detached
layer so that its bitmap has reachability closure.

Teach `should_include_pack()` to choose the MIDX used for pack exclusion
based on whether or not we are performing an incremental write. When
doing so, use `ctx->base_midx`, and use `ctx->m` otherwise.

The t5334 cases from the previous commit can now be marked as
successful.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx-write.c b/midx-write.c
index aa438775ebd23fbd5eec0b4277a0fdf6af8b7987..c50fdb5c6d17c3b8c3cde7860013bf0c89e0dd8b 100644 (file)
--- a/midx-write.c
+++ b/midx-write.c
@@ -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.
+        */
+       if (ctx->incremental)
+               m = ctx->base_midx;
+
+       /*
+        * Note that at most one of m and ctx->to_include are set,
         * so we are testing midx_contains_pack() and
         * string_list_has_string() independently (guarded by the
         * appropriate NULL checks).
@@ -148,10 +157,7 @@ static int should_include_pack(const struct write_midx_context *ctx,
         * should be performed independently (likely checking
         * to_include before the existing MIDX).
         */
-       if (ctx->m && midx_contains_pack(ctx->m, file_name))
-               return 0;
-       else if (ctx->base_midx && midx_contains_pack(ctx->base_midx,
-                                                     file_name))
+       if (m && midx_contains_pack(m, file_name))
                return 0;
        else if (ctx->to_include &&
                 !string_list_has_string(ctx->to_include, file_name))

diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh
index 69e96bf8d93713a9bd2c584954f3b819bfaae2d9..84ff612097888f16c50b37790887faa740e6b411 100755 (executable)
--- a/t/t5334-incremental-multi-pack-index.sh
+++ b/t/t5334-incremental-multi-pack-index.sh
@@ -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 &&
 
@@ -136,7 +136,7 @@ test_expect_failure 'write MIDX layer with --base=none and --no-write-chain-file
        cp "$midx_chain.bak" "$midx_chain"
 '
 
-test_expect_failure 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
+test_expect_success 'write MIDX layer with --base=<hash> and --no-write-chain-file' '
        test_commit base-hash &&
        git repack -d &&