From: Christian Couder <christian.couder@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
John Cai <johncai86@gmail.com>,
Jonathan Tan <jonathantanmy@google.com>,
Jonathan Nieder <jrnieder@gmail.com>,
Derrick Stolee <stolee@gmail.com>, Patrick Steinhardt <ps@pks.im>,
Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v3 5/8] repack: add `--filter=<filter-spec>` option
Date: Tue, 8 Aug 2023 10:34:25 +0200 [thread overview]
Message-ID: <CAP8UFD3BiZQ4-M91sc6p2wBFrzPQQfja_xKpT26BcBS7OVA4MA@mail.gmail.com> (raw)
In-Reply-To: <ZMBU+SALVQthOgC7@nand.local>
On Wed, Jul 26, 2023 at 1:04 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Jul 24, 2023 at 10:59:06AM +0200, Christian Couder wrote:
> > + /*
> > + * names has a confusing double use: it both provides the list
> > + * of just-written new packs, and accepts the name of the
> > + * filtered pack we are writing.
> > + *
> > + * By the time it is read here, it contains only the pack(s)
> > + * that were just written, which is exactly the set of packs we
> > + * want to consider kept.
> > + */
>
> I think that this comment partially comes from the cruft pack code,
> where we use the `names` string list both to reference existing packs at
> the start of the repack, and to keep track of the pack we just wrote (to
> exclude its contents from the cruft pack).
>
> But I think we only write into "names" via finish_pack_objects_cmd() to
> record the name of the pack we just wrote containing objects which
> didn't meet the filter's conditions.
>
> So I think that leaving this comment in is OK, but TBH I was on the
> fence when I wrote that back in f9825d1cf75 (builtin/repack.c: support
> generating a cruft pack, 2022-05-20), so I would just as soon drop it.
I made the comment smaller in version 4.
> > + in = xfdopen(cmd.in, "w");
> > + for_each_string_list_item(item, names)
> > + fprintf(in, "^%s-%s.pack\n", pack_prefix, item->string);
> > + for_each_string_list_item(item, existing_packs)
> > + fprintf(in, "%s.pack\n", item->string);
>
> > + for_each_string_list_item(item, existing_kept_packs)
> > + fprintf(in, "^%s.pack\n", item->string);
>
> I think we may only want to do this if `honor_pack_keep` is zero.
> Otherwise we'd avoid packing objects that appear in kept packs, even if
> the caller told us to include objects found in kept packs.
In version 4 I have made changes to better support kept packfiles and
related options, including adding tests.
> > @@ -858,6 +912,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > N_("limits the maximum number of threads")),
> > OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"),
> > N_("maximum size of each packfile")),
> > + OPT_STRING(0, "filter", &po_args.filter, N_("args"),
> > + N_("object filtering")),
>
> I suppose we're storing the filter as a string here because we're just
> going to pass it down to pack-objects directly. That part makes sense,
> but I think we are producing subtly inconsistent behavior when
> specifying multiple --filter options.
>
> IIRC, passing --filter more than once down to pack-objects produces a
> filter whose objects match all of the individually specified
> sub-filters. But IIUC, using OPT_STRING here means that later
> `--filter`'s override earlier ones.
>
> So I think at minimum we'd want to store the filter arguments in a
> strvec. But I would probably just as soon parse them into a bona-fide
> list_objects_filter_options struct, and then reconstruct the arguments
> to pack-objects based on that.
In version 4 a `list_objects_filter_options` struct is now used, and
there is a test to check that more than one `--filter=<filter-spec>`
option is supported.
> > + git -C bare.git -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
> > + test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
> > + commit_pack=$(test-tool -C bare.git find-pack HEAD) &&
> > + test -n "$commit_pack" &&
>
> I wonder if the test-tool itself should exit with a non-zero code if it
> can't find the given object in any pack. It would at least allow us to
> drop the "test -n $foo" after every invocation of the test-helper in
> this test.
>
> Arguably callers may want to ensure that an object doesn't exist in any
> pack, and this would be inconvenient for them, since they'd have to
> write something like:
>
> test_must_fail test-tool find-pack $obj
>
> but I think a more direct test like
>
> test_must_fail git cat-file -t $obj
>
> would do just as well.
Thanks for these suggestions, but I prefered to add the `--check-count
<n>` option to `test-tool find-pack` in version 4.
This way `--check-count 0` or `-c 0` for short can be used to check
that an object is in no packfile, though it could be for example in a
promisor remote or a loose object file. It's also nice to be able to
check that an object is in exactly 2 packfiles in some cases.
> > + blob_pack=$(test-tool -C bare.git find-pack HEAD:file1) &&
> > + test -n "$blob_pack" &&
> > + test "$commit_pack" != "$blob_pack" &&
> > + tree_pack=$(test-tool -C bare.git find-pack HEAD^{tree}) &&
> > + test "$tree_pack" = "$commit_pack" &&
> > + blob_pack2=$(test-tool -C bare.git find-pack HEAD:file2) &&
> > + test "$blob_pack2" = "$blob_pack"
> > +'
>
> This all looks good, but I think there are a couple of more things that
> we'd want to test for here:
>
> - That the list of all objects appears the same before and after all
> of the repacking. I think that this is tested implicitly already in
> your test, but having it written down explicitly would harden this
> against regressions that cause us to inadvertently delete an object
> we shouldn't have.
I don't think we need to test this. `git pack-objects
--filter=<filter-spec>` already existed before this series and is
tested elsewhere. We can trust that command and its tests, and just
check that we used it correctly by checking that only a few objects
are in the right packfiles.
> (FWIW, I think this would be limited to running something like "git
> cat-file --batch-check='%(objectname)' --batch-all-objects" before
> and after all of the repacking, and ensuring that the two test_cmp
> without failure).
I agree that it would not be difficult to do. I just think it's not necessary.
> - Another thing that I don't think we're testing here is that objects
> that *don't* match the filter don't appear in one of the filtered
> packs.
In version 4 we do test that for some objects, as `test-tool find-pack
-c 1 $object` would error out if the object is in more than one
packfile.
> I think we'd probably want to assert on the exact contents of
> the pack by dumping the list of objects into a file like "expect",
> and then dumping the actual set of objects with "git show-index
> <$idx | cut -d' ' -f2" or something.
>
> Another thought from the OPT_STRING business above is that we probably
> want to test this with non-trivial filter arguments. There are probably
> a handful of interesting cases here, like passing `--no-filter`, passing
> `--filter` multiple times, passing invalid values for `--filter`, etc.
In version 4 there is one test passing `--filter=...` multiple times.
I think this is enough, as the `list_objects_filter_options` struct
and related functions and mechanisms are tested elsewhere already.
> > +test_expect_success '--filter fails with --write-bitmap-index' '
> > + test_must_fail git -C bare.git repack -a -d --write-bitmap-index \
> > + --filter=blob:none &&
>
> Do we want to ensure that we get the exit code corresponding with
> showing the usage text? I could go either way, but I do think that we
> should grep through the output on stderr to ensure that we get the
> appropriate error message.
I am not sure that testing the exit code and the stderr output is
always needed. Here I think that this test is more for documentation
purposes than really enforcing something important. In fact if the
behavior would change and `--write-bitmap-index` would understand that
it should write an MIDX instead of a regular index, that behavior
change could be considered in some ways as an improvement and we would
only need to remove 'test_must_fail' here.
> > + git -C bare.git repack -a -d --no-write-bitmap-index \
> > + --filter=blob:none
>
> I don't think that this test is adding anything that the above
> "repacking with a filter works" test isn't covering already.
Ok, I have removed it in version 4.
Thanks!
next prev parent reply other threads:[~2023-08-08 17:00 UTC|newest]
Thread overview: 161+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-14 19:25 [PATCH 0/9] Repack objects into separate packfiles based on a filter Christian Couder
2023-06-14 19:25 ` [PATCH 1/9] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-06-21 10:49 ` Taylor Blau
2023-07-05 6:16 ` Christian Couder
2023-06-14 19:25 ` [PATCH 2/9] pack-objects: add `--print-filtered` to print omitted objects Christian Couder
2023-06-15 22:50 ` Junio C Hamano
2023-06-21 10:52 ` Taylor Blau
2023-06-21 11:11 ` Christian Couder
2023-06-21 11:54 ` Taylor Blau
2023-06-14 19:25 ` [PATCH 3/9] t/helper: add 'find-pack' test-tool Christian Couder
2023-06-15 23:32 ` Junio C Hamano
2023-06-21 10:40 ` Christian Couder
2023-06-21 10:54 ` Taylor Blau
2023-06-14 19:25 ` [PATCH 4/9] repack: refactor piping an oid to a command Christian Couder
2023-06-15 23:46 ` Junio C Hamano
2023-06-21 10:55 ` Taylor Blau
2023-06-21 10:56 ` Christian Couder
2023-06-14 19:25 ` [PATCH 5/9] repack: refactor finishing pack-objects command Christian Couder
2023-06-16 0:13 ` Junio C Hamano
2023-06-21 11:06 ` Taylor Blau
2023-06-21 11:19 ` Christian Couder
2023-06-21 11:05 ` Taylor Blau
2023-06-14 19:25 ` [PATCH 6/9] repack: add `--filter=<filter-spec>` option Christian Couder
2023-06-16 0:43 ` Junio C Hamano
2023-06-21 11:20 ` Taylor Blau
2023-06-21 15:04 ` Christian Couder
2023-06-22 11:05 ` Taylor Blau
2023-06-21 14:40 ` Christian Couder
2023-06-21 16:53 ` Junio C Hamano
2023-06-22 8:39 ` Christian Couder
2023-06-22 18:32 ` Junio C Hamano
2023-06-21 11:17 ` Taylor Blau
2023-07-05 7:18 ` Christian Couder
2023-06-14 19:25 ` [PATCH 7/9] gc: add `gc.repackFilter` config option Christian Couder
2023-06-14 19:25 ` [PATCH 8/9] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-06-16 2:21 ` Junio C Hamano
2023-06-21 11:49 ` Taylor Blau
2023-06-21 12:08 ` Christian Couder
2023-06-21 12:25 ` Taylor Blau
2023-06-21 16:44 ` Junio C Hamano
2023-07-05 6:19 ` Christian Couder
2023-06-14 19:25 ` [PATCH 9/9] gc: add `gc.repackFilterTo` config option Christian Couder
2023-06-16 2:54 ` Junio C Hamano
2023-06-14 21:36 ` [PATCH 0/9] Repack objects into separate packfiles based on a filter Junio C Hamano
2023-06-16 3:08 ` Junio C Hamano
2023-07-05 6:08 ` [PATCH v2 0/8] " Christian Couder
2023-07-05 6:08 ` [PATCH v2 1/8] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-07-05 6:08 ` [PATCH v2 2/8] t/helper: add 'find-pack' test-tool Christian Couder
2023-07-05 6:08 ` [PATCH v2 3/8] repack: refactor finishing pack-objects command Christian Couder
2023-07-05 6:08 ` [PATCH v2 4/8] repack: refactor finding pack prefix Christian Couder
2023-07-05 6:08 ` [PATCH v2 5/8] repack: add `--filter=<filter-spec>` option Christian Couder
2023-07-05 17:53 ` Junio C Hamano
2023-07-24 9:01 ` Christian Couder
2023-07-24 18:28 ` Junio C Hamano
2023-07-25 15:22 ` Christian Couder
2023-07-25 17:25 ` Junio C Hamano
2023-07-25 23:08 ` Junio C Hamano
2023-08-08 8:45 ` Christian Couder
2023-08-09 20:38 ` Taylor Blau
2023-08-09 22:50 ` Junio C Hamano
2023-08-09 23:38 ` Junio C Hamano
2023-08-10 0:10 ` Jeff King
2023-07-05 18:12 ` Junio C Hamano
2023-07-24 9:02 ` Christian Couder
2023-07-05 6:08 ` [PATCH v2 6/8] gc: add `gc.repackFilter` config option Christian Couder
2023-07-05 6:08 ` [PATCH v2 7/8] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-07-05 18:26 ` Junio C Hamano
2023-07-24 9:00 ` Christian Couder
2023-07-24 18:18 ` Junio C Hamano
2023-07-25 13:41 ` Robert Coup
2023-07-25 16:50 ` Junio C Hamano
2023-07-25 15:45 ` Christian Couder
2023-07-05 6:08 ` [PATCH v2 8/8] gc: add `gc.repackFilterTo` config option Christian Couder
2023-07-24 8:59 ` [PATCH v3 0/8] Repack objects into separate packfiles based on a filter Christian Couder
2023-07-24 8:59 ` [PATCH v3 1/8] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-07-25 22:38 ` Taylor Blau
2023-07-25 23:51 ` Junio C Hamano
2023-07-24 8:59 ` [PATCH v3 2/8] t/helper: add 'find-pack' test-tool Christian Couder
2023-07-25 22:44 ` Taylor Blau
2023-08-08 8:28 ` Christian Couder
2023-07-24 8:59 ` [PATCH v3 3/8] repack: refactor finishing pack-objects command Christian Couder
2023-07-25 22:45 ` Taylor Blau
2023-07-24 8:59 ` [PATCH v3 4/8] repack: refactor finding pack prefix Christian Couder
2023-07-25 22:47 ` Taylor Blau
2023-08-08 8:29 ` Christian Couder
2023-07-24 8:59 ` [PATCH v3 5/8] repack: add `--filter=<filter-spec>` option Christian Couder
2023-07-25 23:04 ` Taylor Blau
2023-08-08 8:34 ` Christian Couder [this message]
2023-08-09 21:12 ` Taylor Blau
2023-07-24 8:59 ` [PATCH v3 6/8] gc: add `gc.repackFilter` config option Christian Couder
2023-07-25 23:07 ` Taylor Blau
2023-08-08 8:38 ` Christian Couder
2023-08-09 21:15 ` Taylor Blau
2023-07-24 8:59 ` [PATCH v3 7/8] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-07-24 8:59 ` [PATCH v3 8/8] gc: add `gc.repackFilterTo` config option Christian Couder
2023-07-25 23:10 ` [PATCH v3 0/8] Repack objects into separate packfiles based on a filter Taylor Blau
2023-08-08 8:26 ` [PATCH v4 " Christian Couder
2023-08-08 8:26 ` [PATCH v4 1/8] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-08-08 8:26 ` [PATCH v4 2/8] t/helper: add 'find-pack' test-tool Christian Couder
2023-08-09 21:18 ` Taylor Blau
2023-08-08 8:26 ` [PATCH v4 3/8] repack: refactor finishing pack-objects command Christian Couder
2023-08-08 8:26 ` [PATCH v4 4/8] repack: refactor finding pack prefix Christian Couder
2023-08-09 21:20 ` Taylor Blau
2023-08-08 8:26 ` [PATCH v4 5/8] repack: add `--filter=<filter-spec>` option Christian Couder
2023-08-09 21:40 ` Taylor Blau
2023-08-08 8:26 ` [PATCH v4 6/8] gc: add `gc.repackFilter` config option Christian Couder
2023-08-08 8:26 ` [PATCH v4 7/8] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-08-08 8:26 ` [PATCH v4 8/8] gc: add `gc.repackFilterTo` config option Christian Couder
2023-08-09 21:45 ` [PATCH v4 0/8] Repack objects into separate packfiles based on a filter Taylor Blau
2023-08-09 21:57 ` Junio C Hamano
2023-08-12 0:12 ` Christian Couder
2023-08-12 0:00 ` [PATCH v5 " Christian Couder
2023-08-12 0:00 ` [PATCH v5 1/8] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-08-12 0:00 ` [PATCH v5 2/8] t/helper: add 'find-pack' test-tool Christian Couder
2023-08-12 0:00 ` [PATCH v5 3/8] repack: refactor finishing pack-objects command Christian Couder
2023-08-12 0:00 ` [PATCH v5 4/8] repack: refactor finding pack prefix Christian Couder
2023-08-12 0:00 ` [PATCH v5 5/8] repack: add `--filter=<filter-spec>` option Christian Couder
2023-08-12 0:00 ` [PATCH v5 6/8] gc: add `gc.repackFilter` config option Christian Couder
2023-08-12 0:00 ` [PATCH v5 7/8] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-08-12 0:00 ` [PATCH v5 8/8] gc: add `gc.repackFilterTo` config option Christian Couder
2023-08-15 0:51 ` [PATCH v5 0/8] Repack objects into separate packfiles based on a filter Junio C Hamano
2023-08-15 21:43 ` Taylor Blau
2023-08-15 22:32 ` Junio C Hamano
2023-08-15 23:09 ` Taylor Blau
2023-08-15 23:18 ` Junio C Hamano
2023-08-16 0:38 ` Taylor Blau
2023-08-16 17:16 ` Junio C Hamano
2023-09-11 15:20 ` Christian Couder
2023-09-11 15:06 ` [PATCH v6 0/9] " Christian Couder
2023-09-11 15:06 ` [PATCH v6 1/9] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-09-11 15:06 ` [PATCH v6 2/9] t/helper: add 'find-pack' test-tool Christian Couder
2023-09-11 15:06 ` [PATCH v6 3/9] repack: refactor finishing pack-objects command Christian Couder
2023-09-11 15:06 ` [PATCH v6 4/9] repack: refactor finding pack prefix Christian Couder
2023-09-11 15:06 ` [PATCH v6 5/9] pack-bitmap-write: rebuild using new bitmap when remapping Christian Couder
2023-09-11 15:06 ` [PATCH v6 6/9] repack: add `--filter=<filter-spec>` option Christian Couder
2023-09-11 15:06 ` [PATCH v6 7/9] gc: add `gc.repackFilter` config option Christian Couder
2023-09-11 15:06 ` [PATCH v6 8/9] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-09-11 15:06 ` [PATCH v6 9/9] gc: add `gc.repackFilterTo` config option Christian Couder
2023-09-25 15:25 ` [PATCH v7 0/9] Repack objects into separate packfiles based on a filter Christian Couder
2023-09-25 15:25 ` [PATCH v7 1/9] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-09-25 15:25 ` [PATCH v7 2/9] t/helper: add 'find-pack' test-tool Christian Couder
2023-09-25 15:25 ` [PATCH v7 3/9] repack: refactor finishing pack-objects command Christian Couder
2023-09-25 15:25 ` [PATCH v7 4/9] repack: refactor finding pack prefix Christian Couder
2023-09-25 15:25 ` [PATCH v7 5/9] pack-bitmap-write: rebuild using new bitmap when remapping Christian Couder
2023-09-25 15:25 ` [PATCH v7 6/9] repack: add `--filter=<filter-spec>` option Christian Couder
2023-09-25 15:25 ` [PATCH v7 7/9] gc: add `gc.repackFilter` config option Christian Couder
2023-09-25 15:25 ` [PATCH v7 8/9] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-09-25 15:25 ` [PATCH v7 9/9] gc: add `gc.repackFilterTo` config option Christian Couder
2023-09-25 19:14 ` [PATCH v7 0/9] Repack objects into separate packfiles based on a filter Junio C Hamano
2023-09-25 22:41 ` Taylor Blau
2023-10-02 16:54 ` [PATCH v8 " Christian Couder
2023-10-02 16:54 ` [PATCH v8 1/9] pack-objects: allow `--filter` without `--stdout` Christian Couder
2023-10-02 16:54 ` [PATCH v8 2/9] t/helper: add 'find-pack' test-tool Christian Couder
2023-10-02 16:54 ` [PATCH v8 3/9] repack: refactor finishing pack-objects command Christian Couder
2023-10-02 16:54 ` [PATCH v8 4/9] repack: refactor finding pack prefix Christian Couder
2023-10-02 16:55 ` [PATCH v8 5/9] pack-bitmap-write: rebuild using new bitmap when remapping Christian Couder
2023-10-02 16:55 ` [PATCH v8 6/9] repack: add `--filter=<filter-spec>` option Christian Couder
2023-10-02 16:55 ` [PATCH v8 7/9] gc: add `gc.repackFilter` config option Christian Couder
2023-10-02 16:55 ` [PATCH v8 8/9] repack: implement `--filter-to` for storing filtered out objects Christian Couder
2023-10-02 16:55 ` [PATCH v8 9/9] gc: add `gc.repackFilterTo` config option Christian Couder
2023-10-02 20:14 ` [PATCH v8 0/9] Repack objects into separate packfiles based on a filter Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAP8UFD3BiZQ4-M91sc6p2wBFrzPQQfja_xKpT26BcBS7OVA4MA@mail.gmail.com \
--to=christian.couder@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johncai86@gmail.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.com \
--cc=me@ttaylorr.com \
--cc=ps@pks.im \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).