Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack
From: Jeff King <hidden>
Date: 2024-06-19 13:35:23
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Jun 19, 2024 at 09:22:08AM -0400, Jeff King wrote:
On Wed, Jun 19, 2024 at 09:02:56AM -0400, Jeff King wrote:quoted
I think it's a bug that fetch.unpackLimit causes index-pack to create a lockfile at all, but there's a more direct way to trigger the issue, which I've used in the patch below. I'll follow up with more details in a reply to the patch.Your original test used transfer.unpackLimit. By itself that should just cause us to avoid calling unpack-objects, keeping the pack we got, but _not_ creating a .keep file. Likewise, if you pass one "-k" to fetch-pack, it should just keep the pack but without a .keep file (that's what the double "-k -k" does). But both of these do trigger a .keep file, which seems wrong to me. The caller has no idea that the .keep file was created, so it won't clean it up, and the pack will be in limbo forever. I tried bisecting and came up with fa74052922 (Always obtain fetch-pack arguments from struct fetch_pack_args, 2007-09-19). Given the length of time it's been this way, that makes me a little afraid to touch it. ;)
Even before that commit, we'd trigger the lock of unpack_limit was set there. I find all of this code really puzzling (which makes me even more hesitant to touch it). But I really don't understand why it is not just this:
diff --git a/fetch-pack.c b/fetch-pack.c
index 42f48fbc31..ed57b0fdac 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c@@ -971,7 +971,7 @@ static int get_pack(struct fetch_pack_args *args, strvec_push(&cmd.args, "-v"); if (args->use_thin_pack) strvec_push(&cmd.args, "--fix-thin"); - if ((do_keep || index_pack_args) && (args->lock_pack || unpack_limit)) + if ((do_keep || index_pack_args) && args->lock_pack) add_index_pack_keep_option(&cmd.args); if (!index_pack_args && args->check_self_contained_and_connected) strvec_push(&cmd.args, "--check-self-contained-and-connected");
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 585ea0ee16..d6d6ea6281 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh@@ -1003,6 +1003,28 @@ test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault -C client fetch-pack -k -k ../server HEAD ' +test_expect_success 'fetch-pack -k does not create .keep file' ' + rm -rf server client && + test_create_repo server && + test_commit -C server one && + + test_create_repo client && + git -C client fetch-pack -k ../server HEAD && + find client/.git/objects/pack -name "*.keep" >keep && + test_must_be_empty keep +' + +test_expect_success 'fetch-pack with unpackLimit does not create .keep file' ' + rm -rf server client && + test_create_repo server && + test_commit -C server one && + + test_create_repo client && + git -c fetch.unpackLimit=1 -C client fetch-pack ../server HEAD && + find client/.git/objects/pack -name "*.keep" >keep && + test_must_be_empty keep +' + test_expect_success 'filtering by size' ' rm -rf server client && test_create_repo server &&