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

From: Jeff King <peff@peff.net>
Date: Jan 13, 2021, 13:17 UTC
Message-ID: <X/7zA3KjlNnS2HhJ@coredump.intra.peff.net>
In-Reply-To: <xmqqa6tdz2fo.fsf@gitster.c.googlers.com>

On Wed, Jan 13, 2021 at 12:06:03AM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> > +int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)
> > +{
> > +	int ret;
> [...]
> > +	*pos = ret;
> 
> The untold assumption is that uint32_t can fit the maximum returned
> value from find_revindex_position() and "signed int" can also big
> enough.  I guess it is OK to be limited to up-to 2 billion objects
> on 32-bit systems.

Thanks for pointing this out. I recalled there being an "int" problem somewhere in the revindex code, but I didn't notice it on my read-through. This bug already exists (the problem is actually in the find_revindex_position() interface), and is fixed when we inline that into offset_to_pack_pos() in patch 18.

It might be worth mentioning the fix there.

-Peff