mbox series

[v5,0/8] Repack objects into separate packfiles based on a filter

Message ID 20230812000011.1227371-1-christian.couder@gmail.com (mailing list archive)
Headers show
Series Repack objects into separate packfiles based on a filter | expand

Message

Christian Couder Aug. 12, 2023, midnight UTC
# Intro

Last year, John Cai sent 2 versions of a patch series to implement
`git repack --filter=<filter-spec>` and later I sent 4 versions of a
patch series trying to do it a bit differently:

  - https://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/
  - https://lore.kernel.org/git/20221012135114.294680-1-christian.couder@gmail.com/

In these patch series, the `--filter=<filter-spec>` removed the
filtered out objects altogether which was considered very dangerous
even though we implemented different safety checks in some of the
latter series.

In some discussions, it was mentioned that such a feature, or a
similar feature in `git gc`, or in a new standalone command (perhaps
called `git prune-filtered`), should put the filtered out objects into
a new packfile instead of deleting them.

Recently there were internal discussions at GitLab about either moving
blobs from inactive repos onto cheaper storage, or moving large blobs
onto cheaper storage. This lead us to rethink at repacking using a
filter, but moving the filtered out objects into a separate packfile
instead of deleting them.

So here is a new patch series doing that while implementing the
`--filter=<filter-spec>` option in `git repack`.

# Use cases for the new feature

This could be useful for example for the following purposes:

  1) As a way for servers to save storage costs by for example moving
     large blobs, or all the blobs, or all the blobs in inactive
     repos, to separate storage (while still making them accessible
     using for example the alternates mechanism).

  2) As a way to use partial clone on a Git server to offload large
     blobs to, for example, an http server, while using multiple
     promisor remotes (to be able to access everything) on the client
     side. (In this case the packfile that contains the filtered out
     object can be manualy removed after checking that all the objects
     it contains are available through the promisor remote.)

  3) As a way for clients to reclaim some space when they cloned with
     a filter to save disk space but then fetched a lot of unwanted
     objects (for example when checking out old branches) and now want
     to remove these unwanted objects. (In this case they can first
     move the packfile that contains filtered out objects to a
     separate directory or storage, then check that everything works
     well, and then manually remove the packfile after some time.)

As the features and the code are quite different from those in the
previous series, I decided to start a new series instead of continuing
a previous one.

Also since version 2 of this new series, commit messages, don't
mention uses cases like 2) or 3) above, as people have different
opinions on how it should be done. How it should be done could depend
a lot on the way promisor remotes are used, the software and hardware
setups used, etc, so it seems more difficult to "sell" this series by
talking about such use cases. As use case 1) seems simpler and more
appealing, it makes more sense to only talk about it in the commit
messages.

# Changes since version 4

Thanks to Junio who reviewed versions 1, 2, 3 and 4, and to Taylor who
reviewed version 1, 3 and 4! Thanks also to Robert Coup who
participated in the discussions related to version 2 and Peff who
participated in the discussions related to version 4. The changes are
the following:

- In patch 2/8, which introduces `test-tool find-pack`, a spurious
  space character has been removed between 'die' and '(', as suggested
  by Taylor.

- In patch 4/8, which refactors code into a find_pack_prefix()
  function, this function has been changed so that the `packdir` and
  `packtmp` arguments are now 'const', as suggested by Taylor.

- In patch 5/8, which introduces `--filter=<filter-spec>` option, the
  `filter_options` member of the 'cruft_po_args' variable is not
  initialized and freed anymore, as this member is actually unused.

