threads / patch / 44761

v2, 2 partsrepack: die on incremental + write-bitmap-index

Subject: [PATCH v2 2/2] repack: die on incremental + write-bitmap-index

## tl;dr

5 messages between Dec 23, 2016 and Dec 23, 2016. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

David Turner· Dec 23, 2016, 19:43 UTC · lore

[PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks

When git gc --auto does an incremental repack of loose objects, we do not expect to be able to write a bitmap; it is very likely that objects in the new pack will have references to objects outside of the pack. So we shouldn't try to write a bitmap, because doing so will likely issue a warning.

This warning was making its way into gc.log. When the gc.log was present, future auto gc runs would refuse to run.

Patch by Jeff King. Bug report, test, and commit message by David Turner.

Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c  |  9 ++++++++-
 t/t6500-gc.sh | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
Show changes to 2 files +30 −1

builtin/gc.c, t/t6500-gc.sh

diff --git a/builtin/gc.c b/builtin/gc.c
index 069950d..331f219 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -191,6 +191,11 @@ static void add_repack_all_option(void)
 	}
 }
 
+static void add_repack_incremental_option(void)
+{
+       argv_array_push(&repack, "--no-write-bitmap-index");
+}
+
 static int need_to_gc(void)
 {
 	/*
@@ -208,7 +213,9 @@ static int need_to_gc(void)
 	 */
 	if (too_many_packs())
 		add_repack_all_option();
-	else if (!too_many_loose_objects())
+	else if (too_many_loose_objects())
+		add_repack_incremental_option();
+	else
 		return 0;
 
 	if (run_hook_le(NULL, "pre-auto-gc", NULL))
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 5d7d414..b83a08c 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -43,4 +43,26 @@ test_expect_success 'gc is not aborted due to a stale symref' '
 	)
 '
 
+test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '
+	test_config gc.auto 3 &&
+	test_config gc.autodetach false &&
+	test_config pack.writebitmaps true &&
+	# We need to create two object whose sha1s start with 17
+	# since this is what git gc counts.  As it happens, these
+	# two blobs will do so.
+	test_commit 263 &&
+	test_commit 410 &&
+	# Our first gc will create a pack; our second will create a second pack
+	git gc --auto &&
+	ls .git/objects/pack |grep -v bitmap >existing_packs &&
+	test_commit 523 &&
+	test_commit 790 &&
+
+	git gc --auto 2>err &&
+	test_i18ngrep ! "^warning:" err &&
+	ls .git/objects/pack/ | grep -v bitmap >post_packs &&
+	! test_cmp existing_packs post_packs
+'
+
+
 test_done

-- 
2.8.0.rc4.22.g8ae061a
David Turner· Dec 23, 2016, 19:43 UTC · re: David Turner · lore

The bitmap index only works for single packs, so requesting an incremental repack with bitmap indexes makes no sense.

Signed-off-by: David Turner <dturner@twosigma.com>
---
 builtin/repack.c        | 9 +++++++++
 t/t5310-pack-bitmaps.sh | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)
Show changes to 2 files +12 −2

builtin/repack.c, t/t5310-pack-bitmaps.sh

diff --git a/builtin/repack.c b/builtin/repack.c
index 80dd06b..9c3dd09 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -18,6 +18,12 @@ static const char *const git_repack_usage[] = {
 	NULL
 };
 
