Re: [PATCH 6/9] repack: add `--filter=<filter-spec>` option

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 14, 2023 at 09:25:38PM +0200, Christian Couder wrote:
> ---
>  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.

> 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