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

[Jeff King <peff@peff.net>]

On Fri, Jan 08, 2021 at 01:16:48PM -0500, Taylor Blau wrote:

> First replace 'find_pack_revindex()' with its replacement
> 'offset_to_pack_pos()'. This prevents any bogus OFS_DELTA that may make
> its way through until 'write_reuse_object()' from causing a bad memory
> read (if 'revidx' is 'NULL')

Nice. Better abstraction, and we're catching more errors.

> @@ -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);

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

-Peff