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