From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 15 Sep 2025 07:25:26 GMT
Subject: Re: [PATCH v4 00/15] packfile: carve out a new packfile store
Message-ID: <aMe_Zu1osrfDnSYL@pks.im>
In-Reply-To: <aMNeWPD6spSMB1yh@nand.local>

On Thu, Sep 11, 2025 at 07:42:16PM -0400, Taylor Blau wrote:
> On Thu, Sep 11, 2025 at 07:40:01PM -0400, Taylor Blau wrote:
> > On Tue, Sep 09, 2025 at 01:02:59PM +0200, Patrick Steinhardt wrote:
> > > Changes in v4:
> > >   - Small code style improvement as suggested by Junio.
> > >   - Some commit message improvements as suggested by Karthik.
> > >   - Link to v3: https://lore.kernel.org/r/20250902-b4-pks-packfiles-store-v3-0-6925278efeda@pks.im
> >
> > Thanks for these changes. I think that this series is getting closer,
> > but I do not think that it is quite ready yet.
> >
> > The largest outstanding topic is that this round claims that
> > get_all_packs() behaves identically to get_packed_git(), but this is not
> > the case. I mentioned in a response to the patch that removes the latter
> > function, but I think we should:
> >
> >  - Permit both get_all_packs() and get_packed_git() to coexist for now.
> >
> >  - In a follow-up series, transition get_packed_git() callers one-by-one
> >    to use get_all_packs() instead. Each of these commits should include
> >    IMHO a justification that the change is safe as-is, or include fixes
> >    to make it safe. I think any "fixes" here are limited to "if
> >    (p->multi_pack_index) continue;".
> >
> >  - Once there are no longer any callers of get_packed_git(), we can
> >    remove it.
> >
> > If you want to pursue that in this series, I am happy to review and
> > discuss it, but IMHO there is already enough going on here that I think
> > it makes more sense to do that in a separate follow-up.
> 
> Oops, hit "send" too early.
> 
> The other outstanding topic that I want to raise is that I think
> transitioning away from get_all_packs() to the new API in all callers is
> premature. I would *much* rather see us do this call-by-call when it is
> necessary to do so rather than forcing all callers onto the new API.
> 
> If there is a compelling reason that we must force callers to all use
> the new API now, I'm happy to discuss that, but as-is I worry that we
> are changing things too quickly here.

Okay, that's fair. I'll rework this patch then to not drop that function
and instead make it work on top of the packfile store, as well. I'll
then have another look at this in the next patch series.

Thanks!

Patrick