threads / discuss / 36879

Disk waste with packs and .keep files

Subject: Disk waste with packs and .keep files

## tl;dr

13 messages between Jun 10, 2014 and Jun 11, 2014.

replies: 12people: 3as markdown or json

Matthieu Moy· Jun 10, 2014, 08:21 UTC · lore
Hi,

To minimize useless on-disk changes, I have a script that periodically creates .keep files for pack files greater than 10 Mb (so than tools like unison and incremental backup remain efficient). From time to time, I delete these .keep files and "git gc" each repo. This worked well for years.

Since a few weeks however, Git started wasting my disk space: instead of creating small .pack files next to the big .keep-ed pack files, it seems to create redundant, big .pack files (i.e. I get N pack files of similar size). "git verify-pack" confirms that, for example, the object corresponding to the root commit is contained in each of the .pack file.

I don't have a reproducible way to get the situation so I didn't bisect, but "git log --grep .keep" points me to this which seems related:

  commit ee34a2beadb94a9595f09af719e3c09b485ca797
  Author: Jeff King <peff@peff.net>
  Date:   Mon Mar 3 15:04:20 2014 -0500
    repack: add `repack.packKeptObjects` config var
Now, my questions:

Is the behavior I observed actually the intended behavior? Should Git be fixed, or should I fix my flow (I guess, stop using .keep files and start using pack.packSizeLimit or so)?

Or is my message not clear enough and do I need to investigate a bit more?

Thanks,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Jeff King· Jun 10, 2014, 18:53 UTC · re: Matthieu Moy · lore

Re: Disk waste with packs and .keep files

On Tue, Jun 10, 2014 at 10:21:03AM +0200, Matthieu Moy wrote:
Show 14 quoted lines
> Since a few weeks however, Git started wasting my disk space: instead of
> creating small .pack files next to the big .keep-ed pack files, it seems
> to create redundant, big .pack files (i.e. I get N pack files of similar
> size). "git verify-pack" confirms that, for example, the object
> corresponding to the root commit is contained in each of the .pack file.
>
> I don't have a reproducible way to get the situation so I didn't bisect,
> but "git log --grep .keep" points me to this which seems related:
> 
>   commit ee34a2beadb94a9595f09af719e3c09b485ca797
>   Author: Jeff King <peff@peff.net>
>   Date:   Mon Mar 3 15:04:20 2014 -0500
> 
>     repack: add `repack.packKeptObjects` config var
Eek. Does anybody have a brown paper bag I can borrow?
-- >8 --
Subject: repack: do not accidentally pack kept objects by default

Commit ee34a2b (repack: add `repack.packKeptObjects` config var, 2014-03-03) added a flag which could duplicate kept objects, but did not mean to turn it on by default. Instead, the option is tied by default to the decision to write bitmaps, like:

  if (pack_kept_objects < 0)
	  pack_kept_objects = write_bitmap;
after which we expect pack_kept_objects to be a boolean 0 or
1.  However, that assignment neglects that write_bitmap is
_also_ a tri-state with "-1" as the default, and with
neither option given, we accidentally turn the option on.

This patch is the minimal fix to restore the desired behavior for the default state. However, the real fix will be more involved.

The decision to turn on bitmaps via config is actually made in pack-objects itself (which is why we need write_bitmap as a tri-state here; we only pass the override option if the user gave us a command-line option). To tie the options together correctly, we need to either pass the "don't know" tristate down to pack-objects (which would also read repack.packKeptObjects), or pull the reading of pack.writebitmaps up to the repack level.

Signed-off-by: Jeff King <peff@peff.net>
---
I think the latter makes the most sense, and it was a mistake to read
the option in pack-objects in the first place. We _never_ want to
write bitmaps when packing to stdout, or even when doing a non-complete
repack. We had to teach pack-objects special logic to turn bitmaps off
in that case, but the right solution instead is that pack-objects should
always respect the --write-bitmap-index flag on the command line, and
the callers should drive that decision (and really only "repack -[aA]"
would want to use it). And then the fix here will just come out
naturally from that.

I'll work up a series, but we may want to fast-track this patch for maint. It's a fairly big regression in v2.0. We didn't notice because it's only an optimization issue, not a correctness one, and I guess not that many people use .keep packs.

 builtin/repack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6b0b62d..17bc8da 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -191,7 +191,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_bitmap;
