Re: [PATCH 6/9] repack: add `--filter=<filter-spec>` option
From: Taylor Blau <hidden>
Date: 2023-06-21 11:17:39
On Wed, Jun 14, 2023 at 09:25:38PM +0200, Christian Couder wrote:
quoted hunk ↗ jump to hunk
--- Documentation/git-repack.txt | 5 +++ builtin/repack.c | 75 ++++++++++++++++++++++++++++++++++-- t/t7700-repack.sh | 16 ++++++++ 3 files changed, 93 insertions(+), 3 deletions(-)
Having read through the implementation in the repack builtin, I am almost certain that my suggestion earlier in the thread to implement this in terms of 'git pack-objects --filter' and 'git pack-objects --stdin-packs' would work.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt index 4017157949..aa29c7e648 100644 --- a/Documentation/git-repack.txt +++ b/Documentation/git-repack.txt@@ -143,6 +143,11 @@ depth is 4095. a larger and slower repository; see the discussion in `pack.packSizeLimit`. +--filter=<filter-spec>:: + Remove objects matching the filter specification from the + resulting packfile and put them into a separate packfile. See + linkgit:git-rev-list[1] for valid `<filter-spec>` forms. +
This documentation leaves me with a handful of questions about how it
interacts with other options. Here are some:
- What happens when you pass it with "-d"? Does it delete objects that
didn't match the filter? Leave them alone? If the latter, should
this combination be declared invalid instead of silently ignoring
the user's request to delete redundant packs?
- What happens with --max-pack-size? Does the filtered pack get split
into multiple packs (as I think we would expect from such a
combination)?
- What about with `--cruft`? Does it split the cruft pack into two
based on whether or not the unreachable object(s) matched or didn't
match the filter?
- What happens when passed with "--geometric"? I don't think there is
a sensible interpretation (at least, I can't think of what it would
mean to do "--filter=<spec> --geometric=<factor>" off the top of my
head).
- What about with "--write-bitmap-index"? Do we write one bitmap
index? Two? If the latter, do we combine the packs into a MIDX
before writing the bitmap? Should we?
I think it may be worth spelling out answers to some of these questions
in the documentation, and codifying those answers in the form of tests.
This makes me wonder whether or not this option should belong in repack
at all, or whether there should be some new special-purpose builtin that
is designed to split existing pack(s) based on whether or not they meet
some filter criteria.
Thanks,
Taylor