Re: [PATCH 17/49] builtin/repack.c: pass "packdir" when removing packs

[Taylor Blau <me@ttaylorr.com>]

On Fri, Oct 10, 2025 at 02:04:01AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:08:16PM -0400, Taylor Blau wrote:
>
> > builtin/repack.c defines a static "packdir" to instruct pack-objects on
> > where to write any new packfiles. This is also the directory scanned
> > when removing any packfiles which were made redundant by the latest
> > repack.
> >
> > Prepare to move the "existing_packs_remove_redundant" function to its
> > own compilation unit by passing in this information as a parameter to
> > that function.
>
> OK. I don't think there's anything wrong here, but it feels like we
> ought to be able to pull this from the existing "struct repository"
> context that's in the existing_packs struct.
>
> I had to dig a little to find it, but I think that always comes from:
>
>   packdir = mkpathdup("%s/pack", repo_get_object_directory(repo));
>
> in cmd_repack(). So we'd have to recreate that string ourselves. It
> feels like object_database (or I guess odb_source) should have an easy
> way to get the pack directory, but it doesn't seem to.
>
> So I dunno. I think it is mostly academic, as you are just moving around
> code here. But as an API, the separate "packdir" argument makes me
> wonder if I can pass anything I like to these functions. But really, it
> should match the packdir used to create the lists in the existing_packs
> struct. So it's kind of a brittle API. I'm not sure how much we should
> care for this series, as it is mostly about moving code around, and not
> providing some kind of flexible repack API.

Oh, it's extremely brittle, I agree ;-).

I agree with your assessment here that packdir can easily be derived
from the repository object, and that doing so would result in a cleaner,
less brittle API. I'd like to make those changes, but again would prefer
to do so on top of this series, since I want to change as few things as
possible here.

Thanks,
Taylor