+		pack_kept_objects = write_bitmap > 0;
 
 	packdir = mkpathdup("%s/pack", get_object_directory());
 	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());

-- 
2.0.0.729.g520999f
Jeff King· Jun 10, 2014, 19:48 UTC · re: Jeff King · lore

Re: Disk waste with packs and .keep files

On Tue, Jun 10, 2014 at 02:53:21PM -0400, Jeff King wrote:
> This patch is the minimal fix to restore the desired
> behavior for the default state. However, the real fix will
> be more involved.

This patch actually breaks t7700, but because the test is wrong. Double yikes. All fixed in my series, which should be along in a moment.

-Peff
Jeff King· Jun 10, 2014, 20:07 UTC · re: Jeff King · lore

[PATCH 0/6] fix repack.packKeptObjects regression in v2.0

Here's a cleaned up version of the fix I posted earlier, with related fixes on top.

  [1/6]: repack: do not accidentally pack kept objects by default
    This is the most critical one for 'maint', as it fixes the
    regression for people who are not using bitmaps at all.
  [2/6]: repack: respect pack.writebitmaps
    We probably want to apply this one to maint, too, as the first one
    breaks people depending on repack.packKeptObjects being impacted by
    pack.writebitmaps (it never worked properly, but the breakage fixed
    by the first patch covered it up).
  [3/6]: repack: s/write_bitmap/&s/ in code
  [4/6]: pack-objects: stop respecting pack.writebitmaps
  [5/6]: repack: simplify handling of --write-bitmap-index
  [6/6]: repack: introduce repack.writeBitmaps config option
    These ones can go the normal route, but see the regression comments in
    4/6.
-Peff
Jeff King· Jun 10, 2014, 20:08 UTC · re: Jeff King · lore

[PATCH 1/6] repack: do not accidentally pack kept objects by default

Commit ee34a2b (repack: add `repack.packKeptObjects` config var, 2014-03-03) added a flag which could duplicate kept objects, but did not mean to turn it on by default. Instead, the option is tied by default to the decision to write bitmaps, like:

  if (pack_kept_objects < 0)
	  pack_kept_objects = write_bitmap;
after which we expect pack_kept_objects to be a boolean 0 or
1.  However, that assignment neglects that write_bitmap is
_also_ a tri-state with "-1" as the default, and with
neither option given, we accidentally turn the option on.

This patch is the minimal fix to restore the desired behavior for the default state. Further patches will fix the more complicated cases.

Note the update to t7700. It failed to turn on bitmaps, meaning we were actually confirming the wrong behavior!

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  | 2 +-
 t/t7700-repack.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6b0b62d..17bc8da 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -191,7 +191,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_bitmap;
+		pack_kept_objects = write_bitmap > 0;
 
 	packdir = mkpathdup("%s/pack", get_object_directory());
 	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 284018e..f054434 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -37,7 +37,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '
 
 test_expect_success 'writing bitmaps can duplicate .keep objects' '
 	# build on $objsha1, $packsha1, and .keep state from previous
