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

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

[David Turner]

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(-)

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

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

[David Turner]

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(-)

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

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

David Turner <dturner@twosigma.com> writes:

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

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

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

[Jeff King]

On Fri, Dec 23, 2016 at 02:43:34PM -0500, David Turner wrote:

> 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