- Also in patch 5/8, the '--filter fails with --write-bitmap-index'
  test has been changed to use `test_must_fail env` to fix failures
  with the 'test-lint' Makefile target, as suggested by Junio and
  Taylor. (Junio's 'SQUASH???' patch was squashed into that patch.)

- Also the series was rebased on top of v2.42.0-rc1 as it will likely
  be merged after v2.42.0 will be released and Junio's
  cc/repack-sift-filtered-objects-to-separate-pack branch is based on
  top of v2.42.0-rc0.

# Commit overview

* 1/8 pack-objects: allow `--filter` without `--stdout`

  This patch is the same as in v1, v2, v3 and v4. To be able to later
  repack with a filter we need `git pack-objects` to write packfiles
  when it's filtering instead of just writing the pack without the
  filtered out objects to stdout.

* 2/8 t/helper: add 'find-pack' test-tool

  For testing `git repack --filter=...` that we are going to
  implement, it's useful to have a test helper that can tell which
  packfiles contain a specific object. Since v4 only a space character
  has been removed between a function name and the following '(' to
  comply with our style guide.

* 3/8 repack: refactor finishing pack-objects command

  No change in this patch compared to v2, v3 and v4. This is a small
  refactoring creating a new useful function, so that `git repack
  --filter=...` will be able to reuse it.

* 4/8 repack: refactor finding pack prefix

  This is another small refactoring creating a small function that
  will be reused in the next patch. Since v4 the new function has been
  changed so that its `packdir` and `packtmp` argument are now const.

* 5/8 repack: add `--filter=<filter-spec>` option

  This actually adds the `--filter=<filter-spec>` option. It uses one
  `git pack-objects` process with the `--filter` option. And then
  another `git pack-objects` process with the `--stdin-packs`
  option. A few changes have been made since v4:

    - The `filter_options` member of the 'cruft_po_args' variable is
      not initialized and freed anymore, as this member is actually
      unused.

    - The test that checks that `--filter=...` fails with
      `--write-bitmap-index` has been changed to use `test_must_fail
      env` to fix failures with the 'test-lint' Makefile target.

* 6/8 gc: add `gc.repackFilter` config option

  No change in this patch compared to v4. This is a gc config option
  so that `git gc` can also repack using a filter and put the filtered
  out objects into a separate packfile.

* 7/8 repack: implement `--filter-to` for storing filtered out objects

  No change in this patch compared to v4. For some use cases, it's
  interesting to create the packfile that contains the filtered out
  objects into a separate location. This is similar to the
  `--expire-to` option for cruft packfiles.

* 8/8 gc: add `gc.repackFilterTo` config option

  No change in this patch compared to v3 and v4. This allows
  specifying the location of the packfile that contains the filtered
  out objects when using `gc.repackFilter`.

# Range-diff since v4

1:  09fd23c7d0 = 1:  bbcc368876 pack-objects: allow `--filter` without `--stdout`
2:  c75010d20c ! 2:  f1b80e5728 t/helper: add 'find-pack' test-tool
    @@ t/helper/test-find-pack.c (new)
     +          }
     +
     +  if (count > -1 && count != actual_count)
    -+          die ("bad packfile count %d instead of %d", actual_count, count);
    ++          die("bad packfile count %d instead of %d", actual_count, count);
     +
     +  return 0;
     +}
3:  28221861a0 = 3:  ffecc73960 repack: refactor finishing pack-objects command
4:  41d4faf62b ! 4:  6c2f381a88 repack: refactor finding pack prefix
    @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar
        return finish_pack_objects_cmd(&cmd, names, local);
      }
      
    -+static const char *find_pack_prefix(char *packdir, char *packtmp)
    ++static const char *find_pack_prefix(const char *packdir, const char *packtmp)
     +{
     +  const char *pack_prefix;
     +  if (!skip_prefix(packtmp, packdir, &pack_prefix))
5:  a929572b96 ! 5:  134700c2ce repack: add `--filter=<filter-spec>` option
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
        };
      
     +  list_objects_filter_init(&po_args.filter_options);
    -+  list_objects_filter_init(&cruft_po_args.filter_options);
     +
        git_config(repack_config, &cruft_po_args);
      
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
        string_list_clear(&existing_kept_packs, 0);
        clear_pack_geometry(geometry);
     +  list_objects_filter_release(&po_args.filter_options);
    -+  list_objects_filter_release(&cruft_po_args.filter_options);
      
        return ret;
      }
    @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila
     +'
     +
     +test_expect_success '--filter fails with --write-bitmap-index' '
    -+  GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 test_must_fail git -C bare.git repack \
    -+          -a -d --write-bitmap-index --filter=blob:none
    ++  test_must_fail \
    ++          env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
    ++          git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
     +'
     +
     +test_expect_success 'repacking with two filters works' '
