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

On Tue, Jan 12, 2021 at 04:22:26AM -0500, Jeff King wrote:

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

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

Much clearer, thanks.

quoted

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

Yup, good call.

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.

Thanks, this rationale is helpful to have. I included an abridged
version of it in the patch message.

-Peff

Thanks,
Taylor