{"thread":{"id":"37334","subject":"[PATCH] pack-objects: turn off bitmaps when we see --shallow lines","startedAt":"2014-08-12T04:34:53Z","lastAt":"2014-08-13T05:09:36Z","messageCount":3,"participants":["Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247603","messageId":"20140812043452.GA11784@peff.net","threadId":"37334","inReplyTo":null,"subject":"[PATCH] pack-objects: turn off bitmaps when we see --shallow lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-12T04:34:53Z","receivedAt":"2014-08-12T04:34:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Reachability bitmaps do not work with shallow operations,\nbecause they cache a view of the object reachability that\nrepresents the true objects. Whereas a shallow repository\n(or a shallow operation in a repository) is inherently\ncutting off the object graph with a graft.\n\nWe explicitly disallow the use of bitmaps in shallow\nrepositories by checking is_repository_shallow(), and we\nshould continue to do that. However, we also want to\ndisallow bitmaps when we are serving a fetch to a shallow\nclient, since we momentarily take on their grafted view of\nthe world.\n\nIt used to be enough to call is_repository_shallow at the\nstart of pack-objects.  Upload-pack wrote the other side's\nshallow state to a temporary file and pointed the whole\npack-objects process at this state with \"git --shallow-file\",\nand from the perspective of pack-objects, we really were\nin a shallow repo.  But since b790e0f (upload-pack: send\nshallow info over stdin to pack-objects, 2014-03-11), we do\nit differently: we send --shallow lines to pack-objects over\nstdin, and it registers them itself.\n\nThis means that our is_repository_shallow check is way too\nearly (we have not been told about the shallowness yet), and\nthat it is insufficient (calling is_repository_shallow is\nnot enough, as the shallow grafts we register do not change\nits return value). Instead, we can just turn off bitmaps\nexplicitly when we see these lines.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSorry not to catch this earlier. The bug is in v2.0, and I only noticed\nthe regression because a very small percentage of shallow fetches from\nGitHub started failing after we deployed v2.0 a few weeks ago. It took\nme a while to figure out the reproduction recipe below. :)\n\nArguably is_repository_shallow should return 1 if anybody has registered\na shallow graft, but that wouldn't be enough to fix this (we'd still\nneed to check it again _after_ reading the --shallow lines). So I think\nthis fix is fine here. I don't know if any other parts of the code would\ncare, though.\n\n builtin/pack-objects.c          |  1 +\n t/t5311-pack-bitmaps-shallow.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+)\n create mode 100755 t/t5311-pack-bitmaps-shallow.sh\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 238b502..b59f5d8 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2494,6 +2494,7 @@ static void get_object_list(int ac, const char **av)\n \t\t\t\tif (get_sha1_hex(line + 10, sha1))\n \t\t\t\t\tdie(\"not an SHA-1 '%s'\", line + 10);\n \t\t\t\tregister_shallow(sha1);\n+\t\t\t\tuse_bitmap_index = 0;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tdie(\"not a rev '%s'\", line);\ndiff --git a/t/t5311-pack-bitmaps-shallow.sh b/t/t5311-pack-bitmaps-shallow.sh\nnew file mode 100755\nindex 0000000..872a95d\n--- /dev/null\n+++ b/t/t5311-pack-bitmaps-shallow.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='check bitmap operation with shallow repositories'\n+. ./test-lib.sh\n+\n+# We want to create a situation where the shallow, grafted\n+# view of reachability does not match reality in a way that\n+# might cause us to send insufficient objects.\n+#\n+# We do this with a history that repeats a state, like:\n+#\n+#      A    --   B    --   C\n+#    file=1    file=2    file=1\n+#\n+# and then create a shallow clone to the second commit, B.\n+# In a non-shallow clone, that would mean we already have\n+# the tree for A. But in a shallow one, we've grafted away\n+# A, and fetching A to B requires that the other side send\n+# us the tree for file=1.\n+test_expect_success 'setup shallow repo' '\n+\techo 1 >file &&\n+\tgit add file &&\n+\tgit commit -m orig &&\n+\techo 2 >file &&\n+\tgit commit -a -m update &&\n+\tgit clone --no-local --bare --depth=1 . shallow.git &&\n+\techo 1 >file &&\n+\tgit commit -a -m repeat\n+'\n+\n+test_expect_success 'turn on bitmaps in the parent' '\n+\tgit repack -adb\n+'\n+\n+test_expect_success 'shallow fetch from bitmapped repo' '\n+\t(cd shallow.git && git fetch)\n+'\n+\n+test_done\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"247606","messageId":"CACsJy8AQ4vC4QKR_y62h_Gjd619QfBg-rDB2F6HFj2fvJj41tg@mail.gmail.com","threadId":"37334","inReplyTo":"20140812043452.GA11784@peff.net","subject":"Re: [PATCH] pack-objects: turn off bitmaps when we see --shallow lines","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-12T15:13:03Z","receivedAt":"2014-08-12T15:13:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 12, 2014 at 11:34 AM, Jeff King <peff@peff.net> wrote:\n> Arguably is_repository_shallow should return 1 if anybody has registered\n> a shallow graft, but that wouldn't be enough to fix this (we'd still\n> need to check it again _after_ reading the --shallow lines). So I think\n> this fix is fine here. I don't know if any other parts of the code would\n> care, though.\n\nIt's getting too subtle (is_repository_shallow fails to return 1).\nregister_shallow() is used elsewhere too, luckily pack bitmap's use is\nstill limited in pack-objects (I think).I prefer (in future) to teach\nis_repository_shallow about register_shallow and move it to right\nbefore get_object_list_from_bitmap() is called, and some sort of\nmechanism to say \"hey I'm all set, never change shallow repo status\nagain from now on, or just die if you have to do it\" to protect us\nfrom similar bugs. But for now your fix is good (and simple).\n-- \nDuy\n"},{"id":"247641","messageId":"20140813050935.GA21559@peff.net","threadId":"37334","inReplyTo":"CACsJy8AQ4vC4QKR_y62h_Gjd619QfBg-rDB2F6HFj2fvJj41tg@mail.gmail.com","subject":"Re: [PATCH] pack-objects: turn off bitmaps when we see --shallow lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-13T05:09:36Z","receivedAt":"2014-08-13T05:09:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 12, 2014 at 10:13:03PM +0700, Duy Nguyen wrote:\n\n> On Tue, Aug 12, 2014 at 11:34 AM, Jeff King <peff@peff.net> wrote:\n> > Arguably is_repository_shallow should return 1 if anybody has registered\n> > a shallow graft, but that wouldn't be enough to fix this (we'd still\n> > need to check it again _after_ reading the --shallow lines). So I think\n> > this fix is fine here. I don't know if any other parts of the code would\n> > care, though.\n> \n> It's getting too subtle (is_repository_shallow fails to return 1).\n> register_shallow() is used elsewhere too, luckily pack bitmap's use is\n> still limited in pack-objects (I think).\n\nIt is, though I have some patches in the works to use it in more places.\n\nI was tempted to make a check in prepare_bitmap_walk() to just return -1\n(the same as if there are no bitmaps at all) if any commit grafts are in\nuse. That would also catch new callers. But the graft (and replace)\nrules are not always the same. We should not respect those features when\npacking or pruning (though I think pruning _does_ currently respect\ngrafts, which seems like an accident waiting to happen).\n\nI think this is a good minimal fix for now, but I'll revisit the\nreplace/graft/shallow issues when I add more bitmap users outside of\npack-objects.\n\n> I prefer (in future) to teach is_repository_shallow about\n> register_shallow and move it to right before\n> get_object_list_from_bitmap() is called, and some sort of mechanism to\n> say \"hey I'm all set, never change shallow repo status again from now\n> on, or just die if you have to do it\" to protect us from similar bugs.\n> But for now your fix is good (and simple).\n\nYeah, that sounds like a good direction for the shallow part of it.\n\n-Peff\n"}]}