Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack

Junio C Hamano [off-list ref] writes:

quoted

+		if (type != OBJ_BLOB || size < big_file_threshold) {
+			data = unpack_entry(p, entries[i].offset, &type, &size);
+			data_valid = 1;

This codepath slurps the data in-core to hash and data is later
freed, i.e. non-blob objects and small ones are handled as before.

quoted

+		}
+
+		if (data_valid && !data)
 			err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
 				    sha1_to_hex(entries[i].sha1), p->pack_name,
 				    (uintmax_t)entries[i].offset);

Otherwise, we'd go to check_sha1_signature() with map==NULL.  And
that is exactly what we want---map==NULL is the way we tell the
function to use the streaming interface to check.

Good.

But not quite correct yet ;-)

Here is what I'd propose to squash in.  Even though data_valid
protects the above if() condition from accessing an otherwise
uninitialized "data", the call to check_sha1_signature() we have
later will get noticed by the compiler that "data" is passed
uninitialized.

More importantly, uninitialized data passed may be non-NULL, in
which case it would not trigger the streaming interface.

 pack-check.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pack-check.c b/pack-check.c
index 0777766..ac263fd 100644
--- a/pack-check.c
+++ b/pack-check.c

@@ -123,7 +123,14 @@ static int verify_packfile(struct packed_git *p,
 		type = unpack_object_header(p, w_curs, &curpos, &size);
 		unuse_pack(w_curs);
 
-		if (type != OBJ_BLOB || size < big_file_threshold) {
+		if (type == OBJ_BLOB && big_file_threshold <= size) {
+			/*
+			 * Let check_sha1_signature() to check it with
+			 * the streaming interface; no point slurping
+			 * the data in-core only to discard.
+			 */
+			data = NULL;
+		} else {
 			data = unpack_entry(p, entries[i].offset, &type, &size);
 			data_valid = 1;
 		}