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

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:

quoted

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