Disk waste with packs and .keep files

Disk waste with packs and .keep files

[Matthieu Moy]

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/

Re: Disk waste with packs and .keep files

[Jeff King]

On Tue, Jun 10, 2014 at 10:21:03AM +0200, Matthieu Moy wrote:

> 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

Re: Disk waste with packs and .keep files

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

[PATCH 5/6] repack: simplify handling of --write-bitmap-index

[Jeff King]

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

[PATCH 6/6] repack: introduce repack.writeBitmaps config option

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

> 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.

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

[Jeff King]

On Tue, Jun 10, 2014 at 02:07:37PM -0700, Junio C Hamano wrote:

> > 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

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

[Jeff King]

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