{"thread":{"id":"36879","subject":"Disk waste with packs and .keep files","startedAt":"2014-06-10T08:21:03Z","lastAt":"2014-06-11T06:32:45Z","messageCount":13,"participants":["Matthieu Moy","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"243716","messageId":"vpqmwdljhu8.fsf@anie.imag.fr","threadId":"36879","inReplyTo":null,"subject":"Disk waste with packs and .keep files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-10T08:21:03Z","receivedAt":"2014-06-10T08:21:03Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nTo minimize useless on-disk changes, I have a script that periodically\ncreates .keep files for pack files greater than 10 Mb (so than tools\nlike unison and incremental backup remain efficient). From time to time,\nI delete these .keep files and \"git gc\" each repo. This worked well for\nyears.\n\nSince a few weeks however, Git started wasting my disk space: instead of\ncreating small .pack files next to the big .keep-ed pack files, it seems\nto create redundant, big .pack files (i.e. I get N pack files of similar\nsize). \"git verify-pack\" confirms that, for example, the object\ncorresponding to the root commit is contained in each of the .pack file.\n\nI don't have a reproducible way to get the situation so I didn't bisect,\nbut \"git log --grep .keep\" points me to this which seems related:\n\n  commit ee34a2beadb94a9595f09af719e3c09b485ca797\n  Author: Jeff King <peff@peff.net>\n  Date:   Mon Mar 3 15:04:20 2014 -0500\n\n    repack: add `repack.packKeptObjects` config var\n\nNow, my questions:\n\nIs the behavior I observed actually the intended behavior? Should Git be\nfixed, or should I fix my flow (I guess, stop using .keep files and\nstart using pack.packSizeLimit or so)?\n\nOr is my message not clear enough and do I need to investigate a bit\nmore?\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"243756","messageId":"20140610185321.GA25845@sigill.intra.peff.net","threadId":"36879","inReplyTo":"vpqmwdljhu8.fsf@anie.imag.fr","subject":"Re: Disk waste with packs and .keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T18:53:21Z","receivedAt":"2014-06-10T18:53:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 10:21:03AM +0200, Matthieu Moy wrote:\n\n> Since a few weeks however, Git started wasting my disk space: instead of\n> creating small .pack files next to the big .keep-ed pack files, it seems\n> to create redundant, big .pack files (i.e. I get N pack files of similar\n> size). \"git verify-pack\" confirms that, for example, the object\n> corresponding to the root commit is contained in each of the .pack file.\n>\n> I don't have a reproducible way to get the situation so I didn't bisect,\n> but \"git log --grep .keep\" points me to this which seems related:\n> \n>   commit ee34a2beadb94a9595f09af719e3c09b485ca797\n>   Author: Jeff King <peff@peff.net>\n>   Date:   Mon Mar 3 15:04:20 2014 -0500\n> \n>     repack: add `repack.packKeptObjects` config var\n\nEek. Does anybody have a brown paper bag I can borrow?\n\n-- >8 --\nSubject: repack: do not accidentally pack kept objects by default\n\nCommit ee34a2b (repack: add `repack.packKeptObjects` config\nvar, 2014-03-03) added a flag which could duplicate kept\nobjects, but did not mean to turn it on by default. Instead,\nthe option is tied by default to the decision to write\nbitmaps, like:\n\n  if (pack_kept_objects < 0)\n\t  pack_kept_objects = write_bitmap;\n\nafter which we expect pack_kept_objects to be a boolean 0 or\n1.  However, that assignment neglects that write_bitmap is\n_also_ a tri-state with \"-1\" as the default, and with\nneither option given, we accidentally turn the option on.\n\nThis patch is the minimal fix to restore the desired\nbehavior for the default state. However, the real fix will\nbe more involved.\n\nThe decision to turn on bitmaps via config is actually made\nin pack-objects itself (which is why we need write_bitmap as\na tri-state here; we only pass the override option if the\nuser gave us a command-line option). To tie the options\ntogether correctly, we need to either pass the \"don't know\"\ntristate down to pack-objects (which would also read\nrepack.packKeptObjects), or pull the reading of\npack.writebitmaps up to the repack level.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think the latter makes the most sense, and it was a mistake to read\nthe option in pack-objects in the first place. We _never_ want to\nwrite bitmaps when packing to stdout, or even when doing a non-complete\nrepack. We had to teach pack-objects special logic to turn bitmaps off\nin that case, but the right solution instead is that pack-objects should\nalways respect the --write-bitmap-index flag on the command line, and\nthe callers should drive that decision (and really only \"repack -[aA]\"\nwould want to use it). And then the fix here will just come out\nnaturally from that.\n\nI'll work up a series, but we may want to fast-track this patch for\nmaint. It's a fairly big regression in v2.0. We didn't notice because\nit's only an optimization issue, not a correctness one, and I guess not\nthat many people use .keep packs.\n\n builtin/repack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6b0b62d..17bc8da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tgit_repack_usage, 0);\n \n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = write_bitmap;\n+\t\tpack_kept_objects = write_bitmap > 0;\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n-- \n2.0.0.729.g520999f\n"},{"id":"243773","messageId":"20140610194822.GA23234@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610185321.GA25845@sigill.intra.peff.net","subject":"Re: Disk waste with packs and .keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T19:48:22Z","receivedAt":"2014-06-10T19:48:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 02:53:21PM -0400, Jeff King wrote:\n\n> This patch is the minimal fix to restore the desired\n> behavior for the default state. However, the real fix will\n> be more involved.\n\nThis patch actually breaks t7700, but because the test is wrong. Double\nyikes. All fixed in my series, which should be along in a moment.\n\n-Peff\n"},{"id":"243763","messageId":"20140610200741.GA11248@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610185321.GA25845@sigill.intra.peff.net","subject":"[PATCH 0/6] fix repack.packKeptObjects regression in v2.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:07:41Z","receivedAt":"2014-06-10T20:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a cleaned up version of the fix I posted earlier, with related\nfixes on top.\n\n  [1/6]: repack: do not accidentally pack kept objects by default\n\n    This is the most critical one for 'maint', as it fixes the\n    regression for people who are not using bitmaps at all.\n\n  [2/6]: repack: respect pack.writebitmaps\n\n    We probably want to apply this one to maint, too, as the first one\n    breaks people depending on repack.packKeptObjects being impacted by\n    pack.writebitmaps (it never worked properly, but the breakage fixed\n    by the first patch covered it up).\n\n  [3/6]: repack: s/write_bitmap/&s/ in code\n  [4/6]: pack-objects: stop respecting pack.writebitmaps\n  [5/6]: repack: simplify handling of --write-bitmap-index\n  [6/6]: repack: introduce repack.writeBitmaps config option\n\n    These ones can go the normal route, but see the regression comments in\n    4/6.\n\n-Peff\n"},{"id":"243765","messageId":"20140610200837.GA14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 1/6] repack: do not accidentally pack kept objects by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:08:38Z","receivedAt":"2014-06-10T20:08:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit ee34a2b (repack: add `repack.packKeptObjects` config\nvar, 2014-03-03) added a flag which could duplicate kept\nobjects, but did not mean to turn it on by default. Instead,\nthe option is tied by default to the decision to write\nbitmaps, like:\n\n  if (pack_kept_objects < 0)\n\t  pack_kept_objects = write_bitmap;\n\nafter which we expect pack_kept_objects to be a boolean 0 or\n1.  However, that assignment neglects that write_bitmap is\n_also_ a tri-state with \"-1\" as the default, and with\nneither option given, we accidentally turn the option on.\n\nThis patch is the minimal fix to restore the desired\nbehavior for the default state. Further patches will fix the\nmore complicated cases.\n\nNote the update to t7700. It failed to turn on bitmaps,\nmeaning we were actually confirming the wrong behavior!\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/repack.c  | 2 +-\n t/t7700-repack.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6b0b62d..17bc8da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tgit_repack_usage, 0);\n \n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = write_bitmap;\n+\t\tpack_kept_objects = write_bitmap > 0;\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 284018e..f054434 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -37,7 +37,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \n test_expect_success 'writing bitmaps can duplicate .keep objects' '\n \t# build on $objsha1, $packsha1, and .keep state from previous\n-\tgit repack -Adl &&\n+\tgit repack -Adbl &&\n \ttest_when_finished \"found_duplicate_object=\" &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n-- \n2.0.0.729.g520999f\n"},{"id":"243766","messageId":"20140610200923.GB14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 2/6] repack: respect pack.writebitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:09:23Z","receivedAt":"2014-06-10T20:09:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config option to turn on bitmaps is read all the way\ndown in the plumbing of pack-objects. This makes it hard for\nother options in the porcelain of repack to make decisions\nbased on the bitmap setting. For example,\nrepack.packKeptObjects tries to kick in by default only when\nbitmaps are turned on. But it can't do so reliably because\nit doesn't yet know whether we are using bitmaps.\n\nThis patch teaches repack to respect pack.writebitmaps. It\nmeans we pass a redundant command-line flag to pack-objects,\nbut that's OK; it shouldn't affect the outcome.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/repack.c  |  6 +++++-\n t/t7700-repack.sh | 18 +++++++++++++++++-\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 17bc8da..fab9989 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -10,6 +10,7 @@\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n+static int write_bitmap = -1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -27,6 +28,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tpack_kept_objects = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"pack.writebitmaps\")) {\n+\t\twrite_bitmap = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -149,7 +154,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint no_update_server_info = 0;\n \tint quiet = 0;\n \tint local = 0;\n-\tint write_bitmap = -1;\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f054434..61e6ed3 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -35,7 +35,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n-test_expect_success 'writing bitmaps can duplicate .keep objects' '\n+test_expect_success 'writing bitmaps via command-line can duplicate .keep objects' '\n \t# build on $objsha1, $packsha1, and .keep state from previous\n \tgit repack -Adbl &&\n \ttest_when_finished \"found_duplicate_object=\" &&\n@@ -51,6 +51,22 @@ test_expect_success 'writing bitmaps can duplicate .keep objects' '\n \ttest \"$found_duplicate_object\" = 1\n '\n \n+test_expect_success 'writing bitmaps via config can duplicate .keep objects' '\n+\t# build on $objsha1, $packsha1, and .keep state from previous\n+\tgit -c pack.writebitmaps=true repack -Adl &&\n+\ttest_when_finished \"found_duplicate_object=\" &&\n+\tfor p in .git/objects/pack/*.idx; do\n+\t\tidx=$(basename $p)\n+\t\ttest \"pack-$packsha1.idx\" = \"$idx\" && continue\n+\t\tif git verify-pack -v $p | egrep \"^$objsha1\"; then\n+\t\t\tfound_duplicate_object=1\n+\t\t\techo \"DUPLICATE OBJECT FOUND\"\n+\t\t\tbreak\n+\t\tfi\n+\tdone &&\n+\ttest \"$found_duplicate_object\" = 1\n+'\n+\n test_expect_success 'loose objects in alternate ODB are not repacked' '\n \tmkdir alt_objects &&\n \techo `pwd`/alt_objects > .git/objects/info/alternates &&\n-- \n2.0.0.729.g520999f\n"},{"id":"243767","messageId":"20140610201007.GC14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 3/6] repack: s/write_bitmap/&s/ in code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:10:07Z","receivedAt":"2014-06-10T20:10:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config name is \"writeBitmaps\", so the internal variable\nmissing the plural is unnecessarily confusing to write.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously not critical, but after getting it wrong several times while\nworking on the code, I was inspired to write this.\n\n builtin/repack.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex fab9989..36c1cf9 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -10,7 +10,7 @@\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n-static int write_bitmap = -1;\n+static int write_bitmaps = -1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -29,7 +29,7 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"pack.writebitmaps\")) {\n-\t\twrite_bitmap = git_config_bool(var, value);\n+\t\twrite_bitmaps = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \treturn git_default_config(var, value, cb);\n@@ -172,7 +172,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tOPT__QUIET(&quiet, N_(\"be quiet\")),\n \t\tOPT_BOOL('l', \"local\", &local,\n \t\t\t\tN_(\"pass --local to git-pack-objects\")),\n-\t\tOPT_BOOL('b', \"write-bitmap-index\", &write_bitmap,\n+\t\tOPT_BOOL('b', \"write-bitmap-index\", &write_bitmaps,\n \t\t\t\tN_(\"write bitmap index\")),\n \t\tOPT_STRING(0, \"unpack-unreachable\", &unpack_unreachable, N_(\"approxidate\"),\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n@@ -195,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tgit_repack_usage, 0);\n \n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = write_bitmap > 0;\n+\t\tpack_kept_objects = write_bitmaps > 0;\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n@@ -221,9 +221,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n-\tif (write_bitmap >= 0)\n+\tif (write_bitmaps >= 0)\n \t\targv_array_pushf(&cmd_args, \"--%swrite-bitmap-index\",\n-\t\t\t\t write_bitmap ? \"\" : \"no-\");\n+\t\t\t\t write_bitmaps ? \"\" : \"no-\");\n \n \tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs);\n-- \n2.0.0.729.g520999f\n"},{"id":"243769","messageId":"20140610201913.GD14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 4/6] pack-objects: stop respecting pack.writebitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:19:13Z","receivedAt":"2014-06-10T20:19:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The handling of the pack.writebitmaps config option\noriginally happened in pack-objects, which is quite\nlow-level. It would make more sense for drivers of\npack-objects to read the config, and then manipulate\npack-objects with command-line options.\n\nRecently, repack learned to do so, making the low-level read\nof pack.writebitmaps redundant here. Other callers, like\nupload-pack, would not generally want to write bitmaps\nanyway.\n\nThis could be considered a regression for somebody who is\ndriving pack-objects themselves outside of repack and\nexpects the config option to be used. However, such users\nseem rather unlikely given how new the bitmap code is (and\nthe fact that they would basically be reimplementing repack\nin the first place).\n\nNote that we do not do anything with pack.writeBitmapHashCache\nhere. That option is not about \"do we write bimaps\", but\nrather \"when we are writing bitmaps, how do we do it?\". You\nwould want that to kick in anytime you decide to write them,\nsimilar to how pack.indexVersion is used.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not sure what we want to do with this. It _is_ a possible regression\nas explained above, but I really do find it improbable that anyone will\ncare. Even at GitHub, where we use a custom script instead of running\n`git gc`, we hook into the repack code, and not directly into\npack-objects.\n\nOne option is obviously to drop it as not worth it (you don't see the\nbenefit here, but it enables the cleanups in the rest of the series).\n\nAnother option is noting that the regression is worth dealing with,\nadding a deprecation notice, and phasing it out eventually. I tend to\nthink it's not worth the trouble here.\n\nAnother option is to track it to graduate to master during the next\ncycle. I.e., decide that the possible regression isn't a big deal.\n\nThe final option is to track it for maint, along with the earlier\npatches.  The argument for that is:\n\n  1. It's not a regression worth caring about (i.e., if it's not worth\n     caring about for master, it's probably not worth caring about for\n     maint, either).\n\n  2. It shortens the window in which the old behavior is in a release,\n     making it less likely for somebody to try depending on it.\n\n builtin/pack-objects.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de36c60..238b502 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2214,10 +2214,6 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\tcache_max_small_delta_size = git_config_int(k, v);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(k, \"pack.writebitmaps\")) {\n-\t\twrite_bitmap_index = git_config_bool(k, v);\n-\t\treturn 0;\n-\t}\n \tif (!strcmp(k, \"pack.writebitmaphashcache\")) {\n \t\tif (git_config_bool(k, v))\n \t\t\twrite_bitmap_options |= BITMAP_OPT_HASH_CACHE;\n-- \n2.0.0.729.g520999f\n"},{"id":"243770","messageId":"20140610201938.GE14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 5/6] repack: simplify handling of --write-bitmap-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:19:38Z","receivedAt":"2014-06-10T20:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We previously needed to pass --no-write-bitmap-index\nexplicitly to pack-objects to override its reading of\npack.writebitmaps from the config. Now that it no longer\ndoes so, we can assume that bitmaps are off by default, and\nonly turn them on when necessary. This also lets us avoid a\nconfusing tri-state flag for write_bitmaps.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously needs to be dropped if we don't take the previous patch.\n\n builtin/repack.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 36c1cf9..0e119b7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -10,7 +10,7 @@\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n-static int write_bitmaps = -1;\n+static int write_bitmaps;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -195,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tgit_repack_usage, 0);\n \n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = write_bitmaps > 0;\n+\t\tpack_kept_objects = write_bitmaps;\n \n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n@@ -221,9 +221,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-object\");\n-\tif (write_bitmaps >= 0)\n-\t\targv_array_pushf(&cmd_args, \"--%swrite-bitmap-index\",\n-\t\t\t\t write_bitmaps ? \"\" : \"no-\");\n+\tif (write_bitmaps)\n+\t\targv_array_push(&cmd_args, \"--write-bitmap-index\");\n \n \tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs);\n-- \n2.0.0.729.g520999f\n"},{"id":"243771","messageId":"20140610202030.GF14974@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200741.GA11248@sigill.intra.peff.net","subject":"[PATCH 6/6] repack: introduce repack.writeBitmaps config option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:20:30Z","receivedAt":"2014-06-10T20:20:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently have pack.writeBitmaps, which originally\noperated at the pack-objects level. This should really have\nbeen a repack.* option from day one. Let's give it the more\nsensible name, but keep the old version as a deprecated\nsynonym.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe can still do this if we don't take 4/6, but the \"this is a synonym\"\nin the documentation is not quite true, then. We'd probably want to\nexplain the difference.\n\n Documentation/config.txt     | 17 ++++++++++-------\n builtin/repack.c             |  3 ++-\n t/perf/p5310-pack-bitmaps.sh |  3 +++\n t/t5310-pack-bitmaps.sh      |  2 +-\n t/t7700-repack.sh            |  2 +-\n 5 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex cd2d651..8f0f800 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1901,12 +1901,7 @@ pack.useBitmaps::\n \tyou are debugging pack bitmaps.\n \n pack.writebitmaps::\n-\tWhen true, git will write a bitmap index when packing all\n-\tobjects to disk (e.g., when `git repack -a` is run).  This\n-\tindex can speed up the \"counting objects\" phase of subsequent\n-\tpacks created for clones and fetches, at the cost of some disk\n-\tspace and extra time spent on the initial repack.  Defaults to\n-\tfalse.\n+\tThis is a deprecated synonym for `repack.writeBitmaps`.\n \n pack.writeBitmapHashCache::\n \tWhen true, git will include a \"hash cache\" section in the bitmap\n@@ -2183,7 +2178,15 @@ repack.packKeptObjects::\n \t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n \tdetails. Defaults to `false` normally, but `true` if a bitmap\n \tindex is being written (either via `--write-bitmap-index` or\n-\t`pack.writeBitmaps`).\n+\t`repack.writeBitmaps`).\n+\n+repack.writeBitmaps::\n+\tWhen true, git will write a bitmap index when packing all\n+\tobjects to disk (e.g., when `git repack -a` is run).  This\n+\tindex can speed up the \"counting objects\" phase of subsequent\n+\tpacks created for clones and fetches, at the cost of some disk\n+\tspace and extra time spent on the initial repack.  Defaults to\n+\tfalse.\n \n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0e119b7..ff2216a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -28,7 +28,8 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tpack_kept_objects = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(var, \"pack.writebitmaps\")) {\n+\tif (!strcmp(var, \"repack.writebitmaps\") ||\n+\t    !strcmp(var, \"pack.writebitmaps\")) {\n \t\twrite_bitmaps = git_config_bool(var, value);\n \t\treturn 0;\n \t}\ndiff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh\nindex 685d46f..f8ed857 100755\n--- a/t/perf/p5310-pack-bitmaps.sh\n+++ b/t/perf/p5310-pack-bitmaps.sh\n@@ -8,6 +8,9 @@ test_perf_large_repo\n # note that we do everything through config,\n # since we want to be able to compare bitmap-aware\n # git versus non-bitmap git\n+#\n+# We intentionally use the deprecated pack.writebitmaps\n+# config so that we can test against older versions of git.\n test_expect_success 'setup bitmap config' '\n \tgit config pack.writebitmaps true &&\n \tgit config pack.writebitmaphashcache true\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex f4f02ba..0580258 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -18,7 +18,7 @@ test_expect_success 'setup repo with moderate-sized history' '\n \tgit checkout master &&\n \tblob=$(echo tagged-blob | git hash-object -w --stdin) &&\n \tgit tag tagged-blob $blob &&\n-\tgit config pack.writebitmaps true &&\n+\tgit config repack.writebitmaps true &&\n \tgit config pack.writebitmaphashcache true\n '\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 61e6ed3..82d39ad 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -53,7 +53,7 @@ test_expect_success 'writing bitmaps via command-line can duplicate .keep object\n \n test_expect_success 'writing bitmaps via config can duplicate .keep objects' '\n \t# build on $objsha1, $packsha1, and .keep state from previous\n-\tgit -c pack.writebitmaps=true repack -Adl &&\n+\tgit -c repack.writebitmaps=true repack -Adl &&\n \ttest_when_finished \"found_duplicate_object=\" &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n-- \n2.0.0.729.g520999f\n"},{"id":"243777","messageId":"xmqqoay08odi.fsf@gitster.dls.corp.google.com","threadId":"36879","inReplyTo":"20140610201913.GD14974@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T21:07:37Z","receivedAt":"2014-06-10T21:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm not sure what we want to do with this. It _is_ a possible regression\n> as explained above, but I really do find it improbable that anyone will\n> care. Even at GitHub, where we use a custom script instead of running\n> `git gc`, we hook into the repack code, and not directly into\n> pack-objects.\n>\n> One option is obviously to drop it as not worth it (you don't see the\n> benefit here, but it enables the cleanups in the rest of the series).\n>\n> Another option is noting that the regression is worth dealing with,\n> adding a deprecation notice, and phasing it out eventually. I tend to\n> think it's not worth the trouble here.\n>\n> Another option is to track it to graduate to master during the next\n> cycle. I.e., decide that the possible regression isn't a big deal.\n\nMy gut feeling is that the last one is sufficient.  These low level\nsubcommands that are designed to be used by scripts (aka plumbing)\nshouldn't have configuration options in the first place, and users\nshouldn't depend on them even if they were added by design mistake.\n\n> The final option is to track it for maint, along with the earlier\n> patches.  The argument for that is:\n>\n>   1. It's not a regression worth caring about (i.e., if it's not worth\n>      caring about for master, it's probably not worth caring about for\n>      maint, either).\n>\n>   2. It shortens the window in which the old behavior is in a release,\n>      making it less likely for somebody to try depending on it.\n\nYeah, probably.  But I am not sure if that is even needed.\n"},{"id":"243780","messageId":"20140610212946.GB15332@sigill.intra.peff.net","threadId":"36879","inReplyTo":"xmqqoay08odi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:29:46Z","receivedAt":"2014-06-10T21:29:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 02:07:37PM -0700, Junio C Hamano wrote:\n\n> > Another option is to track it to graduate to master during the next\n> > cycle. I.e., decide that the possible regression isn't a big deal.\n> \n> My gut feeling is that the last one is sufficient.  These low level\n> subcommands that are designed to be used by scripts (aka plumbing)\n> shouldn't have configuration options in the first place, and users\n> shouldn't depend on them even if they were added by design mistake.\n\nThat is my gut, too (and why I posted the patch). I was mostly just\ntrying to make sure it was not me being lazy (it is easy to be so when\nyou are the one writing the patches, rather than the one reviewing\nthem or acting as maintainer). :)\n\n-Peff\n"},{"id":"243868","messageId":"20140611063244.GA32333@sigill.intra.peff.net","threadId":"36879","inReplyTo":"20140610200837.GA14974@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] repack: do not accidentally pack kept objects by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-11T06:32:45Z","receivedAt":"2014-06-11T06:32:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 04:08:37PM -0400, Jeff King wrote:\n\n> Note the update to t7700. It failed to turn on bitmaps,\n> meaning we were actually confirming the wrong behavior!\n\nAfter writing this, I was thinking about the test, and why we didn't\nnotice this regression. True, the test added to t7700 was checking the\nwrong thing. But the test _before_ it was trying to check the right\nthing, and still did not notice. It's because ee34a2b sabotaged it by\nadding an explicit --no-pack-kept-objects, so we were no longer testing\nthe default behavior at all.\n\nI think we should do something like this on top of the series I posted\nearlier. And possibly look into what kind of crack I was smoking when I\nwrote the original tests.\n\n-- >8 --\nSubject: t7700: drop explicit --no-pack-kept-objects from .keep test\n\nWe want to make sure that the default behavior of git-repack,\nwithout any options, continues to treat .keep files as it\nalways has. Adding an explicit --no-pack-kept-objects, as\nee34a2b did, is a much less interesting test, and prevented\nus from noticing the bug fixed by 64d3dc9 (repack: do not\naccidentally pack kept objects by default, 2014-06-10).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7700-repack.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 82d39ad..021c547 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \tobjsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |\n \t\tsed -e \"s/^\\([0-9a-f]\\{40\\}\\).*/\\1/\") &&\n \tmv pack-* .git/objects/pack/ &&\n-\tgit repack --no-pack-kept-objects -A -d -l &&\n+\tgit repack -A -d -l &&\n \tgit prune-packed &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n-- \n2.0.0.729.g520999f\n"}]}