Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, 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>
Subject: Re: [PATCH v5 0/8] Repack objects into separate packfiles based on a filter
Date: Mon, 11 Sep 2023 17:20:27 +0200	[thread overview]
Message-ID: <CAP8UFD2kFjcD29kCp4j1AceCs=mr+_vN+sT5AVnoQNJBaoFt8A@mail.gmail.com> (raw)
In-Reply-To: <ZNwFlcS3SOS9h77N@nand.local>

On Wed, Aug 16, 2023 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Aug 15, 2023 at 03:32:23PM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:

> > > but I wonder if a more complete fix would be something like:
> > > ...
> > > @@ -966,6 +972,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > >     if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
> > >             die(_(incremental_bitmap_conflict_error));
> > >
> > > +   if (write_bitmaps && po_args.filter_options.choice)
> > > +           die(_(filtered_bitmap_conflict_error));
> > > +
> >
> > It sounds like the most direct fix.

I would be Ok with such a fix, if we think that we don't want to fix
the underlying issue, or if we think that fixing the underlying issue
is not enough...

> I agree.
>
> I think that we would be OK to not change the implementation of
> rebuild_bitmap(), or its caller in fill_bitmap_commit(), since this only
> bites us when bitmapping a filtered pack, and we should catch that case
> well before getting this deep into the bitmap code.
>
> But it does seem suspect that we rebuild right into ent->bitmap, so we
> may want to consider doing something like:
>
> --- >8 ---
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index f6757c3cbf..f4ecdf8b0e 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -413,15 +413,19 @@ static int fill_bitmap_commit(struct bb_commit *ent,
>
>                 if (old_bitmap && mapping) {
>                         struct ewah_bitmap *old = bitmap_for_commit(old_bitmap, c);
> +                       struct bitmap *remapped = bitmap_new();
>                         /*
>                          * If this commit has an old bitmap, then translate that
>                          * bitmap and add its bits to this one. No need to walk
>                          * parents or the tree for this commit.
>                          */
> -                       if (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {
> +                       if (old && !rebuild_bitmap(mapping, old, remapped)) {
> +                               bitmap_or(ent->bitmap, remapped);
> +                               bitmap_free(remapped);
>                                 reused_bitmaps_nr++;
>                                 continue;
>                         }
> +                       bitmap_free(remapped);
>                 }
>
>                 /*
> --- 8< ---
>
> on top.
>
> Applying that patch and then rerunning the tests with the appropriate
> TEST variables causes the 'git repack' to fail as expected, ensuring
> that the containing test passes.

...however I think that fixing this underlying issue is important, as
it might cause other tricky issues in the future, for example if other
bitmap code is copying or reusing this code.

So I just sent a version 6 of this series with this change in a new
patch. I hope my explanations in the commit message are good enough.

Thanks for finding the cause of the CI test failures and suggesting this fix,
Christian.

  parent reply	other threads:[~2023-09-11 21:38 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
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 [this message]
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='CAP8UFD2kFjcD29kCp4j1AceCs=mr+_vN+sT5AVnoQNJBaoFt8A@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --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).