Re: [PATCH v3 6/8] gc: add `gc.repackFilter` config option

On Wed, Jul 26, 2023 at 1:07 AM Taylor Blau [off-list ref] wrote:

On Mon, Jul 24, 2023 at 10:59:07AM +0200, Christian Couder wrote:

quoted

A previous commit has implemented `git repack --filter=<filter-spec>` to
allow users to filter out some objects from the main pack and move them
into a new different pack.

Users might want to perform such a cleanup regularly at the same time as
they perform other repacks and cleanups, so as part of `git gc`.

Let's allow them to configure a <filter-spec> for that purpose using a
new gc.repackFilter config option.

Makes sense.

quoted

Now when `git gc` will perform a repack with a <filter-spec> configured
through this option and not empty, the repack process will be passed a
corresponding `--filter=<filter-spec>` argument.

I may be missing something, but what happens if the user has configured
gc.repackFilter, but passes additional filters over the command-line
arguments? I'm not sure whether these should be AND'd with the existing
filters in config, or if they should reset them to zero, or something
else.

`git gc` doesn't recognize `--filter=<...>` arguments, only `git
repack` is being teached to recognize it in this patch series. So I
don't see how there could be multiple such arguments on the command
line when `git gc` is used.

Also in version 4 `git repack` can be passed many such arguments
anyway. So I think we are good.

We could support multiple gc.repackFilter config options, but on the
other hand using something like
`combine:<filter1>+<filter2>+...<filterN>` should work, as the content
of the option is passed as-is to the command line. So we can leave
that improvement for later if people don't like the `combine:...` and
are interested in it.

Regardless, I think it would be beneficial to users if we spelled this
out in git-gc(1) instead of just this patch message here.

I am not sure what should be spelled out. I think we refer people to
the `repack --filter=...` option which in turn refers to the `rev-list
--filter=...` which contains a good amount of documentation about how
`--filter=...` works, including the fact that `combine:...` can be
used and that multiple `--filter=...` options can be passed.

quoted

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 69509d0c11..5b89faf505 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh

@@ -202,6 +202,18 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
      grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
 '

+test_expect_success 'gc.repackFilter launches repack with a filter' '
+     test_when_finished "rm -rf bare.git" &&
+     git clone --no-local --bare . bare.git &&
+
+     git -C bare.git -c gc.cruftPacks=false gc &&
+     test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+
+     GIT_TRACE=$(pwd)/trace.out git -C bare.git -c gc.repackFilter=blob:none -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&

Nit: can we wrap this across multiple lines?

Done in version 4.

quoted

+     test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
+     grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out
+'

I think the `test_subcommand` helper might work here, and it would allow
you to avoid writing a long grep invocation.

Other tests related to gc.reflogExpire above use a grep invocation
similar to this one, while `test_subcommand` isn't used in the test
script, so I think the grep invocation makes the whole script a bit
easier to understand.

Thanks.