{"thread":{"id":"44762","subject":"[PATCH v3 0/2] pack bitmaps and incremental packing","startedAt":"2016-12-24T01:09:13Z","lastAt":"2016-12-24T02:57:40Z","messageCount":4,"participants":["David Turner","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"308315","messageId":"1482541735-21346-1-git-send-email-dturner@twosigma.com","threadId":"44762","inReplyTo":null,"subject":"[PATCH v3 0/2] pack bitmaps and incremental packing","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-24T01:08:53Z","receivedAt":"2016-12-24T01:09:13Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Cleaned up the first patch's test code.\nDecided to remove the unnecessary checks from the second patch.\n\nDavid Turner (2):\n  auto gc: don't write bitmaps for incremental repacks\n  repack: die on incremental + write-bitmap-index\n\n builtin/gc.c            |  9 ++++++++-\n builtin/repack.c        |  9 +++++++++\n t/t5310-pack-bitmaps.sh |  5 +++--\n t/t6500-gc.sh           | 25 +++++++++++++++++++++++++\n 4 files changed, 45 insertions(+), 3 deletions(-)\n\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"308316","messageId":"1482541735-21346-3-git-send-email-dturner@twosigma.com","threadId":"44762","inReplyTo":"1482541735-21346-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v3 2/2] repack: die on incremental + write-bitmap-index","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-24T01:08:55Z","receivedAt":"2016-12-24T01:09:18Z","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 | 8 +++-----\n 2 files changed, 12 insertions(+), 5 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..424bec7 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -118,12 +118,10 @@ 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-\tfind .git/objects/pack -name \"*.bitmap\" >actual &&\n-\ttest_cmp expect actual\n+\ttest_must_fail git repack -d 2>err &&\n+\ttest_i18ngrep \"Incremental repacks are incompatible with bitmap\" err\n '\n \n test_expect_success 'incremental repack can disable bitmaps' '\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"308317","messageId":"1482541735-21346-2-git-send-email-dturner@twosigma.com","threadId":"44762","inReplyTo":"1482541735-21346-1-git-send-email-dturner@twosigma.com","subject":"[PATCH v3 1/2] auto gc: don't write bitmaps for incremental repacks","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-24T01:08:54Z","receivedAt":"2016-12-24T01:09:19Z","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 | 25 +++++++++++++++++++++++++\n 2 files changed, 33 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..def2aca 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -43,4 +43,29 @@ 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 | sort >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 | sort >post_packs &&\n+\tcomm --output-delimiter , -1 -3 existing_packs post_packs >new &&\n+\tcomm --output-delimiter , -2 -3 existing_packs post_packs >del &&\n+\ttest_line_count = 0 del && # No packs are deleted\n+\ttest_line_count = 2 new # There is one new pack and its .idx\n+'\n+\n+\n test_done\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"308318","messageId":"20161224025730.vwz2k4af6z6piinl@sigill.intra.peff.net","threadId":"44762","inReplyTo":"1482541735-21346-2-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3 1/2] auto gc: don't write bitmaps for incremental repacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-24T02:57:30Z","receivedAt":"2016-12-24T02:57:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 23, 2016 at 08:08:54PM -0500, David Turner wrote:\n\n> +\tgit gc --auto 2>err &&\n> +\ttest_i18ngrep ! \"^warning:\" err &&\n> +\tls .git/objects/pack/ | grep -v bitmap | sort >post_packs &&\n\nI'm not sure why you omit the bitmap here. It shouldn't change, right?\nIn that case it should not be mentioned by comm, and not impact the\ncounts you check later (and in the off chance that a new .bitmap is\ncreated, we'd want to know and fail the test, I would think).\n\n> +\tcomm --output-delimiter , -1 -3 existing_packs post_packs >new &&\n> +\tcomm --output-delimiter , -2 -3 existing_packs post_packs >del &&\n\nI don't think --output-delimiter will be portable beyond GNU comm.\n\n> +\ttest_line_count = 0 del && # No packs are deleted\n> +\ttest_line_count = 2 new # There is one new pack and its .idx\n\nThis logic makes sense (and is much nicer than the previous round).\n\n-Peff\n"}]}