Re: [PATCH 7/5] fsck: use streaming interface for large blobs in pack
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 12, 2016, 18:39 UTC
- Message-ID
- <xmqq7fcq66q6.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160710104555.27478-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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.
Show 29 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> 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.
Show 6 quoted lines
> + }
> +
> + 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.
Show 12 quoted lines
> 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);