From cc2649ae2dac565202bb9041cbb54b29e9f501b3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 31 Jul 2019 01:37:36 -0400 Subject: t7700: clean up .keep file in bitmap-writing test After our test snippet finishes, the .keep file is left in place, making it hard to do further tests of the auto-bitmap-writing code (since it suppresses the feature completely). Let's clean it up. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 0e9af83..8d9a358 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -243,6 +243,7 @@ test_expect_success 'no bitmaps created if .keep files present' ' pack=$(ls bare.git/objects/pack/*.pack) && test_path_is_file "$pack" && keep=${pack%.pack}.keep && + test_when_finished "rm -f \"\$keep\"" && >"$keep" && git -C bare.git repack -ad && find bare.git/objects/pack/ -type f -name "*.bitmap" >actual && -- cgit v0.10.2-6-g49f6 From 25575015cafbbb8554ff6ac82236c2a062055ae6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 31 Jul 2019 01:39:27 -0400 Subject: repack: silence warnings when auto-enabled bitmaps cannot be built Depending on various config options, a full repack may not be able to build a reachability bitmap index (e.g., if pack.packSizeLimit forces us to write multiple packs). In these cases pack-objects may write a warning to stderr. Since 36eba0323d (repack: enable bitmaps by default on bare repos, 2019-03-14), we may generate these warnings even when the user did not explicitly ask for bitmaps. This has two downsides: - it can be confusing, if they don't know what bitmaps are - a daemonized auto-gc will write this to its log file, and the presence of the warning may suppress further auto-gc (until gc.logExpiry has elapsed) Let's have repack communicate to pack-objects that the choice to turn on bitmaps was not made explicitly by the user, which in turn allows pack-objects to suppress these warnings. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 267c562..76ce906 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -96,7 +96,11 @@ static off_t reuse_packfile_offset; static int use_bitmap_index_default = 1; static int use_bitmap_index = -1; -static int write_bitmap_index; +static enum { + WRITE_BITMAP_FALSE = 0, + WRITE_BITMAP_QUIET, + WRITE_BITMAP_TRUE, +} write_bitmap_index; static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE; static int exclude_promisor_objects; @@ -892,7 +896,8 @@ static void write_pack_file(void) nr_written, oid.hash, offset); close(fd); if (write_bitmap_index) { - warning(_(no_split_warning)); + if (write_bitmap_index != WRITE_BITMAP_QUIET) + warning(_(no_split_warning)); write_bitmap_index = 0; } } @@ -1176,7 +1181,8 @@ static int add_object_entry(const struct object_id *oid, enum object_type type, if (!want_object_in_pack(oid, 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 (write_bitmap_index != WRITE_BITMAP_QUIET) + warning(_(no_closure_warning)); write_bitmap_index = 0; } return 0; @@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("do not hide commits by grafts"), 0), OPT_BOOL(0, "use-bitmap-index", &use_bitmap_index, N_("use a bitmap index if available to speed up counting objects")), - OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index, - N_("write a bitmap index together with the pack index")), + OPT_SET_INT(0, "write-bitmap-index", &write_bitmap_index, + N_("write a bitmap index together with the pack index"), + WRITE_BITMAP_TRUE), + OPT_SET_INT_F(0, "write-bitmap-index-quiet", + &write_bitmap_index, + N_("write a bitmap index if possible"), + WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN), OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), { OPTION_CALLBACK, 0, "missing", NULL, N_("action"), N_("handling for missing objects"), PARSE_OPT_NONEG, diff --git a/builtin/repack.c b/builtin/repack.c index 30982ed..db93ca3 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix) die(_("--keep-unreachable and -A are incompatible")); if (write_bitmaps < 0) { - write_bitmaps = (pack_everything & ALL_INTO_ONE) && - is_bare_repository() && - keep_pack_list.nr == 0 && - !has_pack_keep_file(); + if (!(pack_everything & ALL_INTO_ONE) || + !is_bare_repository() || + keep_pack_list.nr != 0 || + has_pack_keep_file()) + write_bitmaps = 0; } if (pack_kept_objects < 0) - pack_kept_objects = write_bitmaps; + pack_kept_objects = !!write_bitmaps; if (write_bitmaps && !(pack_everything & ALL_INTO_ONE)) die(_(incremental_bitmap_conflict_error)); @@ -375,8 +376,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix) argv_array_push(&cmd.args, "--indexed-objects"); if (repository_format_partial_clone) argv_array_push(&cmd.args, "--exclude-promisor-objects"); - if (write_bitmaps) + if (write_bitmaps > 0) argv_array_push(&cmd.args, "--write-bitmap-index"); + else if (write_bitmaps < 0) + argv_array_push(&cmd.args, "--write-bitmap-index-quiet"); if (use_delta_islands) argv_array_push(&cmd.args, "--delta-islands"); diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 8d9a358..54f815b 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -250,4 +250,15 @@ test_expect_success 'no bitmaps created if .keep files present' ' test_must_be_empty actual ' +test_expect_success 'auto-bitmaps do not complain if unavailable' ' + test_config -C bare.git pack.packSizeLimit 1M && + blob=$(test-tool genrandom big $((1024*1024)) | + git -C bare.git hash-object -w --stdin) && + git -C bare.git update-ref refs/tags/big $blob && + git -C bare.git repack -ad 2>stderr && + test_must_be_empty stderr && + find bare.git/objects/pack -type f -name "*.bitmap" >actual && + test_must_be_empty actual +' + test_done -- cgit v0.10.2-6-g49f6 From 7ff024e7b3d576fc265dbdd1a7bd3dcc6dde1eb6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 31 Jul 2019 01:40:56 -0400 Subject: repack: simplify handling of auto-bitmaps and .keep files Commit 7328482253 (repack: disable bitmaps-by-default if .keep files exist, 2019-06-29) taught repack to prefer disabling bitmaps to duplicating objects (unless bitmaps were asked for explicitly). But there's an easier way to do this: if we keep passing the --honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then pack-objects already makes the same decision (it will disable bitmaps rather than duplicate). Better still, pack-objects can actually decide to do so based not just on the presence of a .keep file, but on whether that .keep file actually impacts the new pack we're making (so if we're racing with a push or fetch, for example, their temporary .keep file will not block us from generating bitmaps if they haven't yet updated their refs). And because repack uses the --write-bitmap-index-quiet flag, we don't have to worry about pack-objects generating confusing warnings when it does see a .keep file. We can confirm this by tweaking the .keep test to check repack's stderr. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/repack.c b/builtin/repack.c index db93ca3..632c0c0 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -89,17 +89,6 @@ static void remove_pack_on_signal(int signo) raise(signo); } -static int has_pack_keep_file(void) -{ - struct packed_git *p; - - for (p = get_all_packs(the_repository); p; p = p->next) { - if (p->pack_keep) - return 1; - } - return 0; -} - /* * Adds all packs hex strings to the fname list, which do not * have a corresponding .keep file. These packs are not to @@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (write_bitmaps < 0) { if (!(pack_everything & ALL_INTO_ONE) || - !is_bare_repository() || - keep_pack_list.nr != 0 || - has_pack_keep_file()) + !is_bare_repository()) write_bitmaps = 0; } if (pack_kept_objects < 0) - pack_kept_objects = !!write_bitmaps; + pack_kept_objects = write_bitmaps > 0; if (write_bitmaps && !(pack_everything & ALL_INTO_ONE)) die(_(incremental_bitmap_conflict_error)); diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 54f815b..4e855bc 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' ' keep=${pack%.pack}.keep && test_when_finished "rm -f \"\$keep\"" && >"$keep" && - git -C bare.git repack -ad && + git -C bare.git repack -ad 2>stderr && + test_must_be_empty stderr && find bare.git/objects/pack/ -type f -name "*.bitmap" >actual && test_must_be_empty actual ' -- cgit v0.10.2-6-g49f6