Re: [PATCH v2 13/24] pack-bitmap: read multi-pack bitmaps

From: Jeff King <peff@peff.net>
Date: Jul 23, 2021, 09:40 UTC
Message-ID: <YPqOjAkG7EZ6KXd6@coredump.intra.peff.net>
In-Reply-To: <YPinPFW50Mj8cVkP@nand.local>

On Wed, Jul 21, 2021 at 07:01:16PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> On Wed, Jul 21, 2021 at 07:32:49AM -0400, Jeff King wrote:
> > On Mon, Jun 21, 2021 at 06:25:31PM -0400, Taylor Blau wrote:
> > > +	if (!is_pack_valid(packfile)) {
> > > +		close(fd);
> > > +		return -1;
> > > +	}
> > > +
> >
> > What's this extra is_pack_valid() doing? I wouldn't expect many changes
> > at all to this non-midx code path (aside from the "did we already load a
> > midx bitmap" in the earlier part of the hunk, which makes sense).
> 
> That looks like a mistake to me. I did a little digging and tried to
> remember if it could have ever been useful, but I think that it's just a
> stray change that has no value. Removed.

This turned out to be quite interesting. It _is_ a mistake to include it in this series. But it turns out to be quite valuable on its own. :)

I just cleaned it up and sent it as its own separate patch:

  https://lore.kernel.org/git/YPqL%2FpZt6hNYN4hB@coredump.intra.peff.net/

So it's a happy accident that your series called attention to it. :)

-Peff