Nguyễn Thái Ngọc Duy <pclo...@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 <pclo...@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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
