Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack

From

Jeff King <peff@peff.net>

Date

Jun 19, 2024, 13:35 UTC

Message-ID

<20240619133522.GA942902@coredump.intra.peff.net>

In-Reply-To

<20240619132208.GA895873@coredump.intra.peff.net>

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:
> 
> > 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. ;)

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 &&