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

[Jeff King]

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