Re: [PATCH 14/20] unpack_entry(): convert to new revindex API

[Jeff King <peff@peff.net>]

On Fri, Jan 08, 2021 at 01:17:39PM -0500, Taylor Blau wrote:

> diff --git a/packfile.c b/packfile.c
> index 469c8d4f57..34467ea4a3 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1692,11 +1692,19 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
>  		}
>  
>  		if (do_check_packed_object_crc && p->index_version > 1) {
> -			struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
> -			off_t len = revidx[1].offset - obj_offset;
> -			if (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {
> +			uint32_t pos, nr;

We have "pos" and "nr". What's the difference? :)

I think pack_pos and index_pos might be harder to get confused.

> +			off_t len;
> +
> +			if (offset_to_pack_pos(p, obj_offset, &pos) < 0) {
> +				data = NULL;
> +				goto out;
> +			}

Nice to see the error check here. As with the previous commit, we
probably want to error(), just as we would for errors below.

Do we also need to call mark_bad_packed_object()? I guess we can't,
because we only have the offset, and not the oid (the code below uses
nth_packed_object_id(), but it is relying on the revindex, which we know
just failed to work).

I'm just wondering if an error here is going to put us into an infinite
loop of retrying the lookup in the same pack over and over. Let's
see...our caller is ultimately packed_object_info(), but it too does not
have the oid. It returns an error up to do_oid_object_info_extended().
Which yes, does mark_bad_packed_object() itself. Good. So I think we are
fine, and arguably these lower-level calls to mark_bad_packed_object()
are not necessary. But they do not hurt either.

-Peff