Thread (183 messages) 183 messages, 9 authors, 2022-06-11

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.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.
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help