{"thread":{"id":"44761","subject":"[PATCH v2 2/2] repack: die on incremental + write-bitmap-index","startedAt":"2016-12-23T19:44:39Z","lastAt":"2016-12-23T22:20:36Z","messageCount":5,"participants":["David Turner","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"308306","messageId":"1482522215-13401-2-git-send-email-dturner@twosigma.com","threadId":"44761","inReplyTo":"1482522215-13401-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v2 2/2] repack: die on incremental + write-bitmap-index","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-23T19:43:35Z","receivedAt":"2016-12-23T19:44:39Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"The bitmap index only works for single packs, so requesting an\nincremental repack with bitmap indexes makes no sense.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n builtin/repack.c        | 9 +++++++++\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 80dd06b..9c3dd09 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -18,6 +18,12 @@ static const char *const git_repack_usage[] = {\n \tNULL\n };\n \n+static const char incremental_bitmap_conflict_error[] = N_(\n+\"Incremental repacks are incompatible with bitmap indexes.  Use \\n\"\n+\"--no-write-bitmap-index or disable the pack.writebitmaps configuration.\"\n+);\n+\n+\n static int repack_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"repack.usedeltabaseoffset\")) {\n@@ -206,6 +212,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \n+\tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n+\t\tdie(incremental_bitmap_conflict_error);\n+\n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex b4c7a6f..e9a2771 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -118,10 +118,11 @@ test_expect_success 'fetch (partial bitmap)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'incremental repack cannot create bitmaps' '\n+test_expect_success 'incremental repack fails when bitmaps are requested' '\n \ttest_commit more-1 &&\n \tfind .git/objects/pack -name \"*.bitmap\" >expect &&\n-\tgit repack -d &&\n+\ttest_must_fail git repack -d 2>err &&\n+\ttest_i18ngrep \"Incremental repacks are incompatible with bitmap\" err &&\n \tfind .git/objects/pack -name \"*.bitmap\" >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"308307","messageId":"1482522215-13401-1-git-send-email-dturner@twosigma.com","threadId":"44761","inReplyTo":null,"subject":"[PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-23T19:43:34Z","receivedAt":"2016-12-23T19:44:42Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When git gc --auto does an incremental repack of loose objects, we do\nnot expect to be able to write a bitmap; it is very likely that\nobjects in the new pack will have references to objects outside of the\npack.  So we shouldn't try to write a bitmap, because doing so will\nlikely issue a warning.\n\nThis warning was making its way into gc.log.  When the gc.log was\npresent, future auto gc runs would refuse to run.\n\nPatch by Jeff King.\nBug report, test, and commit message by David Turner.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/gc.c  |  9 ++++++++-\n t/t6500-gc.sh | 22 ++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 069950d..331f219 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -191,6 +191,11 @@ static void add_repack_all_option(void)\n \t}\n }\n \n+static void add_repack_incremental_option(void)\n+{\n+       argv_array_push(&repack, \"--no-write-bitmap-index\");\n+}\n+\n static int need_to_gc(void)\n {\n \t/*\n@@ -208,7 +213,9 @@ static int need_to_gc(void)\n \t */\n \tif (too_many_packs())\n \t\tadd_repack_all_option();\n-\telse if (!too_many_loose_objects())\n+\telse if (too_many_loose_objects())\n+\t\tadd_repack_incremental_option();\n+\telse\n \t\treturn 0;\n \n \tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 5d7d414..b83a08c 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -43,4 +43,26 @@ test_expect_success 'gc is not aborted due to a stale symref' '\n \t)\n '\n \n+test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '\n+\ttest_config gc.auto 3 &&\n+\ttest_config gc.autodetach false &&\n+\ttest_config pack.writebitmaps true &&\n+\t# We need to create two object whose sha1s start with 17\n+\t# since this is what git gc counts.  As it happens, these\n+\t# two blobs will do so.\n+\ttest_commit 263 &&\n+\ttest_commit 410 &&\n+\t# Our first gc will create a pack; our second will create a second pack\n+\tgit gc --auto &&\n+\tls .git/objects/pack |grep -v bitmap >existing_packs &&\n+\ttest_commit 523 &&\n+\ttest_commit 790 &&\n+\n+\tgit gc --auto 2>err &&\n+\ttest_i18ngrep ! \"^warning:\" err &&\n+\tls .git/objects/pack/ | grep -v bitmap >post_packs &&\n+\t! test_cmp existing_packs post_packs\n+'\n+\n+\n test_done\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"308309","messageId":"xmqq4m1ue3oz.fsf@gitster.mtv.corp.google.com","threadId":"44761","inReplyTo":"1482522215-13401-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-23T21:27:40Z","receivedAt":"2016-12-23T21:27:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> +test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '\n> +\ttest_config gc.auto 3 &&\n> +\ttest_config gc.autodetach false &&\n> +\ttest_config pack.writebitmaps true &&\n> +\t# We need to create two object whose sha1s start with 17\n> +\t# since this is what git gc counts.  As it happens, these\n> +\t# two blobs will do so.\n> +\ttest_commit 263 &&\n> +\ttest_commit 410 &&\n> +\t# Our first gc will create a pack; our second will create a second pack\n> +\tgit gc --auto &&\n> +\tls .git/objects/pack |grep -v bitmap >existing_packs &&\n\nMissing SP (compare with the second invocation of the same).\n\n> +\ttest_commit 523 &&\n> +\ttest_commit 790 &&\n> +\n> +\tgit gc --auto 2>err &&\n> +\ttest_i18ngrep ! \"^warning:\" err &&\n> +\tls .git/objects/pack/ | grep -v bitmap >post_packs &&\n> +\t! test_cmp existing_packs post_packs\n\nThis does not look good for two reasons.  test_cmp tries to\nhighlight test breakage by being verbose when two files are\ndifferent while keeping quiet when they are the same.  Running it\nwith \"!\" does not change its expectation.  This undesirable effect\nshould be visible when this test is run with \"-v\" option.\n\nAnother is that this only tests if the set of packs before and after\nare different.  I think you are expecting that the second invocation\nwill create a new one while leaving the old one intact but this test\nwill not catch a breakage if the second repack instead created just\none pack replacing the old one.\n\n> +'\n> +\n> +\n>  test_done\n\nThanks.\n"},{"id":"308310","messageId":"20161223221251.2x5aykpuj6p2kvoh@sigill.intra.peff.net","threadId":"44761","inReplyTo":"1482522215-13401-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v2 1/2] auto gc: don't write bitmaps for incremental repacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-23T22:12:51Z","receivedAt":"2016-12-23T22:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 23, 2016 at 02:43:34PM -0500, David Turner wrote:\n\n> When git gc --auto does an incremental repack of loose objects, we do\n> not expect to be able to write a bitmap; it is very likely that\n> objects in the new pack will have references to objects outside of the\n> pack.  So we shouldn't try to write a bitmap, because doing so will\n> likely issue a warning.\n\nMakes sense. Another reason is that the bitmap-reading code only handles\na single bitmap. So it makes sense only to generate one during the\nall-in-one repack. I don't know if that is worth mentioning.\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nI don't remember if I signed off the original, but just for the record,\nmy signoff here is valid.\n\n-Peff\n"},{"id":"308311","messageId":"20161223222028.7zuwl2bktyyiatrl@sigill.intra.peff.net","threadId":"44761","inReplyTo":"1482522215-13401-2-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v2 2/2] repack: die on incremental + write-bitmap-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-23T22:20:28Z","receivedAt":"2016-12-23T22:20:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 23, 2016 at 02:43:35PM -0500, David Turner wrote:\n\n> The bitmap index only works for single packs, so requesting an\n> incremental repack with bitmap indexes makes no sense.\n\nOK. I doubt this will come up much after the git-gc change from the\nfirst patch. And it should already be printing a warning in this case\nanyway[1]. But I do not mind turning the warning into a hard failure; it\nmay make things more obvious for the user.  The only weird thing is that\nthe bitmap option may come from the config, so this effectively means\nyou can never run \"git repack -d\" in a repo with bitmap config. I'm not\nsure if anybody cares or not, with the new git-gc patch.\n\n[1] Technically \"repack -d\" could actually create bitmaps (with no\n    warning) in a repo that has no existing packs. But that's probably\n    an uninteresting corner case (the user should say \"-ad\" there, and\n    the \"-a\" ends up being a noop).\n\n> @@ -206,6 +212,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (pack_kept_objects < 0)\n>  \t\tpack_kept_objects = write_bitmaps;\n>  \n> +\tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n> +\t\tdie(incremental_bitmap_conflict_error);\n\nI double-checked that ALL_INTO_ONE covers both -A and -a, which I think\nshould be sufficient here.\n\n> -test_expect_success 'incremental repack cannot create bitmaps' '\n> +test_expect_success 'incremental repack fails when bitmaps are requested' '\n>  \ttest_commit more-1 &&\n>  \tfind .git/objects/pack -name \"*.bitmap\" >expect &&\n> -\tgit repack -d &&\n> +\ttest_must_fail git repack -d 2>err &&\n> +\ttest_i18ngrep \"Incremental repacks are incompatible with bitmap\" err &&\n>  \tfind .git/objects/pack -name \"*.bitmap\" >actual &&\n>  \ttest_cmp expect actual\n>  '\n\nThe final `find` is probably overkill now after we know the command\nfailed and reported the error. But it does not hurt to be thorough. :)\n\n-Peff\n"}]}