6:  a22a560d74 = 6:  d3365c7b48 gc: add `gc.repackFilter` config option
7:  387b427fed = 7:  9a09382cd1 repack: implement `--filter-to` for storing filtered out objects
8:  76fac86b0e = 8:  a52e3a71db gc: add `gc.repackFilterTo` config option


Christian Couder (8):
  pack-objects: allow `--filter` without `--stdout`
  t/helper: add 'find-pack' test-tool
  repack: refactor finishing pack-objects command
  repack: refactor finding pack prefix
  repack: add `--filter=<filter-spec>` option
  gc: add `gc.repackFilter` config option
  repack: implement `--filter-to` for storing filtered out objects
  gc: add `gc.repackFilterTo` config option

 Documentation/config/gc.txt            |  16 ++
 Documentation/git-pack-objects.txt     |   4 +-
 Documentation/git-repack.txt           |  23 +++
 Makefile                               |   1 +
 builtin/gc.c                           |  10 ++
 builtin/pack-objects.c                 |   8 +-
 builtin/repack.c                       | 167 +++++++++++++++------
 t/helper/test-find-pack.c              |  50 +++++++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/t0080-find-pack.sh                   |  82 ++++++++++
 t/t5317-pack-objects-filter-objects.sh |   8 +
 t/t6500-gc.sh                          |  24 +++
 t/t7700-repack.sh                      | 197 +++++++++++++++++++++++++
 14 files changed, 542 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/test-find-pack.c
 create mode 100755 t/t0080-find-pack.sh

Comments

Junio C Hamano Aug. 15, 2023, 12:51 a.m. UTC | #1
Christian Couder <christian.couder@gmail.com> writes:

> # Changes since version 4
>
> Thanks to Junio who reviewed versions 1, 2, 3 and 4, and to Taylor who
> reviewed version 1, 3 and 4! Thanks also to Robert Coup who
> participated in the discussions related to version 2 and Peff who
> participated in the discussions related to version 4. The changes are
> the following:
>
> - In patch 2/8, which introduces `test-tool find-pack`, a spurious
>   space character has been removed between 'die' and '(', as suggested
>   by Taylor.
>
> - In patch 4/8, which refactors code into a find_pack_prefix()
>   function, this function has been changed so that the `packdir` and
>   `packtmp` arguments are now 'const', as suggested by Taylor.
>
> - In patch 5/8, which introduces `--filter=<filter-spec>` option, the
>   `filter_options` member of the 'cruft_po_args' variable is not
>   initialized and freed anymore, as this member is actually unused.
>
> - Also in patch 5/8, the '--filter fails with --write-bitmap-index'
>   test has been changed to use `test_must_fail env` to fix failures
>   with the 'test-lint' Makefile target, as suggested by Junio and
>   Taylor. (Junio's 'SQUASH???' patch was squashed into that patch.)

Thanks.  I do not recall if the previous version with SQUASH??? passed
the tests or not, but this round seems to be breaking the exact test
we had trouble with with the previous round:

  https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822

The symptom looks like that "test_must_fail env" test is not
failing.  Ring a bell?

Thanks.
Taylor Blau Aug. 15, 2023, 9:43 p.m. UTC | #2
On Mon, Aug 14, 2023 at 05:51:05PM -0700, Junio C Hamano wrote:
> Thanks.  I do not recall if the previous version with SQUASH??? passed
> the tests or not, but this round seems to be breaking the exact test
> we had trouble with with the previous round:
>
>   https://github.com/git/git/actions/runs/5850998716/job/15861158252#step:4:1822
>
> The symptom looks like that "test_must_fail env" test is not
> failing.  Ring a bell?

That does ring a bell for me, but this is a different failure than
before, IIRC.

This time we're expecting to fail writing a bitmap during a filtered
repack, but we succeed. I was wondering in [1] whether or not we should
be catching this bad combination of options more eagerly than relying on
the pack-bitmap machinery to notice that we're missing a reachability
closure.

I think the reason that this succeeds is that we already have a bitmap,
and it likely reuses all of the existing bitmaps before discovering that
the pack we wrote doesn't contain all objects. So doing this "fixes" the
immediate issue:

--- 8< ---
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 48e92aa6f7..e5134d3451 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -342,6 +342,7 @@ test_expect_success 'repacking with a filter works' '
 '

 test_expect_success '--filter fails with --write-bitmap-index' '
+	rm -f bare.git/objects/pack/*.bitmap &&
 	test_must_fail \
 		env GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 \
 		git -C bare.git repack -a -d --write-bitmap-index --filter=blob:none
--- >8 ---

but I wonder if a more complete fix would be something like:

--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index c396029ec9..f021349c4e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -48,6 +48,11 @@ static const char incremental_bitmap_conflict_error[] = N_(
 "--no-write-bitmap-index or disable the pack.writeBitmaps configuration."
 );

+static const char filtered_bitmap_conflict_error[] = N_(
+"Filtered repacks are incompatible with bitmap indexes.  Use\n"
+"--no-write-bitmap-index or disable the pack.writeBitmaps configuration."
+);
+
 struct pack_objects_args {
 	const char *window;
 	const char *window_memory;
@@ -953,7 +958,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)

 	if (write_bitmaps < 0) {
 		if (!write_midx &&
-		    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
+		    (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()) &&
+		    !po_args.filter_options.choice)
 			write_bitmaps = 0;
 	} else if (write_bitmaps &&
 		   git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0) &&
@@ -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));
+
 	if (write_bitmaps && po_args.local && has_alt_odb(the_repository)) {
 		/*
 		 * When asked to do a local repack, but we have
--- >8 ---

would be preferable.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/ZNQH6EMKqbuUzEhs@nand.local/
Junio C Hamano Aug. 15, 2023, 10:32 p.m. UTC | #3
Taylor Blau <me@ttaylorr.com> writes:

> I think the reason that this succeeds is that we already have a bitmap,
> and it likely reuses all of the existing bitmaps before discovering that
> the pack we wrote doesn't contain all objects.

Now I am confused.

We were asked to write bitmap index when we are going to create an
incomplete pack, and the packfile we generate with the filter will
not have full set of objects, and generating a bitmap with such an
incomplete knowledge of what objects are reachable from what would
be a disaster, so we should turn it off.  But the posted patch
lacked such a "we should abort when bitmap is asked to be written
while filtering" logic.

Then what were we expecting for the test to fail for?

> 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.
Taylor Blau Aug. 15, 2023, 11:09 p.m. UTC | #4
On Tue, Aug 15, 2023 at 03:32:23PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > I think the reason that this succeeds is that we already have a bitmap,
> > and it likely reuses all of the existing bitmaps before discovering that
> > the pack we wrote doesn't contain all objects.
>
> Now I am confused.
>
> We were asked to write bitmap index when we are going to create an
> incomplete pack, and the packfile we generate with the filter will
> not have full set of objects, and generating a bitmap with such an
> incomplete knowledge of what objects are reachable from what would
> be a disaster, so we should turn it off.  But the posted patch
> lacked such a "we should abort when bitmap is asked to be written
> while filtering" logic.

I was similarly confused, and started writing a patch to detect when we
see objects in one bitmap but not the other when remapping. But we
already handle that case, see the call to `rebuild_bitmap()` from
`fill_bitmap_commit()` in pack-bitmap-write.c.

So I don't think we'd ever end up reusing an existing bitmap that refers
to objects that we don't have.

But something is definitely strange here. The bitmap generated by this
test claims to have three commits:

    $ ~/src/git/t/helper/test-tool bitmap list-commits
    95a9e53327b06212dcf98bd44794b0e2b913deab
    3677360288c631b6b2e1f0e1f081b1e518605e9f
    6f105e6234717c52e9b117b08840926910a68314

...but none of them actually appear to exist in the bitmap:

    $ git rev-list --test-bitmap 95a9e53327b06212dcf98bd44794b0e2b913deab
    Bitmap v1 test (3 entries loaded)
    Found bitmap for '95a9e53327b06212dcf98bd44794b0e2b913deab'. 64 bits / 8b3b6ee7 checksum
    fatal: object not in bitmap: 'ac3e272b72bbf89def8657766b855d0656630ed4'

I think what's going on here is that we attempt to create bitmaps for
all three of those commits. We then try and reuse the existing bitmaps,
but fail, because we are missing some objects.

So then we try and generate the bitmap from scratch, and when we get
down to fill_bitmap_tree() we look up the bit position of the tree
itself, and find a non-zero answer, indicating that we have already
marked that tree.

And fill_bitmap_tree() correctly assumes that if we have marked the bit
corresponding to the tree, that everything reachable from that tree has
also been marked. So we never try and locate the bit position for the
blob, since we already think that we have a blob marked in the resulting
bitmap!

But why is that tree marked in the first place? It's because we attempt
to rebuild the bitmap from the existing .bitmap file, but fail part of
the way through (when we look up the first blob object in the reposition
table). But that happens *after* we see the tree object, so its bit
position is marked, even though we didn't rebuild a complete bitmap.

I don't think this matters outside of filtered repacks, but it would be
a serious bug to not catch this earlier up like suggested in the
(quoted) patch below.

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

Thanks,
Taylor
Junio C Hamano Aug. 15, 2023, 11:18 p.m. UTC | #5
Taylor Blau <me@ttaylorr.com> writes:

> But why is that tree marked in the first place? It's because we attempt
> to rebuild the bitmap from the existing .bitmap file, but fail part of
> the way through (when we look up the first blob object in the reposition
> table). But that happens *after* we see the tree object, so its bit
> position is marked, even though we didn't rebuild a complete bitmap.

So, there is another bug lurking, other than the lack of "combining
filtered repack and bitmaps are explicitly forbidden" logic?  We see
the tree object, we immediately mark it as "done" even we are not,
then we finish in failure and the "done" mark is left behind?  Do we
need two bits, "under review" and "done", or something then?

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

Interesting.
Taylor Blau Aug. 16, 2023, 12:38 a.m. UTC | #6
On Tue, Aug 15, 2023 at 04:18:02PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > But why is that tree marked in the first place? It's because we attempt
> > to rebuild the bitmap from the existing .bitmap file, but fail part of
> > the way through (when we look up the first blob object in the reposition
> > table). But that happens *after* we see the tree object, so its bit
> > position is marked, even though we didn't rebuild a complete bitmap.
>
> So, there is another bug lurking, other than the lack of "combining
> filtered repack and bitmaps are explicitly forbidden" logic?

I think that there is a bug lurking in the sense of trying to reuse
bitmaps when covering a pack that doesn't have reachability closure in
this particular scenario.

But there are no "blessed" use-cases for doing this. So I think that we
should indeed fix this, but I am not immediately concerned here.

> We see the tree object, we immediately mark it as "done" even we are
> not, then we finish in failure and the "done" mark is left behind?  Do
> we need two bits, "under review" and "done", or something then?

No; we can either reuse a complete bitmap or not. So it's fine to OR
all of the (permuted) bits into ent->bitmap, but it's not OK to fill in
just part of them.

Thanks,
Taylor
Junio C Hamano Aug. 16, 2023, 5:16 p.m. UTC | #7
Taylor Blau <me@ttaylorr.com> writes:

> I think that there is a bug lurking in the sense of trying to reuse
> bitmaps when covering a pack that doesn't have reachability closure in
> this particular scenario.
>
> But there are no "blessed" use-cases for doing this. So I think that we
> should indeed fix this, but I am not immediately concerned here.

OK.

> No; we can either reuse a complete bitmap or not. So it's fine to OR
> all of the (permuted) bits into ent->bitmap, but it's not OK to fill in
> just part of them.

Sounds sane.
Christian Couder Sept. 11, 2023, 3:20 p.m. UTC | #8
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.