Re: [PATCH] pack-objects: don't warn about bitmaps on incremental pack

[Jeff King <peff@xxxxxxxx>]

On Fri, Dec 16, 2016 at 06:59:35PM -0500, David Turner wrote:

> When running git pack-objects --incremental, 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 don't need
> to warn the user about it.
> [...]
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 0fd52bd..96de213 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1083,7 +1083,8 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,
>  	if (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset)) {
>  		/* The pack is missing an object, so it will not have closure */
>  		if (write_bitmap_index) {
> -			warning(_(no_closure_warning));
> +			if (!incremental)
> +				warning(_(no_closure_warning));
>  			write_bitmap_index = 0;
>  		}
>  		return 0;

I agree that the user doesn't need to be warned about it when running
"gc --auto", but I wonder if somebody invoking "pack-objects
--incremental --write-bitmap-index" ought to be.

In other words, your patch is detecting at a low level that we've been
given a nonsense combination of options, but should we perhaps stop
passing nonsense in the first place?

Either at the repack level, with something like:

diff --git a/builtin/repack.c b/builtin/repack.c
index 80dd06b4a2..6608a902b1 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -231,8 +231,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		argv_array_pushf(&cmd.args, "--no-reuse-delta");
 	if (no_reuse_object)
 		argv_array_pushf(&cmd.args, "--no-reuse-object");
-	if (write_bitmaps)
-		argv_array_push(&cmd.args, "--write-bitmap-index");
 
 	if (pack_everything & ALL_INTO_ONE) {
 		get_non_kept_pack_filenames(&existing_packs);
@@ -256,8 +254,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	} else {
 		argv_array_push(&cmd.args, "--unpacked");
 		argv_array_push(&cmd.args, "--incremental");
+		write_bitmap_index = 0;
 	}
 
+	if (write_bitmaps)
+		argv_array_push(&cmd.args, "--write-bitmap-index");
 	if (local)
 		argv_array_push(&cmd.args,  "--local");
 	if (quiet)

Though that still means we do not warn on:

  git repack --write-bitmap-index

which is nonsense (it is asking for an incremental repack with bitmaps).

So maybe do it at the gc level, like:

diff --git a/builtin/gc.c b/builtin/gc.c
index 069950d0b4..d3c978c765 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))

-Peff