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

[Junio C Hamano <gitster@pobox.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.

> 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.

> +		}
> +
> +		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.

> 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);