{"thread":{"id":"36168","subject":"[PATCH] pack-objects: turn off bitmaps when skipping objects","startedAt":"2014-03-15T02:38:29Z","lastAt":"2014-03-15T02:38:29Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236774","messageId":"20140315023828.GA6790@sigill.intra.peff.net","threadId":"36168","inReplyTo":null,"subject":"[PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-15T02:38:29Z","receivedAt":"2014-03-15T02:38:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The pack bitmap format requires that we have a single bit\nfor each object in the pack, and that each object's bitmap\nrepresents its complete set of reachable objects. Therefore\nwe have no way to represent the bitmap of an object which\nreferences objects outside the pack.\n\nWe notice this problem while generating the bitmaps, as we\ntry to find the offset of a particular object and realize\nthat we do not have it. In this case we die, and neither the\nbitmap nor the pack is generated. This is correct, but\nperhaps a little unfriendly. If you have bitmaps turned on\nin the config, many repacks will fail which would otherwise\nsucceed. E.g., incremental repacks, repacks with \"-l\" when\nyou have alternates, \".keep\" files.\n\nInstead, this patch notices early that we are omitting some\nobjects from the pack and turns off bitmaps (with a\nwarning). Note that this is not strictly correct, as it's\npossible that the object being omitted is not reachable from\nany other object in the pack. In practice, this is almost\nnever the case, and there are two advantages to doing it\nthis way:\n\n  1. The code is much simpler, as we do not have to cleanly\n     abort the bitmap-generation process midway through.\n\n  2. We do not waste time partially generating bitmaps only\n     to find out that some object deep in the history is not\n     being packed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI posted this earlier here:\n\n  http://article.gmane.org/gmane.comp.version-control.git/240969\n\nThe discussion resulted in the jk/repack-pack-keep-objects topic.\nHowever, I think this is still worth applying, as it means git behaves\nsensibly when objects are omitted for other reasons (e.g., because you\ntried to use \"-b\" with an incremental repack, or because you favor\n\".keep\" files to bitmaps by explicitly setting repack.packKeptObjects\nto \"false\"). In our previous discussions, I had assumed this patch had\nalready been picked up, but I don't see it anywhere. Without it, setting\n\"repack.packKeptObjects\" to false is largely pointless (instead of\ncontinuing without bitmaps, git will die).\n\n builtin/pack-objects.c  | 12 +++++++++++-\n t/t5310-pack-bitmaps.sh |  5 ++++-\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 418801f..4ca3946 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1011,6 +1011,10 @@ static void create_object_entry(const unsigned char *sha1,\n \tentry->no_try_delta = no_try_delta;\n }\n \n+static const char no_closure_warning[] = N_(\n+\"disabling bitmap writing, as some objects are not being packed\"\n+);\n+\n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\t\t    const char *name, int exclude)\n {\n@@ -1021,8 +1025,14 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \tif (have_duplicate_entry(sha1, exclude, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset))\n+\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset)) {\n+\t\t/* The pack is missing an object, so it will not have closure */\n+\t\tif (write_bitmap_index) {\n+\t\t\twarning(_(no_closure_warning));\n+\t\t\twrite_bitmap_index = 0;\n+\t\t}\n \t\treturn 0;\n+\t}\n \n \tcreate_object_entry(sha1, type, pack_name_hash(name),\n \t\t\t    exclude, name && no_try_delta(name),\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex d3a3afa..f13525c 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -91,7 +91,10 @@ test_expect_success 'fetch (partial bitmap)' '\n \n test_expect_success 'incremental repack cannot create bitmaps' '\n \ttest_commit more-1 &&\n-\ttest_must_fail git repack -d\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 '\n \n test_expect_success 'incremental repack can disable bitmaps' '\n-- \n1.9.0.417.gc6bea4f\n"}]}