On Sun, Jul 02, 2023 at 09:11:17AM -0400, Jeff King wrote:
> On Thu, Jun 29, 2023 at 05:24:40AM -0400, Taylor Blau wrote:
>
> > > I also kind of wonder if this repack code should simply be loading and
> > > iterating the packed_git list, but that is a much bigger change.
> >
> > I have wanted to do this for a while ;-). The minimal patch is less
> > invasive than I had thought:
>
> Yeah, I agree it's not too bad. If we want to go that route, though, I
> think we should do it on top of Stolee's patch, though (which makes it a
> pure cleanup once the behaviors are aligned).
>
> I'm also not sure if you'd need to do anything tricky with alternate
> object dirs (it looks like the existing code ignores them entirely, so I
> guess we'd want to skip entries without pack_local set).
Yep, we definitely want a `if (!p->is_local) continue` in there to be
consistent with the existing behavior.
> > [...]
> > I think you could probably go further than this, since having to store
> > the suffix-less pack names in the fname_kept and fname_nonkept lists is
> > kind of weird.
> >
> > It would be nice if we could store pointers to the actual packed_git
> > structs themselves in place of those lists instead, but I'm not
> > immediately sure how feasible it would be to do since we re-prepare the
> > object store between enumerating and then removing these packs.
>
> I think that would work, because we do not ever drop packed_git entries
> from the list (even if the files were deleted between prepare/reprepare).
> But it also creates a subtle memory ownership dependency/assumption
> between the two bits of code. It seems clearer to me to just copy the
> names out to our own lists here (i.e., the patch you showed).
Yeah, I agree. I thought that it might clean things up further, and to
an extent it does:
https://github.com/ttaylorr/git/compare/tb/collect-packs-readdir
, but I think that the memory ownership issue is sufficiently subtle
that the clean-up isn't really worth it.
I put the above patch together with Stolee's, and sent the result back
to the list here:
https://lore.kernel.org/git/cover.1689017830.git.me@xxxxxxxxxxxx/T/#t
Thanks,
Taylor