-	git repack -Adl &&
+	git repack -Adbl &&
 	test_when_finished "found_duplicate_object=" &&
 	for p in .git/objects/pack/*.idx; do
 		idx=$(basename $p)

-- 
2.0.0.729.g520999f
Jeff King· Jun 11, 2014, 06:32 UTC · re: Jeff King · lore

Re: [PATCH 1/6] repack: do not accidentally pack kept objects by default

On Tue, Jun 10, 2014 at 04:08:37PM -0400, Jeff King wrote:
> Note the update to t7700. It failed to turn on bitmaps,
> meaning we were actually confirming the wrong behavior!

After writing this, I was thinking about the test, and why we didn't notice this regression. True, the test added to t7700 was checking the wrong thing. But the test _before_ it was trying to check the right thing, and still did not notice. It's because ee34a2b sabotaged it by adding an explicit --no-pack-kept-objects, so we were no longer testing the default behavior at all.

I think we should do something like this on top of the series I posted earlier. And possibly look into what kind of crack I was smoking when I wrote the original tests.

-- >8 --
Subject: 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 <peff@peff.net>
---
 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 82d39ad..021c547 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)

-- 
2.0.0.729.g520999f
Jeff King· Jun 10, 2014, 20:09 UTC · re: Jeff King · lore

[PATCH 2/6] repack: respect pack.writebitmaps

The config option to turn on bitmaps is read all the way down in the plumbing of pack-objects. This makes it hard for other options in the porcelain of repack to make decisions based on the bitmap setting. For example, repack.packKeptObjects tries to kick in by default only when bitmaps are turned on. But it can't do so reliably because it doesn't yet know whether we are using bitmaps.

This patch teaches repack to respect pack.writebitmaps. It means we pass a redundant command-line flag to pack-objects, but that's OK; it shouldn't affect the outcome.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c  |  6 +++++-
 t/t7700-repack.sh | 18 +++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 17bc8da..fab9989 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -10,6 +10,7 @@
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
+static int write_bitmap = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -27,6 +28,10 @@ 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")) {
+		write_bitmap = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -149,7 +154,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	int no_update_server_info = 0;
 	int quiet = 0;
 	int local = 0;
-	int write_bitmap = -1;
 
 	struct option builtin_repack_options[] = {
 		OPT_BIT('a', NULL, &pack_everything,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f054434..61e6ed3 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -35,7 +35,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '
 	test -z "$found_duplicate_object"
 '
 
-test_expect_success 'writing bitmaps can duplicate .keep objects' '
+test_expect_success 'writing bitmaps via command-line can duplicate .keep objects' '
 	# build on $objsha1, $packsha1, and .keep state from previous
 	git repack -Adbl &&
 	test_when_finished "found_duplicate_object=" &&
@@ -51,6 +51,22 @@ test_expect_success 'writing bitmaps can duplicate .keep objects' '
 	test "$found_duplicate_object" = 1
 '
 
+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 &&
+	test_when_finished "found_duplicate_object=" &&
+	for p in .git/objects/pack/*.idx; do
+		idx=$(basename $p)
+		test "pack-$packsha1.idx" = "$idx" && continue
+		if git verify-pack -v $p | egrep "^$objsha1"; then
+			found_duplicate_object=1
+			echo "DUPLICATE OBJECT FOUND"
+			break
+		fi
+	done &&
+	test "$found_duplicate_object" = 1
+'
+
 test_expect_success 'loose objects in alternate ODB are not repacked' '
 	mkdir alt_objects &&
 	echo `pwd`/alt_objects > .git/objects/info/alternates &&

-- 
2.0.0.729.g520999f
Jeff King· Jun 10, 2014, 20:10 UTC · re: Jeff King · lore

[PATCH 3/6] repack: s/write_bitmap/&s/ in code

The config name is "writeBitmaps", so the internal variable missing the plural is unnecessarily confusing to write.

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not critical, but after getting it wrong several times while
working on the code, I was inspired to write this.
 builtin/repack.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index fab9989..36c1cf9 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_bitmap = -1;
+static int write_bitmaps = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -29,7 +29,7 @@ static int repack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 	if (!strcmp(var, "pack.writebitmaps")) {
-		write_bitmap = git_config_bool(var, value);
+		write_bitmaps = git_config_bool(var, value);
 		return 0;
 	}
 	return git_default_config(var, value, cb);
@@ -172,7 +172,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		OPT__QUIET(&quiet, N_("be quiet")),
 		OPT_BOOL('l', "local", &local,
 				N_("pass --local to git-pack-objects")),
-		OPT_BOOL('b', "write-bitmap-index", &write_bitmap,
+		OPT_BOOL('b', "write-bitmap-index", &write_bitmaps,
 				N_("write bitmap index")),
 		OPT_STRING(0, "unpack-unreachable", &unpack_unreachable, N_("approxidate"),
 				N_("with -A, do not loosen objects older than this")),
@@ -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_bitmap > 0;
+		pack_kept_objects = write_bitmaps > 0;
 
 	packdir = mkpathdup("%s/pack", get_object_directory());
 	packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
@@ -221,9 +221,9 @@ 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_bitmap >= 0)
+	if (write_bitmaps >= 0)
 		argv_array_pushf(&cmd_args, "--%swrite-bitmap-index",
-				 write_bitmap ? "" : "no-");
+				 write_bitmaps ? "" : "no-");
 
 	if (pack_everything & ALL_INTO_ONE) {
 		get_non_kept_pack_filenames(&existing_packs);

-- 
2.0.0.729.g520999f
Jeff King· Jun 10, 2014, 20:19 UTC · re: Jeff King · lore

[PATCH 4/6] 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 <peff@peff.net>
---
I'm not sure what we want to do with this. It _is_ a possible regression
as explained above, but I really do find it improbable that anyone will
care. Even at GitHub, where we use a custom script instead of running
`git gc`, we hook into the repack code, and not directly into
pack-objects.

One option is obviously to drop it as not worth it (you don't see the benefit here, but it enables the cleanups in the rest of the series).

Another option is noting that the regression is worth dealing with, adding a deprecation notice, and phasing it out eventually. I tend to think it's not worth the trouble here.

Another option is to track it to graduate to master during the next cycle. I.e., decide that the possible regression isn't a big deal.

The final option is to track it for maint, along with the earlier patches. The argument for that is:

  1. It's not a regression worth caring about (i.e., if it's not worth
     caring about for master, it's probably not worth caring about for
     maint, either).
  2. It shortens the window in which the old behavior is in a release,
     making it less likely for somebody to try depending on it.
 builtin/pack-objects.c | 4 ----
 1 file changed, 4 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de36c60..238b502 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2214,10 +2214,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;

-- 
2.0.0.729.g520999f
Junio C Hamano· Jun 10, 2014, 21:07 UTC · re: Jeff King · lore

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> I'm not sure what we want to do with this. It _is_ a possible regression
> as explained above, but I really do find it improbable that anyone will
> care. Even at GitHub, where we use a custom script instead of running
> `git gc`, we hook into the repack code, and not directly into
> pack-objects.
>
> One option is obviously to drop it as not worth it (you don't see the
> benefit here, but it enables the cleanups in the rest of the series).
>
> Another option is noting that the regression is worth dealing with,
> adding a deprecation notice, and phasing it out eventually. I tend to
> think it's not worth the trouble here.
>
> Another option is to track it to graduate to master during the next
> cycle. I.e., decide that the possible regression isn't a big deal.

My gut feeling is that the last one is sufficient. These low level subcommands that are designed to be used by scripts (aka plumbing) shouldn't have configuration options in the first place, and users shouldn't depend on them even if they were added by design mistake.

Show 9 quoted lines
> The final option is to track it for maint, along with the earlier
> patches.  The argument for that is:
>
>   1. It's not a regression worth caring about (i.e., if it's not worth
>      caring about for master, it's probably not worth caring about for
>      maint, either).
>
>   2. It shortens the window in which the old behavior is in a release,
>      making it less likely for somebody to try depending on it.
Yeah, probably.  But I am not sure if that is even needed.
Jeff King· Jun 10, 2014, 21:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

On Tue, Jun 10, 2014 at 02:07:37PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> > Another option is to track it to graduate to master during the next
> > cycle. I.e., decide that the possible regression isn't a big deal.
> 
> My gut feeling is that the last one is sufficient.  These low level
> subcommands that are designed to be used by scripts (aka plumbing)
> shouldn't have configuration options in the first place, and users
> shouldn't depend on them even if they were added by design mistake.

That is my gut, too (and why I posted the patch). I was mostly just trying to make sure it was not me being lazy (it is easy to be so when you are the one writing the patches, rather than the one reviewing them or acting as maintainer). :)

-Peff
Jeff King· Jun 10, 2014, 20:19 UTC · re: Jeff King · lore

[PATCH 5/6] 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 <peff@peff.net>
---
Obviously needs to be dropped if we don't take the previous patch.
 builtin/repack.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 36c1cf9..0e119b7 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);

-- 
2.0.0.729.g520999f
Jeff King· Jun 10, 2014, 20:20 UTC · re: Jeff King · lore

[PATCH 6/6] 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 <peff@peff.net>
---
We can still do this if we don't take 4/6, but the "this is a synonym"
in the documentation is not quite true, then. We'd probably want to
explain the difference.
 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 cd2d651..8f0f800 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1901,12 +1901,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
@@ -2183,7 +2178,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 0e119b7..ff2216a 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 685d46f..f8ed857 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 f4f02ba..0580258 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -18,7 +18,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 61e6ed3..82d39ad 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)

-- 
2.0.0.729.g520999f

← back to recent threads