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

On Wed, Jun 21, 2023 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote:

Yeah, it works, thanks. That's what is used in the version 2 I just sent.

I have improved this doc in version 2. It now says:

--filter=<filter-spec>::
       Remove objects matching the filter specification from the
       resulting packfile and put them into a separate packfile. Note
       that objects used in the working directory are not filtered
       out. So for the split to fully work, it's best to perform it
       in a bare repo and to use the `-a` and `-d` options along with
       this option.  See linkgit:git-rev-list[1] for valid
       `<filter-spec>` forms.

For comparison the doc about --cruft is:

--cruft::
       Same as `-a`, unless `-d` is used. Then any unreachable objects
       are packed into a separate cruft pack. Unreachable objects can
       be pruned using the normal expiry rules with the next `git gc`
       invocation (see linkgit:git-gc[1]). Incompatible with `-k`.

With or without "-d", no object is deleted, objects are just put into 2 different packfiles depending on whether or not they match the filter.

"-d" is indeed about removing redundant packs. If you want to split the objects into different packs according to a filter, then you probably don't want to keep packs that contain both kinds of objects that you want to separate into different packs, so the doc now recommends using "-d" (along with "-a") when using "--filter". That's also what the tests are using.

I am not sure it's worth erroring out when "-d", or "-a", is not used. Perhaps there are some use cases where it's interesting to keep old packs, or, in case of not using "-a", to split only loose objects into separate packs, but not objects from existing packs.

Yes, in version 2 I have added a test (in the commit introducing --filter-to) that checks that objects that are filtered out and that are larger than --max-pack-size get split into their own packs.

I am not sure as I don't know well how cruft works. I would need to check the code and/or test it.

Not sure either, as I also don't know well how --geometric works.

In the tests, repack --filter is used with "-c repack.writebitmaps=false" as otherwise the bitmap writing code tends to complain that a pack is not complete or something like that. I think it would make sense to have options that would make the bitmap writing code write a regular bitmap index if all the objects are in a single pack, but a MIDX if they are in multiple packs. I don't know what --cruft or --max-pack-size are doing about this, but I think such a feature could be useful in case those options are used.

In the meantime I would be Ok with disallowing using both --write-bitmap-index and --filter. The issue is that writing a bitmap index is the default behavior, even without --write-bitmap-index, so it might be a bit more complex, but I plan to do something about it in version 3.

In version 2, the doc has been improved and now it seems comparable to the --cruft doc. About the tests, I added one in version 2 related to --max-pack-size, and I am open to adding a few others, but I don't think it's a good idea to add a lot of them. It just doesn't scale when commands have more than a few options.

The new code is reusing existing features (like --stdin-packs) that are already used by other code (like --cruft code). It is not adding new intricate mechanisms, nor a lot of new code. It is not changing code used by other features except for a few clean refactorings. This should give a good indication that it will work well with other features.

--cruft is also splitting existing packs based on some criteria. So if we go this way, perhaps --cruft code should be moved first to this new special purpose builtin?