From 15a906c5e99fc34534fd40d61590da142dee71d2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 16:19:13 -0400 Subject: [PATCH 1/4] pack-objects: stop respecting pack.writebitmaps The handling of the pack.writebitmaps config option originally happened in pack-objects, which is quite low-level. It would make more sense for drivers of pack-objects to read the config, and then manipulate pack-objects with command-line options. Recently, repack learned to do so, making the low-level read of pack.writebitmaps redundant here. Other callers, like upload-pack, would not generally want to write bitmaps anyway. This could be considered a regression for somebody who is driving pack-objects themselves outside of repack and expects the config option to be used. However, such users seem rather unlikely given how new the bitmap code is (and the fact that they would basically be reimplementing repack in the first place). Note that we do not do anything with pack.writeBitmapHashCache here. That option is not about "do we write bimaps", but rather "when we are writing bitmaps, how do we do it?". You would want that to kick in anytime you decide to write them, similar to how pack.indexVersion is used. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index fd741970e6..2175031699 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -2199,10 +2199,6 @@ static int git_pack_config(const char *k, const char *v, void *cb) cache_max_small_delta_size = git_config_int(k, v); return 0; } - if (!strcmp(k, "pack.writebitmaps")) { - write_bitmap_index = git_config_bool(k, v); - return 0; - } if (!strcmp(k, "pack.writebitmaphashcache")) { if (git_config_bool(k, v)) write_bitmap_options |= BITMAP_OPT_HASH_CACHE; From 2bed2d47b4394bd6d4ae4645be9f7424009d3c9c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 16:19:38 -0400 Subject: [PATCH 2/4] repack: simplify handling of --write-bitmap-index We previously needed to pass --no-write-bitmap-index explicitly to pack-objects to override its reading of pack.writebitmaps from the config. Now that it no longer does so, we can assume that bitmaps are off by default, and only turn them on when necessary. This also lets us avoid a confusing tri-state flag for write_bitmaps. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/repack.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index 0ec643f388..634668a7c8 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -10,7 +10,7 @@ static int delta_base_offset = 1; static int pack_kept_objects = -1; -static int write_bitmaps = -1; +static int write_bitmaps; static char *packdir, *packtmp; static const char *const git_repack_usage[] = { @@ -195,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) git_repack_usage, 0); if (pack_kept_objects < 0) - pack_kept_objects = write_bitmaps > 0; + pack_kept_objects = write_bitmaps; packdir = mkpathdup("%s/pack", get_object_directory()); packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid()); @@ -221,9 +221,8 @@ 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 >= 0) - argv_array_pushf(&cmd_args, "--%swrite-bitmap-index", - write_bitmaps ? "" : "no-"); + if (write_bitmaps) + argv_array_push(&cmd_args, "--write-bitmap-index"); if (pack_everything & ALL_INTO_ONE) { get_non_kept_pack_filenames(&existing_packs); From 71d76cb48029c94af022ea9dc9c74b4ba8bcc3a3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 10 Jun 2014 16:20:30 -0400 Subject: [PATCH 3/4] repack: introduce repack.writeBitmaps config option We currently have pack.writeBitmaps, which originally operated at the pack-objects level. This should really have been a repack.* option from day one. Let's give it the more sensible name, but keep the old version as a deprecated synonym. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 17 ++++++++++------- builtin/repack.c | 3 ++- t/perf/p5310-pack-bitmaps.sh | 3 +++ t/t5310-pack-bitmaps.sh | 2 +- t/t7700-repack.sh | 2 +- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index c0a6e0d056..411bbe5a0d 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1865,12 +1865,7 @@ pack.useBitmaps:: you are debugging pack bitmaps. pack.writebitmaps:: - When true, git will write a bitmap index when packing all - objects to disk (e.g., when `git repack -a` is run). This - index can speed up the "counting objects" phase of subsequent - packs created for clones and fetches, at the cost of some disk - space and extra time spent on the initial repack. Defaults to - false. + This is a deprecated synonym for `repack.writeBitmaps`. pack.writeBitmapHashCache:: When true, git will include a "hash cache" section in the bitmap @@ -2133,7 +2128,15 @@ repack.packKeptObjects:: `--pack-kept-objects` was passed. See linkgit:git-repack[1] for details. Defaults to `false` normally, but `true` if a bitmap index is being written (either via `--write-bitmap-index` or - `pack.writeBitmaps`). + `repack.writeBitmaps`). + +repack.writeBitmaps:: + When true, git will write a bitmap index when packing all + objects to disk (e.g., when `git repack -a` is run). This + index can speed up the "counting objects" phase of subsequent + packs created for clones and fetches, at the cost of some disk + space and extra time spent on the initial repack. Defaults to + false. rerere.autoupdate:: When set to true, `git-rerere` updates the index with the diff --git a/builtin/repack.c b/builtin/repack.c index 634668a7c8..cd4d4d4273 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -28,7 +28,8 @@ static int repack_config(const char *var, const char *value, void *cb) pack_kept_objects = git_config_bool(var, value); return 0; } - if (!strcmp(var, "pack.writebitmaps")) { + if (!strcmp(var, "repack.writebitmaps") || + !strcmp(var, "pack.writebitmaps")) { write_bitmaps = git_config_bool(var, value); return 0; } diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh index 685d46f8b7..f8ed8573b7 100755 --- a/t/perf/p5310-pack-bitmaps.sh +++ b/t/perf/p5310-pack-bitmaps.sh @@ -8,6 +8,9 @@ test_perf_large_repo # note that we do everything through config, # since we want to be able to compare bitmap-aware # git versus non-bitmap git +# +# We intentionally use the deprecated pack.writebitmaps +# config so that we can test against older versions of git. test_expect_success 'setup bitmap config' ' git config pack.writebitmaps true && git config pack.writebitmaphashcache true diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index d3a3afaba8..d81f2c7c72 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -14,7 +14,7 @@ test_expect_success 'setup repo with moderate-sized history' ' git checkout master && blob=$(echo tagged-blob | git hash-object -w --stdin) && git tag tagged-blob $blob && - git config pack.writebitmaps true && + git config repack.writebitmaps true && git config pack.writebitmaphashcache true ' diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index e70b98358b..28f7135656 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -53,7 +53,7 @@ test_expect_success 'writing bitmaps via command-line can duplicate .keep object test_expect_success 'writing bitmaps via config can duplicate .keep objects' ' # build on $objsha1, $packsha1, and .keep state from previous - git -c pack.writebitmaps=true repack -Adl && + git -c repack.writebitmaps=true repack -Adl && test_when_finished "found_duplicate_object=" && for p in .git/objects/pack/*.idx; do idx=$(basename $p) From 2d0174e38eed8143b1df0584a6c038fdaee5b89b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 11 Jun 2014 02:32:45 -0400 Subject: [PATCH 4/4] t7700: drop explicit --no-pack-kept-objects from .keep test We want to make sure that the default behavior of git-repack, without any options, continues to treat .keep files as it always has. Adding an explicit --no-pack-kept-objects, as ee34a2b did, is a much less interesting test, and prevented us from noticing the bug fixed by 64d3dc9 (repack: do not accidentally pack kept objects by default, 2014-06-10). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7700-repack.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 28f7135656..44407ae7c9 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' ' objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 | sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && mv pack-* .git/objects/pack/ && - git repack --no-pack-kept-objects -A -d -l && + git repack -A -d -l && git prune-packed && for p in .git/objects/pack/*.idx; do idx=$(basename $p)