From: Jeff King Date: Fri, 10 Oct 2025 06:04:01 GMT Subject: Re: [PATCH 17/49] builtin/repack.c: pass "packdir" when removing packs Message-ID: <20251010060401.GF1965904@coredump.intra.peff.net> In-Reply-To: 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. -Peff