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