Re: [PATCH] pack-objects: don't warn about bitmaps on incremental pack
From: Jeff King <hidden>
Date: 2016-12-17 04:04:37
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Fri, Dec 16, 2016 at 06:59:35PM -0500, David Turner wrote:
quoted hunk ↗ jump to hunk
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