Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack
From: Junio C Hamano <hidden>
Date: 2016-07-12 18:39:53
Nguyễn Thái Ngọc Duy [off-list ref] writes:
For this particular operation, not unpacking the blob and letting check_sha1_signature, which supports streaming interface, do the job is sufficient.
That reasoning is sound, I would think, but...
We will call the callback function "fn" with NULL as "data". The only callback of this function is fsck_obj_buffer(), which does not touch "data" at all if it's a blob.
... this may be a bit too magical that the assumption needs to be left in an in-code comment to warn people against temptation to using "data" over there, not just where the type of verify_fn is declared. Essentially, you are changing the expectation from existing functions of that type.
quoted hunk
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- pack-check.c | 15 +++++++++++++-- pack.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-)diff --git a/pack-check.c b/pack-check.c index 1da89a4..0777766 100644 --- a/pack-check.c +++ b/pack-check.c@@ -105,6 +105,8 @@ static int verify_packfile(struct packed_git *p, void *data; enum object_type type; unsigned long size; + off_t curpos; + int data_valid = 0; if (p->index_version > 1) { off_t offset = entries[i].offset;@@ -116,8 +118,17 @@ static int verify_packfile(struct packed_git *p, sha1_to_hex(entries[i].sha1), p->pack_name, (uintmax_t)offset); } - data = unpack_entry(p, entries[i].offset, &type, &size); - if (!data) + + curpos = entries[i].offset; + type = unpack_object_header(p, w_curs, &curpos, &size); + unuse_pack(w_curs);
This made me wonder where this "unuse" comes from; w_curs's in-use count is incremented by calling unpack_object_header() and we are done with that access at this point, hence we have unuse here.
+ 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.
+ }
+
+ 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.
quoted hunk
diff --git a/pack.h b/pack.h index 3223f5a..0e77429 100644 --- a/pack.h +++ b/pack.h@@ -74,6 +74,7 @@ struct pack_idx_entry { struct progress; +/* Note, the data argument could be NULL if object type is blob */ typedef int (*verify_fn)(const unsigned char*, enum object_type, unsigned long, void*, int*); extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);