+static const char incremental_bitmap_conflict_error[] = N_(
+"Incremental repacks are incompatible with bitmap indexes.  Use \n"
+"--no-write-bitmap-index or disable the pack.writebitmaps configuration."
+);
+
+
 static int repack_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "repack.usedeltabaseoffset")) {
@@ -206,6 +212,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (pack_kept_objects < 0)
 		pack_kept_objects = write_bitmaps;
 
+	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
+		die(incremental_bitmap_conflict_error);
+
 	packdir = mkpathdup("%s/pack", get_object_directory());
 	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index b4c7a6f..e9a2771 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -118,10 +118,11 @@ test_expect_success 'fetch (partial bitmap)' '
 	test_cmp expect actual
 '
 
-test_expect_success 'incremental repack cannot create bitmaps' '
+test_expect_success 'incremental repack fails when bitmaps are requested' '
 	test_commit more-1 &&
 	find .git/objects/pack -name "*.bitmap" >expect &&
-	git repack -d &&
+	test_must_fail git repack -d 2>err &&
+	test_i18ngrep "Incremental repacks are incompatible with bitmap" err &&
 	find .git/objects/pack -name "*.bitmap" >actual &&
 	test_cmp expect actual
 '

-- 
2.8.0.rc4.22.g8ae061a
Jeff King· Dec 23, 2016, 22:20 UTC · re: David Turner · lore

Re: [PATCH v2 2/2] repack: die on incremental + write-bitmap-index

On Fri, Dec 23, 2016 at 02:43:35PM -0500, David Turner wrote:
> The bitmap index only works for single packs, so requesting an
> incremental repack with bitmap indexes makes no sense.

OK. I doubt this will come up much after the git-gc change from the first patch. And it should already be printing a warning in this case anyway[1]. But I do not mind turning the warning into a hard failure; it may make things more obvious for the user. The only weird thing is that the bitmap option may come from the config, so this effectively means you can never run "git repack -d" in a repo with bitmap config. I'm not sure if anybody cares or not, with the new git-gc patch.

[1] Technically "repack -d" could actually create bitmaps (with no
    warning) in a repo that has no existing packs. But that's probably
    an uninteresting corner case (the user should say "-ad" there, and
    the "-a" ends up being a noop).
Show 6 quoted lines
> @@ -206,6 +212,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (pack_kept_objects < 0)
>  		pack_kept_objects = write_bitmaps;
>  
> +	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
> +		die(incremental_bitmap_conflict_error);

I double-checked that ALL_INTO_ONE covers both -A and -a, which I think should be sufficient here.

Show 10 quoted lines
> -test_expect_success 'incremental repack cannot create bitmaps' '
> +test_expect_success 'incremental repack fails when bitmaps are requested' '
>  	test_commit more-1 &&
>  	find .git/objects/pack -name "*.bitmap" >expect &&
> -	git repack -d &&
> +	test_must_fail git repack -d 2>err &&
> +	test_i18ngrep "Incremental repacks are incompatible with bitmap" err &&
>  	find .git/objects/pack -name "*.bitmap" >actual &&
>  	test_cmp expect actual
>  '

The final `find` is probably overkill now after we know the command failed and reported the error. But it does not hurt to be thorough. :)

-Peff
Junio C Hamano· Dec 23, 2016, 21:27 UTC · re: David Turner · lore

Re: [PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks

David Turner <dturner@twosigma.com> writes:
Show 12 quoted lines
> +test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '
> +	test_config gc.auto 3 &&
> +	test_config gc.autodetach false &&
> +	test_config pack.writebitmaps true &&
> +	# We need to create two object whose sha1s start with 17
> +	# since this is what git gc counts.  As it happens, these
> +	# two blobs will do so.
> +	test_commit 263 &&
> +	test_commit 410 &&
> +	# Our first gc will create a pack; our second will create a second pack
> +	git gc --auto &&
> +	ls .git/objects/pack |grep -v bitmap >existing_packs &&
Missing SP (compare with the second invocation of the same).
Show 7 quoted lines
> +	test_commit 523 &&
> +	test_commit 790 &&
> +
> +	git gc --auto 2>err &&
> +	test_i18ngrep ! "^warning:" err &&
> +	ls .git/objects/pack/ | grep -v bitmap >post_packs &&
> +	! test_cmp existing_packs post_packs

This does not look good for two reasons. test_cmp tries to highlight test breakage by being verbose when two files are different while keeping quiet when they are the same. Running it with "!" does not change its expectation. This undesirable effect should be visible when this test is run with "-v" option.

Another is that this only tests if the set of packs before and after are different. I think you are expecting that the second invocation will create a new one while leaving the old one intact but this test will not catch a breakage if the second repack instead created just one pack replacing the old one.

> +'
> +
> +
>  test_done
Thanks.
Jeff King· Dec 23, 2016, 22:12 UTC · re: David Turner · lore

Re: [PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks

On Fri, Dec 23, 2016 at 02:43:34PM -0500, David Turner wrote:
Show 5 quoted lines
> When git gc --auto does an incremental repack of loose objects, we do
> not expect to be able to write a bitmap; it is very likely that
> objects in the new pack will have references to objects outside of the
> pack.  So we shouldn't try to write a bitmap, because doing so will
> likely issue a warning.

Makes sense. Another reason is that the bitmap-reading code only handles a single bitmap. So it makes sense only to generate one during the all-in-one repack. I don't know if that is worth mentioning.

> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Jeff King <peff@peff.net>

I don't remember if I signed off the original, but just for the record, my signoff here is valid.

-Peff

← back to recent threads