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
