Subject: Re: [PATCH 02/20] write_reuse_object(): convert to new revindex API

From: Taylor Blau <[email-masked]>
Date: Tue, 12 Jan 2021 11:31:40 -0500

On Tue, Jan 12, 2021 at 03:47:47AM -0500, Jeff King wrote:
> > @@ -436,10 +436,13 @@ static off_t write_reuse_object(struct hashfile *f, struct object_entry *entry,
> >  					      type, entry_size);
> >
> >  	offset = entry->in_pack_offset;
> > -	revidx = find_pack_revindex(p, offset);
> > -	datalen = revidx[1].offset - offset;
> > +	if (offset_to_pack_pos(p, offset, &pos) < 0)
> > +		die(_("write_reuse_object: could not locate %s"),
> > +		    oid_to_hex(&entry->idx.oid));
>
> If we believe the offset is bogus, should we print that in the error
> message, too? Something like:
>
>   die("could not locate %s, expected at offset %"PRIuMAX" in pack %s",
>       oid_to_hex(&entry->idx.oid), (uintmax_t)offset, p->pack_name);

Good idea, thanks.

> > +	datalen = pack_pos_to_offset(p, pos + 1) - offset;
>
> This "pos + 1" means we may be looking one past the end of the array.
> That's OK (at least for now), because our revindex always puts in an
> extra dummy value exactly for computing these kinds of byte-distances.
> That might be worth documenting in the API header.

Yeah, I made sure to document that when I was touching up the last
patch. FWIW, that's a behavior that we're going to carry over even when
the reverse index is stored on-disk (not by writing four extra bytes
into the .rev file, but by handling queries for pos == p->num_objects
separately.)

> -Peff

Thanks,
Taylor