Re: [PATCH v7 1/5] unpack-objects.c: add dry_run mode for get_data()
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-12-21 14:15:50
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Dec 21 2021, Han Xin wrote:
quoted hunk ↗ jump to hunk
From: Han Xin <redacted> In dry_run mode, "get_data()" is used to verify the inflation of data, and the returned buffer will not be used at all and will be freed immediately. Even in dry_run mode, it is dangerous to allocate a full-size buffer for a large blob object. Therefore, only allocate a low memory footprint when calling "get_data()" in dry_run mode. Suggested-by: Jiang Xin <redacted> Signed-off-by: Han Xin <redacted> --- builtin/unpack-objects.c | 23 +++++++++--- t/t5590-unpack-non-delta-objects.sh | 57 +++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) create mode 100755 t/t5590-unpack-non-delta-objects.shdiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 4a9466295b..9104eb48da 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c@@ -96,15 +96,21 @@ static void use(int bytes) display_throughput(progress, consumed_bytes); } -static void *get_data(unsigned long size) +static void *get_data(size_t size, int dry_run) { git_zstream stream; - void *buf = xmallocz(size); + size_t bufsize; + void *buf; memset(&stream, 0, sizeof(stream)); + if (dry_run && size > 8192) + bufsize = 8192; + else + bufsize = size; + buf = xmallocz(bufsize);
Maybe I'm misunderstanding this, but the commit message says it would be dangerous to allocate a very larger buffer, but here we only limit the size under "dry_run". Removing that "&& size > 8192" makes all the tests pass still, so there seems to be some missing coverage here in any case.
quoted hunk ↗ jump to hunk
diff --git a/t/t5590-unpack-non-delta-objects.sh b/t/t5590-unpack-non-delta-objects.sh new file mode 100755 index 0000000000..48c4fb1ba3 --- /dev/null +++ b/t/t5590-unpack-non-delta-objects.sh@@ -0,0 +1,57 @@ +#!/bin/sh +# +# Copyright (c) 2021 Han Xin +# + +test_description='Test unpack-objects with non-delta objects' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +prepare_dest () { + test_when_finished "rm -rf dest.git" && + git init --bare dest.git +} + +test_expect_success "setup repo with big blobs (1.5 MB)" ' + test-tool genrandom foo 1500000 >big-blob && + test_commit --append foo big-blob && + test-tool genrandom bar 1500000 >big-blob && + test_commit --append bar big-blob && + ( + cd .git && + find objects/?? -type f | sort + ) >expect && + PACK=$(echo main | git pack-objects --revs test) +' + +test_expect_success 'setup env: GIT_ALLOC_LIMIT to 1MB' ' + GIT_ALLOC_LIMIT=1m && + export GIT_ALLOC_LIMIT +' + +test_expect_success 'fail to unpack-objects: cannot allocate' ' + prepare_dest && + test_must_fail git -C dest.git unpack-objects <test-$PACK.pack 2>err && + grep "fatal: attempting to allocate" err && + ( + cd dest.git && + find objects/?? -type f | sort + ) >actual && + test_file_not_empty actual && + ! test_cmp expect actual +' + +test_expect_success 'unpack-objects dry-run' ' + prepare_dest && + git -C dest.git unpack-objects -n <test-$PACK.pack && + ( + cd dest.git && + find objects/ -type f + ) >actual && + test_must_be_empty actual +' + +test_done
I commented on this "find" usage in an earlier round, I think there's a much easier way to do this. You're really just going back and forth between checking whether or not all the objects are loose. I think that the below fix-up on top of this series is a better way to do that, and more accurate. I.e. in your test here you check "! test_cmp", which means that we could have some packed and some loose, but really what you're meaning to check is a flip-flop between "all loose?" and "no loose?. In addition to that there was no reason to hardcode "main", we can just use HEAD. All in all I think the below fix-up makes sense:
diff --git a/t/t5590-unpack-non-delta-objects.sh b/t/t5590-unpack-non-delta-objects.sh
index 8436cbf8db6..d78bb89225d 100755
--- a/t/t5590-unpack-non-delta-objects.sh
+++ b/t/t5590-unpack-non-delta-objects.sh@@ -5,9 +5,6 @@ test_description='Test unpack-objects with non-delta objects' -GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main -export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME - . ./test-lib.sh prepare_dest () {
@@ -20,16 +17,22 @@ prepare_dest () { fi } +assert_no_loose () { + glob=dest.git/objects/?? && + echo "$glob" >expect && + echo $glob >actual && + test_cmp expect actual +} + test_expect_success "setup repo with big blobs (1.5 MB)" ' test-tool genrandom foo 1500000 >big-blob && test_commit --append foo big-blob && test-tool genrandom bar 1500000 >big-blob && test_commit --append bar big-blob && - ( - cd .git && - find objects/?? -type f | sort - ) >expect && - PACK=$(echo main | git pack-objects --revs test) + + # Everything is loose + rmdir .git/objects/pack && + PACK=$(echo HEAD | git pack-objects --revs test) ' test_expect_success 'setup env: GIT_ALLOC_LIMIT to 1MB' '
@@ -41,51 +44,27 @@ test_expect_success 'fail to unpack-objects: cannot allocate' ' prepare_dest 2m && test_must_fail git -C dest.git unpack-objects <test-$PACK.pack 2>err && grep "fatal: attempting to allocate" err && - ( - cd dest.git && - find objects/?? -type f | sort - ) >actual && - test_file_not_empty actual && - ! test_cmp expect actual + rmdir dest.git/objects/pack ' test_expect_success 'unpack big object in stream' ' prepare_dest 1m && mkdir -p dest.git/objects/05 && git -C dest.git unpack-objects <test-$PACK.pack && - git -C dest.git fsck && - ( - cd dest.git && - find objects/?? -type f | sort - ) >actual && - test_cmp expect actual + rmdir dest.git/objects/pack ' test_expect_success 'unpack big object in stream with existing oids' ' prepare_dest 1m && git -C dest.git index-pack --stdin <test-$PACK.pack && - ( - cd dest.git && - find objects/?? -type f | sort - ) >actual && - test_must_be_empty actual && git -C dest.git unpack-objects <test-$PACK.pack && - git -C dest.git fsck && - ( - cd dest.git && - find objects/?? -type f | sort - ) >actual && - test_must_be_empty actual + assert_no_loose ' test_expect_success 'unpack-objects dry-run' ' prepare_dest && git -C dest.git unpack-objects -n <test-$PACK.pack && - ( - cd dest.git && - find objects/ -type f - ) >actual && - test_must_be_empty actual + assert_no_loose ' test_done