Re: [PATCH 01/20] pack-revindex: introduce a new API

From: Jeff King <peff@peff.net>
Date: Jan 12, 2021, 09:41 UTC
Message-ID: <X/1u46v1dhu0Aj8G@coredump.intra.peff.net>
In-Reply-To: <X/1guCOGWybOzIS7@coredump.intra.peff.net>

On Tue, Jan 12, 2021 at 03:41:28AM -0500, Jeff King wrote:

Show 21 quoted lines

> > +int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> > +{
> > +	int ret;
> > +
> > +	if (load_pack_revindex(p) < 0)
> > +		return -1;
> 
> This one lazy-loads the revindex for us, which seems handy...
> 
> > +uint32_t pack_pos_to_index(struct packed_git *p, uint32_t pos)
> > +{
> > +	if (!p->revindex)
> > +		BUG("pack_pos_to_index: reverse index not yet loaded");
> > +	if (pos >= p->num_objects)
> > +		BUG("pack_pos_to_index: out-of-bounds object at %"PRIu32, pos);
> > +	return p->revindex[pos].nr;
> > +}
> 
> But these ones don't. I'm glad we at least catch it with a BUG(), but it
> makes the API a little funny. Returning an error here would require a
> similarly awkward out-parameter, I guess.

Having now looked at the callers through the series, I think adding an error return to pack_pos_to_index() would be really awkward (since it cannot currently fail).

We _could_ insist that callers of offset_to_pack_pos() also make sure the revindex is loaded themselves. But it would be annoying and error-prone to check the existing callers. So I'm OK with leaving this asymmetry in the API.

-Peff