On Tue, Dec 10, 2024 at 12:01 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
> > I noticed that fcb2205b77 (midx: implement support for writing
> > incremental MIDX chains, 2024-08-06)
> > which introduced GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL adds lines like:
> >
> > GIT_TEST_MULTI_PACK_INDEX=0
> > GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
> >
> > at the top of a number of repack related test scripts like
> > t7700-repack.sh, so I guess that it should be OK to add the same lines
> > at the top of the t5710 test script added by this series. This should
> > fix the CI failures.
> >
> > I have made this change in my current version.
>
> Thanks.
>
> Is it because the feature is fundamentally incompatible with the
> multi-pack index (or its incremental writing),
It's not an incompatibility with the feature developed in this series.
Adding the following test script on top of master or even fcb2205b77
(midx: implement support for writing incremental MIDX chains,
2024-08-06), shows that it fails in the same way without any code
change to `git` itself from this series:
diff --git a/t/t5709-midx-increment-write.sh b/t/t5709-midx-increment-write.sh
new file mode 100755
index 0000000000..8801222374
--- /dev/null
+++ b/t/t5709-midx-increment-write.sh
@@ -0,0 +1,132 @@
+#!/bin/sh
+
+test_description='test midx incremental write'
+
+. ./test-lib.sh
+
+export GIT_TEST_MULTI_PACK_INDEX=1
+export GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1
+
+# Setup the repository with three commits, this way HEAD is always
+# available and we can hide commit 1 or 2.
+test_expect_success 'setup: create "template" repository' '
+ git init template &&
+ test_commit -C template 1 &&
+ test_commit -C template 2 &&
+ test_commit -C template 3 &&
+ test-tool genrandom foo 10240 >template/foo &&
+ git -C template add foo &&
+ git -C template commit -m foo
+'
+
+# A bare repo will act as a server repo with unpacked objects.
+test_expect_success 'setup: create bare "server" repository' '
+ git clone --bare --no-local template server &&
+ mv server/objects/pack/pack-* . &&
+ packfile=$(ls pack-*.pack) &&
+ git -C server unpack-objects --strict <"$packfile"
+'
+
+check_missing_objects () {
+ git -C "$1" rev-list --objects --all --missing=print > all.txt &&
+ perl -ne 'print if s/^[?]//' all.txt >missing.txt &&
+ test_line_count = "$2" missing.txt &&
+ if test "$2" -lt 2
+ then
+ test "$3" = "$(cat missing.txt)"
+ else
+ test -f "$3" &&
+ sort <"$3" >expected_sorted &&
+ sort <missing.txt >actual_sorted &&
+ test_cmp expected_sorted actual_sorted
+ fi
+}
+
+initialize_server () {
+ count="$1"
+ missing_oids="$2"
+
+ # Repack everything first
+ git -C server -c repack.writebitmaps=false repack -a -d &&
+
+ # Remove promisor file in case they exist, useful when reinitializing
+ rm -rf server/objects/pack/*.promisor &&
+
+ # Repack without the largest object and create a promisor pack on server
+ git -C server -c repack.writebitmaps=false repack -a -d \
+ --filter=blob:limit=5k --filter-to="$(pwd)/pack" &&
+ promisor_file=$(ls server/objects/pack/*.pack | sed
"s/\.pack/.promisor/") &&
+ >"$promisor_file" &&
+
+ # Check objects missing on the server
+ check_missing_objects server "$count" "$missing_oids"
+}
+
+copy_to_server2 () {
+ oid_path="$(test_oid_to_path $1)" &&
+ path="server/objects/$oid_path" &&
+ path2="server2/objects/$oid_path" &&
+ mkdir -p $(dirname "$path2") &&
+ cp "$path" "$path2"
+}
+
+test_expect_success "setup for testing promisor remote advertisement" '
+ # Create another bare repo called "server2"
+ git init --bare server2 &&
+
+ # Copy the largest object from server to server2
+ obj="HEAD:foo" &&
+ oid="$(git -C server rev-parse $obj)" &&
+ copy_to_server2 "$oid" &&
+
+ initialize_server 1 "$oid" &&
+
+ # Configure server2 as promisor remote for server
+ git -C server remote add server2 "file://$(pwd)/server2" &&
+ git -C server config remote.server2.promisor true &&
+
+ git -C server2 config uploadpack.allowFilter true &&
+ git -C server2 config uploadpack.allowAnySHA1InWant true &&
+ git -C server config uploadpack.allowFilter true &&
+ git -C server config uploadpack.allowAnySHA1InWant true
+'
+
+test_expect_success "setup for subsequent fetches" '
+ # Generate new commit with large blob
+ test-tool genrandom bar 10240 >template/bar &&
+ git -C template add bar &&
+ git -C template commit -m bar &&
+
+ # Fetch new commit with large blob
+ git -C server fetch origin &&
+ git -C server update-ref HEAD FETCH_HEAD &&
+ git -C server rev-parse HEAD >expected_head &&
+
+ # Repack everything twice and remove .promisor files before
+ # each repack. This makes sure everything gets repacked
+ # into a single packfile. The second repack is necessary
+ # because the first one fetches from server2 and creates a new
+ # packfile and its associated .promisor file.
+
+ rm -f server/objects/pack/*.promisor &&
+ git -C server -c repack.writebitmaps=false repack -a -d &&
+ rm -f server/objects/pack/*.promisor &&
+ git -C server -c repack.writebitmaps=false repack -a -d &&
+
+ # Unpack everything
+ rm pack-* &&
+ mv server/objects/pack/pack-* . &&
+ packfile=$(ls pack-*.pack) &&
+ git -C server unpack-objects --strict <"$packfile" &&
+
+ # Copy new large object to server2
+ obj_bar="HEAD:bar" &&
+ oid_bar="$(git -C server rev-parse $obj_bar)" &&
+ copy_to_server2 "$oid_bar" &&
+
+ # Reinitialize server so that the 2 largest objects are missing
+ printf "%s\n" "$oid" "$oid_bar" >expected_missing.txt &&
+ initialize_server 2 expected_missing.txt
+'
+
+test_done
Changing `export GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1` into
`export GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0` at the top of
the file makes it work.
This could probably be simplified, but I think it shows that it's just
the incremental writing of the multi-pack index that is incompatible
or has a bug when doing some repacking.
> or is it merely
> because the way the feature is verified assumes that the multi-pack
> index is not used, even though the protocol exchange, capability
> selection, and the actual behaviour adjustment for the capability
> are all working just fine? I am assuming it is the latter, but just
> to make sure we know where we stand...
Let me know if you need more than the above, but I think it's fair for
now to just use:
GIT_TEST_MULTI_PACK_INDEX=0
GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
at the top of the tests, like it's done in the version 4 of this
series I will send soon.
