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

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 8, 2023, 08:38 UTC
Message-ID: <CAP8UFD1Z3O5Wf8D9dYZdgPuOWgo7NbQsHAWifkPUGE=Eo1U7EQ@mail.gmail.com>
In-Reply-To: <ZMBVzWoFQCUCSTwE@nand.local>

On Wed, Jul 26, 2023 at 1:07 AM Taylor Blau <me@ttaylorr.com> wrote:

Show 23 quoted lines

>
> On Mon, Jul 24, 2023 at 10:59:07AM +0200, Christian Couder wrote:
> > 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.
>
> > 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.

Show 18 quoted lines

> > 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.

Show 6 quoted lines

> > +     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.