From: Taylor Blau <me@ttaylorr.com>
Date: Thu, 01 Aug 2024 19:07:58 GMT
Subject: Re: [PATCH v2 06/19] midx: teach `nth_bitmapped_pack()` about incremental MIDXs
Message-ID: <ZqvdDkMZA8Yxcm/q@nand.local>
In-Reply-To: <20240801093947.GF1159276@coredump.intra.peff.net>

On Thu, Aug 01, 2024 at 05:39:47AM -0400, Jeff King wrote:
> >  int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
> >  		       struct bitmapped_pack *bp, uint32_t pack_int_id)
> >  {
> > +	uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);
> > +
>
> Heh, after the last one reused the "n" variable, now we are back to a
> separate local variable. Not wrong, but curious to go back and forth.

This one we care about having both, for a couple of reasons:
prepare_midx_pack() still expects us to have the global pack_int_id, and
just as well for bp->pack_int_id.

We could write this as:

    pack_int_id = midx_for_pack(&m, pack_int_id);
    if (prepare_midx_pack(r, m, pack_int_id + m->num_packs_in_base))
        return -1;

But I found it easier to have a separate local_-prefixed variable for
when referring to the MIDX-local pack identifier.

I'll add a short note in the commit message explaining why we took this
approach in this commit.

Thanks,
Taylor