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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 12, 2021, 16:27 UTC
Message-ID: <X/3ODgaa9wr65M09@nand.local>
In-Reply-To: <X/1guCOGWybOzIS7@coredump.intra.peff.net>

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

Show 6 quoted lines

> > There are four ways to interact with the reverse index. Accordingly,
> > four functions will be exported from 'pack-revindex.h' by the time that
> > the existing API is removed. A caller may:
>
> This tells us what the new API functions do. That's useful, but should
> it be in the header file itself, documenting each function?

Mm, that's a good idea. I took your suggestion for a comment at the top of pack-revindex.h directly, and then added some of my own commentary above each function. I avoided documenting the functions we're about to remove for obvious reasons.

I think that the commit message is OK as-is, since it provides more of a rationale of what operations need to exist, rather than the specifics of each implementation.

We'll have to update these again when the on-disk format exists (i.e., because all of the runtimes become constant with the exception of going to the index), but that's a topic for another series.

Show 9 quoted lines

> > +int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
>
> The types here make sense. off_t is clearly needed for a pack offset,
> and uint32_t is correct for the position fields, because packs have a
> 4-byte object count.
>
> Separating the error return from the out-parameter makes the interface
> slightly more awkward, but is needed to use the properly-sized types.
> Makes sense.

Yep, exactly.

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.

It is awkward, but (as you note downthread) the callers of pack_pos_to_index() make it difficult to do so, so I didn't pursue it here.

> -Peff

Thanks, Taylor