Re: [PATCH v7 1/5] unpack-objects.c: add dry_run mode for get_data()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 21, 2021, 14:09 UTC
- Message-ID
- <211221.86bl1arqls.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20211221115201.12120-2-chiyutianyi@gmail.com>
On Tue, Dec 21 2021, Han Xin wrote:
Show 38 quoted lines
> From: Han Xin <hanxin.hx@alibaba-inc.com>
>
> 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 <zhiyou.jx@alibaba-inc.com>
> Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com>
> ---
> 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.sh
>
> diff --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.
Show 63 quoted lines
> 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_doneI 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