{"thread":{"id":"44719","subject":"\"disabling bitmap writing, as some objects are not being packed\"?","startedAt":"2016-12-16T21:05:59Z","lastAt":"2017-02-09T07:12:34Z","messageCount":21,"participants":["David Turner","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307918","messageId":"1481922331.28176.11.camel@frank","threadId":"44719","inReplyTo":null,"subject":"\"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-12-16T21:05:31Z","receivedAt":"2016-12-16T21:05:59Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"I'm a bit confused by the message \"disabling bitmap writing, as some\nobjects are not being packed\".  I see it the my gc.log file on my git\nserver.\n\n1. Its presence in the gc.log file prevents future automatic garbage\ncollection.  This seems bad.  I understand the desire to avoid making\nthings worse if a past gc has run into issues.  But this warning is\nnon-fatal; the only consequence is that many operations get slower.  But\na lack of gc when there are too many packs causes that consequence too\n(often a much worse slowdown than would be caused by the missing\nbitmap).\n\nSo I wonder if it would be better for auto gc to grep gc.log for fatal\nerrors (as opposed to warnings) and only skip running if any are found.\nAlternately, we could simply put warnings into gc.log.warning and\nreserve gc.log for fatal errors. I'm not sure which would be simpler.  \n\n2. I don't understand what would cause that message.  That is, what bad\nthing am I doing that I should stop doing?  I've briefly skimmed the\ncode and commit message, but the answer isn't leaping out at me.\n\n\n"},{"id":"307919","messageId":"20161216212731.eac2q4caitlh3rjw@sigill.intra.peff.net","threadId":"44719","inReplyTo":"1481922331.28176.11.camel@frank","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T21:27:31Z","receivedAt":"2016-12-16T21:27:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 04:05:31PM -0500, David Turner wrote:\n\n> 1. Its presence in the gc.log file prevents future automatic garbage\n> collection.  This seems bad.  I understand the desire to avoid making\n> things worse if a past gc has run into issues.  But this warning is\n> non-fatal; the only consequence is that many operations get slower.  But\n> a lack of gc when there are too many packs causes that consequence too\n> (often a much worse slowdown than would be caused by the missing\n> bitmap).\n> \n> So I wonder if it would be better for auto gc to grep gc.log for fatal\n> errors (as opposed to warnings) and only skip running if any are found.\n> Alternately, we could simply put warnings into gc.log.warning and\n> reserve gc.log for fatal errors. I'm not sure which would be simpler.  \n\nWithout thinking too hard on it, that seems like the appropriate\nsolution to me, too.\n\n> 2. I don't understand what would cause that message.  That is, what bad\n> thing am I doing that I should stop doing?  I've briefly skimmed the\n> code and commit message, but the answer isn't leaping out at me.\n\nDo you have alternates and are using --local? Do you have .keep packs\nand have set repack.packKeptObjects to false?\n\nThere are other ways (e.g., an incremental repack), but I think those\nare the likely ones to get via \"git gc\".\n\n-Peff\n"},{"id":"307920","messageId":"xmqqpokrr2cf.fsf@gitster.mtv.corp.google.com","threadId":"44719","inReplyTo":"1481922331.28176.11.camel@frank","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T21:28:00Z","receivedAt":"2016-12-16T21:28:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <novalis@novalis.org> writes:\n\n> I'm a bit confused by the message \"disabling bitmap writing, as some\n> objects are not being packed\".  I see it the my gc.log file on my git\n> server.\n\n> 1. Its presence in the gc.log file prevents future automatic garbage\n> collection.  This seems bad.  I understand the desire to avoid making\n> things worse if a past gc has run into issues.  But this warning is\n> non-fatal; the only consequence is that many operations get slower.  But\n> a lack of gc when there are too many packs causes that consequence too\n> (often a much worse slowdown than would be caused by the missing\n> bitmap).\n>\n> So I wonder if it would be better for auto gc to grep gc.log for fatal\n> errors (as opposed to warnings) and only skip running if any are found.\n> Alternately, we could simply put warnings into gc.log.warning and\n> reserve gc.log for fatal errors. I'm not sure which would be simpler.  \n\nI am not sure if string matching is really a good idea, as I'd\nassume that these messages are eligible for i18n.\n\n329e6e8794 (\"gc: save log from daemonized gc --auto and print it\nnext time\", 2015-09-19) wanted to notice that auto-gc is not\nmaking progress and used the presense of error messages as a cue.\nIn your case, I think the auto-gc _is_ making progress, reducing\nnumber of loose objects in the repository or consolidating many\npackfiles into one, and the message is only about the fact that\npacking is punting and not producing a bitmap as you asked, which\nis different from not making any progress.  I do not think log vs\nwarn is a good criteria to tell them apart, either.\n\nIn any case, as the error message asks the user to do, the user\neventually wants to correct the root cause before removing the\ngc.log; I am not sure report_last_gc_error() is the place to correct\nthis in the first place.\n\n> 2. I don't understand what would cause that message.  That is, what bad\n> thing am I doing that I should stop doing?  I've briefly skimmed the\n> code and commit message, but the answer isn't leaping out at me.\n\nEnabling bitmap generation for incremental packing that does not\ncram everything into a single pack is triggering it, I would\npresume.  Perhaps we should ignore -b option in most of the cases\nand enable it only for \"repack -a -d -f\" codepath?  Or detect that\nwe are being run from \"gc --auto\" and automatically disable -b?  I\nhave a feeling that an approach along that line is closer to the\nreal solution than tweaking report_last_gc_error() and trying to\ndeduce if we are making any progress.\n\n\n"},{"id":"307921","messageId":"20161216213214.z3mzkp2xqnwrqkh2@sigill.intra.peff.net","threadId":"44719","inReplyTo":"xmqqpokrr2cf.fsf@gitster.mtv.corp.google.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T21:32:14Z","receivedAt":"2016-12-16T21:32:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 01:28:00PM -0800, Junio C Hamano wrote:\n\n> > 2. I don't understand what would cause that message.  That is, what bad\n> > thing am I doing that I should stop doing?  I've briefly skimmed the\n> > code and commit message, but the answer isn't leaping out at me.\n> \n> Enabling bitmap generation for incremental packing that does not\n> cram everything into a single pack is triggering it, I would\n> presume.  Perhaps we should ignore -b option in most of the cases\n> and enable it only for \"repack -a -d -f\" codepath?  Or detect that\n> we are being run from \"gc --auto\" and automatically disable -b?  I\n> have a feeling that an approach along that line is closer to the\n> real solution than tweaking report_last_gc_error() and trying to\n> deduce if we are making any progress.\n\nAh, indeed. I was thinking in my other response that \"git gc\" would\nalways kick off an all-into-one repack. But \"gc --auto\" will not in\ncertain cases. And yes, in those cases you definitely would want\n--no-write-bitmap-index. I think it would be reasonable for \"git repack\"\nto disable bitmap-writing automatically when not doing an all-into-one\nrepack.\n\n-Peff\n"},{"id":"307922","messageId":"1481924416.28176.19.camel@frank","threadId":"44719","inReplyTo":"20161216213214.z3mzkp2xqnwrqkh2@sigill.intra.peff.net","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2016-12-16T21:40:16Z","receivedAt":"2016-12-16T21:40:23Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2016-12-16 at 16:32 -0500, Jeff King wrote:\n> On Fri, Dec 16, 2016 at 01:28:00PM -0800, Junio C Hamano wrote:\n> \n> > > 2. I don't understand what would cause that message.  That is, what bad\n> > > thing am I doing that I should stop doing?  I've briefly skimmed the\n> > > code and commit message, but the answer isn't leaping out at me.\n> > \n> > Enabling bitmap generation for incremental packing that does not\n> > cram everything into a single pack is triggering it, I would\n> > presume.  Perhaps we should ignore -b option in most of the cases\n> > and enable it only for \"repack -a -d -f\" codepath?  Or detect that\n> > we are being run from \"gc --auto\" and automatically disable -b?  I\n> > have a feeling that an approach along that line is closer to the\n> > real solution than tweaking report_last_gc_error() and trying to\n> > deduce if we are making any progress.\n> \n> Ah, indeed. I was thinking in my other response that \"git gc\" would\n> always kick off an all-into-one repack. But \"gc --auto\" will not in\n> certain cases. And yes, in those cases you definitely would want\n> --no-write-bitmap-index. I think it would be reasonable for \"git repack\"\n> to disable bitmap-writing automatically when not doing an all-into-one\n> repack.\n\nI do not have alternates and am not using --local.  Nor do I have .keep\npacks.\n\nI would assume, based on the documentation, that auto gc would be doing\nan all-into-one repack:\n\"If the number of packs exceeds the value of gc.autopacklimit, then\n existing packs (except those marked with a .keep file) are\n consolidated into a single pack by using the -A option of git\n repack.\"\n\nI don't have any settings that limit the size of packs, either.  And a\nmanual git repack -a -d creates only a single pack.  Its loneliness\ndoesn't last long, because pretty soon a new pack is created by an\nincoming push.\n\nUnless this just means that some objects are being kept loose (perhaps\nbecause they are unreferenced)? \n\n\n"},{"id":"307926","messageId":"20161216214906.z53yp2x4n6hdc27m@sigill.intra.peff.net","threadId":"44719","inReplyTo":"1481924416.28176.19.camel@frank","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-16T21:49:07Z","receivedAt":"2016-12-16T21:49:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 04:40:16PM -0500, David Turner wrote:\n\n> I would assume, based on the documentation, that auto gc would be doing\n> an all-into-one repack:\n> \"If the number of packs exceeds the value of gc.autopacklimit, then\n>  existing packs (except those marked with a .keep file) are\n>  consolidated into a single pack by using the -A option of git\n>  repack.\"\n> \n> I don't have any settings that limit the size of packs, either.  And a\n> manual git repack -a -d creates only a single pack.  Its loneliness\n> doesn't last long, because pretty soon a new pack is created by an\n> incoming push.\n\nThe interesting code is in need_to_gc():\n\n        /*\n         * If there are too many loose objects, but not too many\n         * packs, we run \"repack -d -l\".  If there are too many packs,\n         * we run \"repack -A -d -l\".  Otherwise we tell the caller\n         * there is no need.\n         */\n        if (too_many_packs())\n                add_repack_all_option();\n        else if (!too_many_loose_objects())\n                return 0;\n\nSo if you have (say) 10 packs and 10,000 objects, we'll incrementally\npack those objects into a single new pack.\n\nI never noticed this myself because we do not use auto-gc at GitHub at\nall. We only ever do a big all-into-one repack.\n\n> Unless this just means that some objects are being kept loose (perhaps\n> because they are unreferenced)? \n\nIf they're unreferenced, they won't be part of the new pack. You might\naccumulate loose objects that are ejected from previous packs, which\ncould trigger auto-gc to do an incremental pack (even though it wouldn't\nbe productive, because they're unreferenced!). You may also get them\nfrom pushes (small pushes will be exploded into loose objects by\ndefault).\n\n-Peff\n"},{"id":"307931","messageId":"1481932775-12952-1-git-send-email-dturner@twosigma.com","threadId":"44719","inReplyTo":"20161216214906.z53yp2x4n6hdc27m@sigill.intra.peff.net","subject":"[PATCH] pack-objects: don't warn about bitmaps on incremental pack","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-12-16T23:59:35Z","receivedAt":"2016-12-16T23:59:49Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When running git pack-objects --incremental, we do not expect to be\nable to write a bitmap; it is very likely that objects in the new pack\nwill have references to objects outside of the pack.  So we don't need\nto warn the user about it.\n\nThis warning was making its way into gc.log because auto-gc will do an\nincremental repack when there are too many loose objects but not too\nmany packs.  When the gc.log was present, future auto gc runs would\nrefuse to run.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n builtin/pack-objects.c  |  3 ++-\n t/t5310-pack-bitmaps.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fd52bd..96de213 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1083,7 +1083,8 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\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\tif (!incremental)\n+\t\t\t\twarning(_(no_closure_warning));\n \t\t\twrite_bitmap_index = 0;\n \t\t}\n \t\treturn 0;\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex b4c7a6f..d81636e 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -247,6 +247,18 @@ test_expect_success 'pack-objects respects --incremental' '\n \ttest_cmp 4.objects objects\n '\n \n+test_expect_success 'incremental repack does not create bitmaps' '\n+\ttest_commit 11 &&\n+\tls .git/objects/pack/ | grep bitmap >existing_bitmaps &&\n+\tls .git/objects/pack/ | grep -v bitmap >existing_packs &&\n+\tgit repack -d 2>err &&\n+\ttest_line_count = 0 err &&\n+\tls .git/objects/pack/ | grep bitmap >output &&\n+\tls .git/objects/pack/ | grep -v bitmap >post_packs &&\n+\ttest_cmp existing_bitmaps output &&\n+\t! test_cmp existing_packs post_packs\n+'\n+\n test_expect_success 'pack with missing blob' '\n \trm $(objpath $blob) &&\n \tgit pack-objects --stdout --revs <revs >/dev/null\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"307948","messageId":"20161217040426.7qeixbihiou5mbsl@sigill.intra.peff.net","threadId":"44719","inReplyTo":"1481932775-12952-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH] pack-objects: don't warn about bitmaps on incremental pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-17T04:04:26Z","receivedAt":"2016-12-17T04:04:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 16, 2016 at 06:59:35PM -0500, David Turner wrote:\n\n> When running git pack-objects --incremental, we do not expect to be\n> able to write a bitmap; it is very likely that objects in the new pack\n> will have references to objects outside of the pack.  So we don't need\n> to warn the user about it.\n> [...]\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 0fd52bd..96de213 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1083,7 +1083,8 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\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\tif (!incremental)\n> +\t\t\t\twarning(_(no_closure_warning));\n>  \t\t\twrite_bitmap_index = 0;\n>  \t\t}\n>  \t\treturn 0;\n\nI agree that the user doesn't need to be warned about it when running\n\"gc --auto\", but I wonder if somebody invoking \"pack-objects\n--incremental --write-bitmap-index\" ought to be.\n\nIn other words, your patch is detecting at a low level that we've been\ngiven a nonsense combination of options, but should we perhaps stop\npassing nonsense in the first place?\n\nEither at the repack level, with something like:\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 80dd06b4a2..6608a902b1 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -231,8 +231,6 @@ 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)\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@@ -256,8 +254,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\targv_array_push(&cmd.args, \"--unpacked\");\n \t\targv_array_push(&cmd.args, \"--incremental\");\n+\t\twrite_bitmap_index = 0;\n \t}\n \n+\tif (write_bitmaps)\n+\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n \tif (local)\n \t\targv_array_push(&cmd.args,  \"--local\");\n \tif (quiet)\n\nThough that still means we do not warn on:\n\n  git repack --write-bitmap-index\n\nwhich is nonsense (it is asking for an incremental repack with bitmaps).\n\nSo maybe do it at the gc level, like:\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 069950d0b4..d3c978c765 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+\targv_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))\n\n-Peff\n"},{"id":"307950","messageId":"CACsJy8ACy+Hv1Z3FgG-WFBozwWqmMuN-JnMWF-+rdpF0knFjqg@mail.gmail.com","threadId":"44719","inReplyTo":"xmqqpokrr2cf.fsf@gitster.mtv.corp.google.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-17T07:50:48Z","receivedAt":"2016-12-17T07:51:26Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Dec 17, 2016 at 4:28 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Turner <novalis@novalis.org> writes:\n>\n>> I'm a bit confused by the message \"disabling bitmap writing, as some\n>> objects are not being packed\".  I see it the my gc.log file on my git\n>> server.\n>\n>> 1. Its presence in the gc.log file prevents future automatic garbage\n>> collection.  This seems bad.  I understand the desire to avoid making\n>> things worse if a past gc has run into issues.  But this warning is\n>> non-fatal; the only consequence is that many operations get slower.  But\n>> a lack of gc when there are too many packs causes that consequence too\n>> (often a much worse slowdown than would be caused by the missing\n>> bitmap).\n>>\n>> So I wonder if it would be better for auto gc to grep gc.log for fatal\n>> errors (as opposed to warnings) and only skip running if any are found.\n>> Alternately, we could simply put warnings into gc.log.warning and\n>> reserve gc.log for fatal errors. I'm not sure which would be simpler.\n>\n> I am not sure if string matching is really a good idea, as I'd\n> assume that these messages are eligible for i18n.\n\nAnd we can't grep for fatal errors anyway. The problem that led to\n329e6e8794 was this line\n\n    warning: There are too many unreachable loose objects; run 'git\nprune' to remove them.\n\nwhich is not fatal.\n\n> 329e6e8794 (\"gc: save log from daemonized gc --auto and print it\n> next time\", 2015-09-19) wanted to notice that auto-gc is not\n> making progress and used the presense of error messages as a cue.\n> In your case, I think the auto-gc _is_ making progress, reducing\n> number of loose objects in the repository or consolidating many\n> packfiles into one\n\nYeah the key point is making progress, and to reliably detect that we\nneed some way for all the commands that git-gc executes to tell it\nabout that, git-repack in this particular case but...\n\n> and the message is only about the fact that\n> packing is punting and not producing a bitmap as you asked, which\n> is different from not making any progress.  I do not think log vs\n> warn is a good criteria to tell them apart, either.\n>\n> In any case, as the error message asks the user to do, the user\n> eventually wants to correct the root cause before removing the\n> gc.log; I am not sure report_last_gc_error() is the place to correct\n> this in the first place.\n>\n>> 2. I don't understand what would cause that message.  That is, what bad\n>> thing am I doing that I should stop doing?  I've briefly skimmed the\n>> code and commit message, but the answer isn't leaping out at me.\n>\n> Enabling bitmap generation for incremental packing that does not\n> cram everything into a single pack is triggering it, I would\n> presume.  Perhaps we should ignore -b option in most of the cases\n> and enable it only for \"repack -a -d -f\" codepath?  Or detect that\n> we are being run from \"gc --auto\" and automatically disable -b?\n\n... since we have to change down in git-repack for that, perhaps doing\nthis is better. We can pass --auto (or something) to repack to tell it\nabout this special caller, so it only prints something to stderr in\nserious cases.\n\nOr we detect cases where background gc'ing won't work well and always\ndo it in foreground (e.g. when bitmap generation is enabled).\n\n> I have a feeling that an approach along that line is closer to the\n> real solution than tweaking report_last_gc_error() and trying to\n> deduce if we are making any progress.\n-- \nDuy\n"},{"id":"308038","messageId":"84da83e80fff40e3b7de43d2a11d440d@exmbdft7.ad.twosigma.com","threadId":"44719","inReplyTo":"20161217040426.7qeixbihiou5mbsl@sigill.intra.peff.net","subject":"RE: [PATCH] pack-objects: don't warn about bitmaps on incremental pack","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-12-19T16:03:09Z","receivedAt":"2016-12-19T16:04:12Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> diff --git a/builtin/gc.c b/builtin/gc.c index 069950d0b4..d3c978c765\n> 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> +\targv_array_push(&repack, \"--no-write-bitmap-index\"); }\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))\n\nSure, that's fine.\n"},{"id":"311021","messageId":"1486515795.1938.45.camel@novalis.org","threadId":"44719","inReplyTo":"CACsJy8ACy+Hv1Z3FgG-WFBozwWqmMuN-JnMWF-+rdpF0knFjqg@mail.gmail.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2017-02-08T01:03:15Z","receivedAt":"2017-02-08T01:03:18Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:\n> And we can't grep for fatal errors anyway. The problem that led to\n> 329e6e8794 was this line\n> \n>     warning: There are too many unreachable loose objects; run 'git\n> prune' to remove them.\n> \n> which is not fatal.\n\nSo, speaking of that message, I noticed that our git servers were\ngetting slow again and found that message in gc.log.\n\nI propose to make auto gc not write that message either. Any objections?\n\n\n"},{"id":"311042","messageId":"CACsJy8C81+D=UG4pZ4e+URQqKRCPG=5bLiCHbGCQamvE-2y2MQ@mail.gmail.com","threadId":"44719","inReplyTo":"1486515795.1938.45.camel@novalis.org","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-08T06:45:42Z","receivedAt":"2017-02-08T06:47:46Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:\n> On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:\n>> And we can't grep for fatal errors anyway. The problem that led to\n>> 329e6e8794 was this line\n>>\n>>     warning: There are too many unreachable loose objects; run 'git\n>> prune' to remove them.\n>>\n>> which is not fatal.\n>\n> So, speaking of that message, I noticed that our git servers were\n> getting slow again and found that message in gc.log.\n>\n> I propose to make auto gc not write that message either. Any objections?\n\nDoes that really help? auto gc would run more often, but unreachable\nloose objects are still present and potentially make your servers\nslow? Should these servers run periodic and explicit gc/prune?\n-- \nDuy\n"},{"id":"311044","messageId":"1486542299.1938.47.camel@novalis.org","threadId":"44719","inReplyTo":"CACsJy8C81+D=UG4pZ4e+URQqKRCPG=5bLiCHbGCQamvE-2y2MQ@mail.gmail.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2017-02-08T08:24:59Z","receivedAt":"2017-02-08T08:25:01Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:\n> On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:\n> > On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:\n> >> And we can't grep for fatal errors anyway. The problem that led to\n> >> 329e6e8794 was this line\n> >>\n> >>     warning: There are too many unreachable loose objects; run 'git\n> >> prune' to remove them.\n> >>\n> >> which is not fatal.\n> >\n> > So, speaking of that message, I noticed that our git servers were\n> > getting slow again and found that message in gc.log.\n> >\n> > I propose to make auto gc not write that message either. Any objections?\n> \n> Does that really help? auto gc would run more often, but unreachable\n> loose objects are still present and potentially make your servers\n> slow? Should these servers run periodic and explicit gc/prune?\n\nAt least pack files wouldn't accumulate.  This is the major cause of\nslowdown, since each pack file must be checked for each object.\n\n(And, also, maybe those unreachable loose objects are too new to get\ngc'd, but if we retry next week, we'll gc them).\n\n\n"},{"id":"311045","messageId":"CACsJy8C4DO-GYREUhED3YU_WetoTZaB3MUq1kGfRjA3e-FOLYQ@mail.gmail.com","threadId":"44719","inReplyTo":"1486542299.1938.47.camel@novalis.org","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-08T08:37:24Z","receivedAt":"2017-02-08T08:38:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 8, 2017 at 3:24 PM, David Turner <novalis@novalis.org> wrote:\n> On Wed, 2017-02-08 at 13:45 +0700, Duy Nguyen wrote:\n>> On Wed, Feb 8, 2017 at 8:03 AM, David Turner <novalis@novalis.org> wrote:\n>> > On Sat, 2016-12-17 at 14:50 +0700, Duy Nguyen wrote:\n>> >> And we can't grep for fatal errors anyway. The problem that led to\n>> >> 329e6e8794 was this line\n>> >>\n>> >>     warning: There are too many unreachable loose objects; run 'git\n>> >> prune' to remove them.\n>> >>\n>> >> which is not fatal.\n>> >\n>> > So, speaking of that message, I noticed that our git servers were\n>> > getting slow again and found that message in gc.log.\n>> >\n>> > I propose to make auto gc not write that message either. Any objections?\n>>\n>> Does that really help? auto gc would run more often, but unreachable\n>> loose objects are still present and potentially make your servers\n>> slow? Should these servers run periodic and explicit gc/prune?\n>\n> At least pack files wouldn't accumulate.  This is the major cause of\n> slowdown, since each pack file must be checked for each object.\n>\n> (And, also, maybe those unreachable loose objects are too new to get\n> gc'd, but if we retry next week, we'll gc them).\n\nI was about to suggest a config option that lets you run auto gc\nunconditionally, which, I think, is better than suppressing the\nmessage. Then I found gc.autoDetach. If you set it to false globally,\nI think you'll get the behavior you want.\n\nOn second thought, perhaps gc.autoDetach should default to false if\nthere's no tty, since its main point it to stop breaking interactive\nusage. That would make the server side happy (no tty there).\n-- \nDuy\n"},{"id":"311072","messageId":"xmqqtw84wpag.fsf@gitster.mtv.corp.google.com","threadId":"44719","inReplyTo":"CACsJy8C4DO-GYREUhED3YU_WetoTZaB3MUq1kGfRjA3e-FOLYQ@mail.gmail.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-08T17:44:23Z","receivedAt":"2017-02-08T17:44:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On second thought, perhaps gc.autoDetach should default to false if\n> there's no tty, since its main point it to stop breaking interactive\n> usage. That would make the server side happy (no tty there).\n\nSounds like an idea, but wouldn't that keep the end-user coming over\nthe network waiting after accepting a push until the GC completes, I\nwonder.  If an impatient user disconnects, would that end up killing\nan ongoing GC?  etc.\n\n"},{"id":"311078","messageId":"20170208190858.rjoqehbhyizlwg5q@sigill.intra.peff.net","threadId":"44719","inReplyTo":"1486580742.1938.52.camel@novalis.org","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-08T19:08:58Z","receivedAt":"2017-02-08T19:12:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2017 at 02:05:42PM -0500, David Turner wrote:\n\n> On Wed, 2017-02-08 at 09:44 -0800, Junio C Hamano wrote:\n> > Duy Nguyen <pclouds@gmail.com> writes:\n> > \n> > > On second thought, perhaps gc.autoDetach should default to false if\n> > > there's no tty, since its main point it to stop breaking interactive\n> > > usage. That would make the server side happy (no tty there).\n> > \n> > Sounds like an idea, but wouldn't that keep the end-user coming over\n> > the network waiting after accepting a push until the GC completes, I\n> > wonder.  If an impatient user disconnects, would that end up killing\n> > an ongoing GC?  etc.\n> \n> Regardless, it's impolite to keep the user waiting. So, I think we\n> should just not write the \"too many unreachable loose objects\" message\n> if auto-gc is on.  Does that sound OK?\n\nI thought the point of that message was to prevent auto-gc from kicking\nin over and over again due to objects that won't actually get pruned.\n\nI wonder if you'd want to either bump the auto-gc object limit, or\npossibly reduce the gc.pruneExpire limit to keep this situation from\ncoming up in the first place (or at least mitigating the amount of time\nit's the case).\n\n-Peff\n"},{"id":"311108","messageId":"1486592043.1938.82.camel@novalis.org","threadId":"44719","inReplyTo":"20170208190858.rjoqehbhyizlwg5q@sigill.intra.peff.net","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2017-02-08T22:14:03Z","receivedAt":"2017-02-08T22:14:05Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2017-02-08 at 14:08 -0500, Jeff King wrote:\n> On Wed, Feb 08, 2017 at 02:05:42PM -0500, David Turner wrote:\n> \n> > On Wed, 2017-02-08 at 09:44 -0800, Junio C Hamano wrote:\n> > > Duy Nguyen <pclouds@gmail.com> writes:\n> > > \n> > > > On second thought, perhaps gc.autoDetach should default to false if\n> > > > there's no tty, since its main point it to stop breaking interactive\n> > > > usage. That would make the server side happy (no tty there).\n> > > \n> > > Sounds like an idea, but wouldn't that keep the end-user coming over\n> > > the network waiting after accepting a push until the GC completes, I\n> > > wonder.  If an impatient user disconnects, would that end up killing\n> > > an ongoing GC?  etc.\n> > \n> > Regardless, it's impolite to keep the user waiting. So, I think we\n> > should just not write the \"too many unreachable loose objects\" message\n> > if auto-gc is on.  Does that sound OK?\n> \n> I thought the point of that message was to prevent auto-gc from kicking\n> in over and over again due to objects that won't actually get pruned.\n> \n> I wonder if you'd want to either bump the auto-gc object limit, or\n> possibly reduce the gc.pruneExpire limit to keep this situation from\n> coming up in the first place (or at least mitigating the amount of time\n> it's the case).\n\nAuto-gc might not succeed in pruning objects, but it will at least\nreduce the number of packs, which is super-important for performance.\n\nI think the intent of automatic gc is to have a git repository be\nrelatively low-maintenance from a server-operator perspective.  (Side\nnote: it's fairly trivial for a user with push access to mess with the\ncheck simply by pushing a bunch of objects whose shas start with 17).\nIt seems odd that git gets itself into a state where it refuses to do\nany maintenance just because at some point some piece of the maintenance\ndidn't make progress.\n\nSure, I could change my configuration, but that doesn't help the other\nfolks (e.g. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813084 )\nwho run into this.\n\nI have three thoughts on this:\n\nIdea 1: when gc --auto would issue this message, instead it could create\na file named gc.too-much-garbage (instead of gc.log), with this message.\nIf that file exists, and it is less than one day (?) old, then we don't\nattempt to do a full gc; instead we just run git repack -A -d.  (If it's\nmore than one day old, we just delete it and continue anyway).\n\nIdea 2 : Like idea 1, but instead of repacking, just smash the existing\npacks together into one big pack.  In other words, don't consider\ndangling objects, or recompute deltas.  Twitter has a tool called \"git\ncombine-pack\" that does this:\nhttps://github.com/dturner-tw/git/blob/dturner/journal/builtin/combine-pack.c\n\nThat's less space-efficient than a true repack, but it's no worse than\nhaving the packs separate, and it's a win for read performance because\nthere's no need to do a linear search over N packs to find an object.\n\nIdea 3: As I suggested last time, separate fatal and non-fatal errors.\nIf gc fails because of EIO or something, we probably don't want to touch\nthe disk anymore. But here, the worst consequence is that we waste some\nprocessing power. And it's better to occasionally waste processing power\nin a non-interactive setting than it is to do so when a user will be\nblocked on it.  So non-fatal warnings should go to gc.log, and fatal\nerrors should go to gc.fatal.  gc.log won't block gc from running. I\nthink this is my preferred option.\n\n"},{"id":"311115","messageId":"20170208230057.hking37uuynf4cgd@sigill.intra.peff.net","threadId":"44719","inReplyTo":"1486592043.1938.82.camel@novalis.org","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-08T23:00:57Z","receivedAt":"2017-02-08T23:01:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2017 at 05:14:03PM -0500, David Turner wrote:\n\n> > I wonder if you'd want to either bump the auto-gc object limit, or\n> > possibly reduce the gc.pruneExpire limit to keep this situation from\n> > coming up in the first place (or at least mitigating the amount of time\n> > it's the case).\n> \n> Auto-gc might not succeed in pruning objects, but it will at least\n> reduce the number of packs, which is super-important for performance.\n\nRight, I mean to bump the loose-object limit but keep the\ngc.autoPackLimit at 50. If you couple that with setting\ntransfer.unpackLimit, then each push creates a single pack, and you\nrepack after 50 pushes.\n\nYou don't have to care about loose objects, because you know you only\nget them when a \"gc\" ejects loose objects (so they're not as efficient,\nbut nothing actually accesses them; they just hang around until their\nmtime grace period is up).\n\n> I think the intent of automatic gc is to have a git repository be\n> relatively low-maintenance from a server-operator perspective.  (Side\n> note: it's fairly trivial for a user with push access to mess with the\n> check simply by pushing a bunch of objects whose shas start with 17).\n> It seems odd that git gets itself into a state where it refuses to do\n> any maintenance just because at some point some piece of the maintenance\n> didn't make progress.\n\nIn my experience, auto-gc has never been a low-maintenance operation on\nthe server side (and I do think it was primarily designed with clients\nin mind).\n\nAt GitHub we disable it entirely, and do our own gc based on a throttled\njob queue (one reason to throttle is that repacking is memory and I/O\nintensive, so you really don't want to a bunch of repacks kicking off\nall at once). So basically any repo that gets pushed to goes on the\nqueue, and then we pick the worst cases from the queue based on how\nbadly they need packing[1].\n\nI wish regular Git were more turn-key in that respect. Maybe it is for\nsmaller sites, but we certainly didn't find it so. And I don't know that\nit's feasible to really share the solution. It's entangled with our\ndatabase (to store last-pushed and last-maintenance values for repos)\nand our job scheduler.\n\n[1] The \"how bad\" thing is a heuristic, and we found it's generally\n    proportional to the number of bytes stored in objects _outside_ of\n    the big \"main\" pack. So 2 big pushes may need maintenance more\n    than 10 tiny pushes, because they have more objects (and our goal\n    with maintenance isn't just saving disk space or avoiding the linear\n    pack search, but having up-to-date bitmaps and good on-disk deltas\n    to make serving fetches as cheap as possible).\n\n> Sure, I could change my configuration, but that doesn't help the other\n> folks (e.g. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813084 )\n> who run into this.\n\nYeah, I'm certainly open to improving Git's defaults. If it's not clear\nfrom the above, I mostly just gave up for a site the size of GitHub. :)\n\n> Idea 1: when gc --auto would issue this message, instead it could create\n> a file named gc.too-much-garbage (instead of gc.log), with this message.\n> If that file exists, and it is less than one day (?) old, then we don't\n> attempt to do a full gc; instead we just run git repack -A -d.  (If it's\n> more than one day old, we just delete it and continue anyway).\n\nI kind of wonder if this should apply to _any_ error. I.e., just check\nthe mtime of gc.log and forcibly remove it when it's older than a day.\nYou never want to get into a state that will fail to resolve itself\neventually. That might still happen (e.g., corrupt repo), but at the\nvery least it won't be because Git is too dumb to try again.\n\n> Idea 2 : Like idea 1, but instead of repacking, just smash the existing\n> packs together into one big pack.  In other words, don't consider\n> dangling objects, or recompute deltas.  Twitter has a tool called \"git\n> combine-pack\" that does this:\n> https://github.com/dturner-tw/git/blob/dturner/journal/builtin/combine-pack.c\n\nWe wrote something similar at GitHub, too, but we never ended up using\nit in production. We found that with a sane scheduler, it's not too big\na deal to just do maintenance once in a while.\n\n  Also, our original problem was that repos which have gotten out of\n  hand (say, 5000 packs) repacked _very_ slowly with a normal repack. So\n  a \"fast pack\" followed by a real pack was a viable way out of that. In\n  the end, I just made pack-objects handle this case better, and we\n  don't need the fast-pack.\n\n> That's less space-efficient than a true repack, but it's no worse than\n> having the packs separate, and it's a win for read performance because\n> there's no need to do a linear search over N packs to find an object.\n\nOver the long term you may end up with worse packs, because the true\nrepack will drop some delta opportunities between objects in the same\npack (reasoning that they weren't made into deltas last time, so it's\nnot worth trying again). You'd probably need to use \"-f\" periodically.\n\nThis is all speculation, though. We never did it in production, so I was\nnever able to measure the real impact over time.\n\n> Idea 3: As I suggested last time, separate fatal and non-fatal errors.\n> If gc fails because of EIO or something, we probably don't want to touch\n> the disk anymore. But here, the worst consequence is that we waste some\n> processing power. And it's better to occasionally waste processing power\n> in a non-interactive setting than it is to do so when a user will be\n> blocked on it.  So non-fatal warnings should go to gc.log, and fatal\n> errors should go to gc.fatal.  gc.log won't block gc from running. I\n> think this is my preferred option.\n\nThis seems like your (1), except that it handles more than one type of\nnon-fatal error. So I like it much better.\n\nI'm still not sure if it's worth making the fatal/non-fatal distinction.\nDoing so is perhaps safer, but it does mean that somebody has to decide\nwhich errors are important enough to block a retry totally, and which\nare not. In theory, it would be safe to always _try_ and then the gc\nprocess can decide when something is broken and abort. And all you've\nwasted is some processing power each day.\n\n-Peff\n"},{"id":"311129","messageId":"xmqqbmuctdwu.fsf@gitster.mtv.corp.google.com","threadId":"44719","inReplyTo":"20170208230057.hking37uuynf4cgd@sigill.intra.peff.net","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T00:18:25Z","receivedAt":"2017-02-09T00:18:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In my experience, auto-gc has never been a low-maintenance operation on\n> the server side (and I do think it was primarily designed with clients\n> in mind).\n\nI do not think auto-gc was ever tweaked to help server usage, in its\nhistory since it was invented strictly to help end-users (mostly new\nones).\n\n> At GitHub we disable it entirely, and do our own gc based on a throttled\n> job queue ...\n> I wish regular Git were more turn-key in that respect. Maybe it is for\n> smaller sites, but we certainly didn't find it so. And I don't know that\n> it's feasible to really share the solution. It's entangled with our\n> database (to store last-pushed and last-maintenance values for repos)\n> and our job scheduler.\n\nThanks for sharing the insights from the trenches ;-)\n\n> Yeah, I'm certainly open to improving Git's defaults. If it's not clear\n> from the above, I mostly just gave up for a site the size of GitHub. :)\n>\n>> Idea 1: when gc --auto would issue this message, instead it could create\n>> a file named gc.too-much-garbage (instead of gc.log), with this message.\n>> If that file exists, and it is less than one day (?) old, then we don't\n>> attempt to do a full gc; instead we just run git repack -A -d.  (If it's\n>> more than one day old, we just delete it and continue anyway).\n>\n> I kind of wonder if this should apply to _any_ error. I.e., just check\n> the mtime of gc.log and forcibly remove it when it's older than a day.\n> You never want to get into a state that will fail to resolve itself\n> eventually. That might still happen (e.g., corrupt repo), but at the\n> very least it won't be because Git is too dumb to try again.\n\n;-)\n\n>> Idea 2 : Like idea 1, but instead of repacking, just smash the existing\n>> packs together into one big pack.  In other words, don't consider\n>> dangling objects, or recompute deltas.  Twitter has a tool called \"git\n>> combine-pack\" that does this:\n>> https://github.com/dturner-tw/git/blob/dturner/journal/builtin/combine-pack.c\n>\n> We wrote something similar at GitHub, too, but we never ended up using\n> it in production. We found that with a sane scheduler, it's not too big\n> a deal to just do maintenance once in a while.\n\nThanks again for this.  I've also been wondering about how effective\na \"concatenate packs without paying reachability penalty\" would be.\n\n> I'm still not sure if it's worth making the fatal/non-fatal distinction.\n> Doing so is perhaps safer, but it does mean that somebody has to decide\n> which errors are important enough to block a retry totally, and which\n> are not. In theory, it would be safe to always _try_ and then the gc\n> process can decide when something is broken and abort. And all you've\n> wasted is some processing power each day.\n\nYup, and somebody or something need to monitor so that repeated\nfailures can be dealt with.\n"},{"id":"311134","messageId":"20170209011241.vfiup56gwrvlxm2k@sigill.intra.peff.net","threadId":"44719","inReplyTo":"xmqqbmuctdwu.fsf@gitster.mtv.corp.google.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-09T01:12:41Z","receivedAt":"2017-02-09T01:20:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2017 at 04:18:25PM -0800, Junio C Hamano wrote:\n\n> > We wrote something similar at GitHub, too, but we never ended up using\n> > it in production. We found that with a sane scheduler, it's not too big\n> > a deal to just do maintenance once in a while.\n> \n> Thanks again for this.  I've also been wondering about how effective\n> a \"concatenate packs without paying reachability penalty\" would be.\n\nFor the sake of posterity, I'll include our patch at the end (sorry, not\nchunked into nice readable commits; that never existed in the first\nplace).\n\n> > I'm still not sure if it's worth making the fatal/non-fatal distinction.\n> > Doing so is perhaps safer, but it does mean that somebody has to decide\n> > which errors are important enough to block a retry totally, and which\n> > are not. In theory, it would be safe to always _try_ and then the gc\n> > process can decide when something is broken and abort. And all you've\n> > wasted is some processing power each day.\n> \n> Yup, and somebody or something need to monitor so that repeated\n> failures can be dealt with.\n\nYes. I think that part is probably outside the scope of Git. But if\nauto-gc leaves gc.log lying around, it would be easy to visit each repo\nand collect the various failures.\n\n-- >8 --\nThis is the \"pack-fast\" patch, for reference. It applies on v2.6.5,\nthough I had to do some wiggling due to a few of our other custom\npatches, so it's possible I introduced new bugs. It compiles, but I\ndidn't actually re-test the result.  I _think_ the original at least\ngenerated valid packs in all cases.\n\nSo I would certainly not recommend anybody run this. It's just a\npossible base to work off of if anybody's interested in the topic. I\nhaven't looked at David's combine-packs at all to see if it is any less\ngross. :)\n\n---\n Makefile            |   1 +\n builtin.h           |   1 +\n builtin/pack-fast.c | 618 +++++++++++++++++++++++++++++++++++\n cache.h             |   5 +\n git.c               |   1 +\n pack-bitmap-write.c | 167 +++++++++-\n pack-bitmap.c       |   2 +-\n pack-bitmap.h       |   8 +\n sha1_file.c         |   4 +-\n 9 files changed, 792 insertions(+), 15 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 37e2d9e18..524b185ec 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -887,6 +887,7 @@ BUILTIN_OBJS += builtin/mv.o\n BUILTIN_OBJS += builtin/name-rev.o\n BUILTIN_OBJS += builtin/notes.o\n BUILTIN_OBJS += builtin/pack-objects.o\n+BUILTIN_OBJS += builtin/pack-fast.o\n BUILTIN_OBJS += builtin/pack-redundant.o\n BUILTIN_OBJS += builtin/pack-refs.o\n BUILTIN_OBJS += builtin/patch-id.o\ndiff --git a/builtin.h b/builtin.h\nindex 79aaf0afe..df4e4d668 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -95,6 +95,7 @@ extern int cmd_mv(int argc, const char **argv, const char *prefix);\n extern int cmd_name_rev(int argc, const char **argv, const char *prefix);\n extern int cmd_notes(int argc, const char **argv, const char *prefix);\n extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);\n+extern int cmd_pack_fast(int argc, const char **argv, const char *prefix);\n extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);\n extern int cmd_patch_id(int argc, const char **argv, const char *prefix);\n extern int cmd_prune(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/pack-fast.c b/builtin/pack-fast.c\nnew file mode 100644\nindex 000000000..ad9f5e5f1\n--- /dev/null\n+++ b/builtin/pack-fast.c\n@@ -0,0 +1,618 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"pack.h\"\n+#include \"progress.h\"\n+#include \"csum-file.h\"\n+#include \"sha1-lookup.h\"\n+#include \"parse-options.h\"\n+#include \"tempfile.h\"\n+#include \"pack-bitmap.h\"\n+#include \"pack-revindex.h\"\n+\n+static const char *pack_usage[] = {\n+\tN_(\"git pack-fast --quiet [options...] [base-name]\"),\n+\tNULL\n+};\n+\n+struct packwriter {\n+\tstruct tempfile *tmp;\n+\toff_t total;\n+\tint fd;\n+\tuint32_t crc32;\n+\tunsigned do_crc;\n+};\n+\n+static void packwriter_crc32_start(struct packwriter *w)\n+{\n+\tw->crc32 = crc32(0, NULL, 0);\n+\tw->do_crc = 1;\n+}\n+\n+static uint32_t packwriter_crc32_end(struct packwriter *w)\n+{\n+\tw->do_crc = 0;\n+\treturn w->crc32;\n+}\n+\n+static void packwriter_write(struct packwriter *w, const void *buf, unsigned int count)\n+{\n+\tif (w->do_crc)\n+\t\tw->crc32 = crc32(w->crc32, buf, count);\n+\twrite_or_die(w->fd, buf, count);\n+\tw->total += count;\n+}\n+\n+static off_t packwriter_total(struct packwriter *w)\n+{\n+\treturn w->total;\n+}\n+\n+static void packwriter_init(struct packwriter *w)\n+{\n+\tchar tmpname[PATH_MAX];\n+\n+\tw->fd = odb_mkstemp(tmpname, sizeof(tmpname), \"pack/tmp_pack_XXXXXX\");\n+\tw->total = 0;\n+\tw->do_crc = 0;\n+\tw->tmp = xcalloc(1, sizeof(*w->tmp));\n+\n+\tregister_tempfile(w->tmp, tmpname);\n+}\n+\n+\n+static int progress = 1;\n+static struct progress *progress_state;\n+static struct pack_idx_option pack_idx_opts;\n+static const char *base_name = \"pack-fast\";\n+static int skip_largest;\n+static int write_bitmap_index = 1;\n+\n+static struct packed_git **all_packfiles;\n+static unsigned int all_packfiles_nr;\n+\n+static struct pack_idx_entry **written_list;\n+static unsigned int written_nr;\n+\n+struct write_slab {\n+\tstruct write_slab *next;\n+\tunsigned int nr;\n+\n+\tstruct write_slab_entry {\n+\t\tstruct pack_idx_entry idx;\n+\t\tenum object_type real_type;\n+\t} entries[];\n+};\n+\n+static struct write_slab *written_slab_root;\n+static struct write_slab *written_slab_current;\n+\n+static void add_to_write_list(\n+\tconst unsigned char *sha1, off_t offset, uint32_t crc32,\n+\tenum object_type real_type)\n+{\n+\tstruct write_slab *slab = written_slab_current;\n+\tstruct write_slab_entry *entry = &(slab->entries[slab->nr++]);\n+\n+\tentry->real_type = real_type;\n+\tentry->idx.offset = offset;\n+\tentry->idx.crc32 = crc32;\n+\thashcpy(entry->idx.sha1, sha1);\n+}\n+\n+static void preallocate_write_slab(unsigned int num_entries)\n+{\n+\tstruct write_slab *slab = xmalloc(\n+\t\tsizeof(struct write_slab) +\n+\t\tnum_entries * sizeof(struct write_slab_entry));\n+\n+\tslab->next = NULL;\n+\tslab->nr = 0;\n+\n+\tif (!written_slab_current) {\n+\t\twritten_slab_current = slab;\n+\t\twritten_slab_root = slab;\n+\t} else {\n+\t\twritten_slab_current->next = slab;\n+\t\twritten_slab_current = slab;\n+\t}\n+}\n+\n+static struct skipped_object {\n+\toff_t skipped_offset;\n+\toff_t real_offset;\n+} *skipped_list;\n+static unsigned int skipped_nr;\n+static unsigned int skipped_alloc;\n+\n+static void add_to_skipped_list(off_t skipped_offset, off_t real_offset)\n+{\n+\tif (skipped_nr >= skipped_alloc) {\n+\t\tskipped_alloc = (skipped_alloc + 32) * 2;\n+\t\tREALLOC_ARRAY(skipped_list, skipped_alloc);\n+\t}\n+\n+\tskipped_list[skipped_nr].skipped_offset = skipped_offset;\n+\tskipped_list[skipped_nr].real_offset = real_offset;\n+\tskipped_nr++;\n+}\n+\n+static off_t find_real_offset_for_base(off_t skipped_offset)\n+{\n+\tint lo = 0, hi = skipped_nr;\n+\twhile (lo < hi) {\n+\t\tint mi = lo + ((hi - lo) / 2);\n+\t\tif (skipped_offset == skipped_list[mi].skipped_offset)\n+\t\t\treturn skipped_list[mi].real_offset;\n+\t\tif (skipped_offset < skipped_list[mi].skipped_offset)\n+\t\t\thi = mi;\n+\t\telse\n+\t\t\tlo = mi + 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+/*\n+ * Record the offsets needed in our reused packfile chunks due to\n+ * \"gaps\" where we omitted some objects.\n+ */\n+static struct reused_chunk {\n+\toff_t start;\n+\toff_t offset;\n+} *reused_chunks;\n+static int reused_chunks_nr;\n+static int reused_chunks_alloc;\n+\n+static void record_reused_object(off_t where, off_t offset)\n+{\n+\tif (reused_chunks_nr && reused_chunks[reused_chunks_nr-1].offset == offset)\n+\t\treturn;\n+\n+\tALLOC_GROW(reused_chunks, reused_chunks_nr + 1,\n+\t\t   reused_chunks_alloc);\n+\treused_chunks[reused_chunks_nr].start = where;\n+\treused_chunks[reused_chunks_nr].offset = offset;\n+\treused_chunks_nr++;\n+}\n+\n+/*\n+ * Binary search to find the chunk that \"where\" is in. Note\n+ * that we're not looking for an exact match, just the first\n+ * chunk that contains it (which implicitly ends at the start\n+ * of the next chunk.\n+ */\n+static off_t find_reused_offset(off_t where)\n+{\n+\tint lo = 0, hi = reused_chunks_nr;\n+\twhile (lo < hi) {\n+\t\tint mi = lo + ((hi - lo) / 2);\n+\t\tif (where == reused_chunks[mi].start)\n+\t\t\treturn reused_chunks[mi].offset;\n+\t\tif (where < reused_chunks[mi].start)\n+\t\t\thi = mi;\n+\t\telse\n+\t\t\tlo = mi + 1;\n+\t}\n+\n+\t/*\n+\t * The first chunk starts at zero, so we can't have gone below\n+\t * there.\n+\t */\n+\tassert(lo);\n+\treturn reused_chunks[lo-1].offset;\n+}\n+\n+static uint32_t nth_packed_object_crc32(const struct packed_git *p, uint32_t nr)\n+{\n+\tconst uint32_t *index_crc = p->index_data;\n+\tindex_crc += 2 + 256 + p->num_objects * (20/4) + nr;\n+\treturn ntohl(*index_crc);\n+}\n+\n+static void load_index_or_die(struct packed_git *p)\n+{\n+\tif (open_pack_index(p) < 0)\n+\t\tdie(\"failed to open index for '%s'\", p->pack_name);\n+\n+\tif (p->index_version != 2)\n+\t\tdie(\"unsupported index version %d (fast-pack requires index v2)\\n\",\n+\t\t\tp->index_version);\n+}\n+\n+static int sort_pack(const void *a_, const void *b_)\n+{\n+\tstruct packed_git *a = *((struct packed_git **)a_);\n+\tstruct packed_git *b = *((struct packed_git **)b_);\n+\n+\tif (a->mtime > b->mtime)\n+\t\treturn 1;\n+\telse if (a->mtime == b->mtime)\n+\t\treturn 0;\n+\treturn -1;\n+}\n+\n+static void find_packfiles(void)\n+{\n+\tstruct packed_git *p;\n+\tunsigned int n;\n+\n+\tprepare_packed_git();\n+\n+\tfor (n = 0, p = packed_git; p; p = p->next) {\n+\t\tif (p->pack_local)\n+\t\t\tn++;\n+\t}\n+\n+\tall_packfiles = xcalloc(n, sizeof(struct packed_git *));\n+\tall_packfiles_nr = n;\n+\n+\tfor (n = 0, p = packed_git; p; p = p->next) {\n+\t\tif (p->pack_local)\n+\t\t\tall_packfiles[n++] = p;\n+\t}\n+\n+\tfor (n = 1; n < all_packfiles_nr; ++n) {\n+\t\tif (all_packfiles[n]->pack_size > all_packfiles[0]->pack_size) {\n+\t\t\tstruct packed_git *tmp = all_packfiles[0];\n+\t\t\tall_packfiles[0] = all_packfiles[n];\n+\t\t\tall_packfiles[n] = tmp;\n+\t\t}\n+\t}\n+\n+\tqsort(all_packfiles + 1, all_packfiles_nr - 1, sizeof(struct packed_git *), sort_pack);\n+}\n+\n+static int sha1_index__cmp(const void *a_, const void *b_)\n+{\n+\tstruct pack_idx_entry *a = *((struct pack_idx_entry **)a_);\n+\tstruct pack_idx_entry *b = *((struct pack_idx_entry **)b_);\n+\treturn hashcmp(a->sha1, b->sha1);\n+}\n+\n+static const unsigned char *sha1_index__access(size_t pos, void *table)\n+{\n+\tstruct pack_idx_entry **index = table;\n+\treturn index[pos]->sha1;\n+}\n+\n+static void sha1_index_update(void)\n+{\n+\tconst unsigned int left_nr = written_nr;\n+\tconst unsigned int right_nr = written_slab_current->nr;\n+\tconst unsigned int total_nr = left_nr + right_nr;\n+\n+\tstruct pack_idx_entry **left = written_list;\n+\tstruct pack_idx_entry **right = xmalloc(right_nr * sizeof(struct pack_idx_entry *));\n+\tstruct pack_idx_entry **result = xmalloc(total_nr * sizeof(struct pack_idx_entry *));\n+\n+\tunsigned int i, j, n;\n+\n+\tfor (j = 0; j < right_nr; ++j)\n+\t\tright[j] = (struct pack_idx_entry *)(&written_slab_current->entries[j]);\n+\n+\tqsort(right, right_nr, sizeof(struct pack_idx_entry  *), sha1_index__cmp);\n+\n+\tfor (i = j = n = 0; i < left_nr && j < right_nr; ++n) {\n+\t\tstruct pack_idx_entry *a = left[i];\n+\t\tstruct pack_idx_entry *b = right[j];\n+\n+\t\tif (hashcmp(a->sha1, b->sha1) <= 0) {\n+\t\t\tresult[n] = a;\n+\t\t\ti++;\n+\t\t} else {\n+\t\t\tresult[n] = b;\n+\t\t\tj++;\n+\t\t}\n+\t}\n+\n+\tfor (; i < left_nr; ++n, ++i)\n+\t\tresult[n] = left[i];\n+\n+\tfor (; j < right_nr; ++n, ++j)\n+\t\tresult[n] = right[j];\n+\n+\tfree(written_list);\n+\tfree(right);\n+\n+\twritten_list = result;\n+\twritten_nr = total_nr;\n+}\n+\n+static off_t sha1_index_find_offset(const unsigned char *sha1)\n+{\n+\tint pos = sha1_pos(sha1, written_list, written_nr, sha1_index__access);\n+\treturn (pos < 0) ? 0 : written_list[pos]->offset;\n+}\n+\n+static void copy_pack_data(\n+\t\tstruct packwriter *w,\n+\t\tstruct packed_git *p,\n+\t\tstruct pack_window **w_curs,\n+\t\toff_t offset,\n+\t\toff_t len)\n+{\n+\tunsigned char *in;\n+\tunsigned long avail;\n+\n+\twhile (len) {\n+\t\tin = use_pack(p, w_curs, offset, &avail);\n+\t\tif (avail > len)\n+\t\t\tavail = (unsigned long)len;\n+\t\tpackwriter_write(w, in, avail);\n+\t\toffset += avail;\n+\t\tlen -= avail;\n+\t}\n+}\n+\n+extern enum object_type packed_to_object_type(\n+\tstruct packed_git *p, off_t obj_offset, enum object_type type,\n+\tstruct pack_window **w_curs, off_t curpos);\n+\n+static int append_object_1(\n+\tstruct revindex_entry *reventry,\n+\tstruct packwriter *w,\n+\tstruct packed_git *pack,\n+\tstruct pack_window **w_curs,\n+\tenum object_type *real_type)\n+{\n+\tconst off_t offset = reventry[0].offset;\n+\tconst off_t next = reventry[1].offset;\n+\n+\toff_t cur;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\trecord_reused_object(offset, offset - packwriter_total(w));\n+\n+\tcur = offset;\n+\ttype = unpack_object_header(pack, w_curs, &cur, &size);\n+\tassert(type >= 0);\n+\n+\tif (write_bitmap_index)\n+\t\t*real_type = packed_to_object_type(pack, offset, type, w_curs, cur);\n+\n+\tif (type == OBJ_OFS_DELTA) {\n+\t\tconst off_t base_offset = get_delta_base(pack, w_curs, &cur, type, offset);\n+\t\tconst off_t real_base_offset = find_real_offset_for_base(base_offset);\n+\t\toff_t fixed_offset = 0;\n+\n+\t\tassert(base_offset != 0);\n+\n+\t\tif (real_base_offset) {\n+\t\t\tfixed_offset = packwriter_total(w) - real_base_offset;\n+\t\t} else {\n+\t\t\toff_t fixup = find_reused_offset(offset) - find_reused_offset(base_offset);\n+\t\t\tif (fixup)\n+\t\t\t\tfixed_offset = offset - base_offset - fixup;\n+\t\t}\n+\n+\t\tif (fixed_offset) {\n+\t\t\tunsigned char header[10], ofs_header[10];\n+\t\t\tunsigned i, len, ofs_len;\n+\n+\t\t\tassert(fixed_offset > 0);\n+\t\t\tlen = encode_in_pack_object_header(OBJ_OFS_DELTA, size, header);\n+\n+\t\t\ti = sizeof(ofs_header) - 1;\n+\t\t\tofs_header[i] = fixed_offset & 127;\n+\t\t\twhile (fixed_offset >>= 7)\n+\t\t\t\tofs_header[--i] = 128 | (--fixed_offset & 127);\n+\n+\t\t\tofs_len = sizeof(ofs_header) - i;\n+\n+\t\t\tpackwriter_write(w, header, len);\n+\t\t\tpackwriter_write(w, ofs_header + sizeof(ofs_header) - ofs_len, ofs_len);\n+\t\t\tcopy_pack_data(w, pack, w_curs, cur, next - cur);\n+\t\t\treturn 1;\n+\t\t}\n+\n+\t\t/* ...otherwise we have no fixup, and can write it verbatim */\n+\t}\n+\n+\tcopy_pack_data(w, pack, w_curs, offset, next - offset);\n+\treturn 0;\n+}\n+\n+static int copy_packfile(int from, struct packwriter *w)\n+{\n+\tunsigned char buffer[8192];\n+\tstruct stat st;\n+\tssize_t to_read;\n+\n+\tif (from < 0 || fstat(from, &st))\n+\t\treturn -1;\n+\n+\tposix_fadvise(from, 0, st.st_size, POSIX_FADV_SEQUENTIAL);\n+\tto_read = st.st_size - 20;\n+\n+\tif (progress)\n+\t\tfprintf(stderr, \"Copying main packfile...\");\n+\n+\twhile (to_read) {\n+\t\tssize_t r, cap = sizeof(buffer);\n+\n+\t\tif (cap > to_read)\n+\t\t\tcap = to_read;\n+\n+\t\tr = xread(from, buffer, cap);\n+\t\tif (r < 0)\n+\t\t\treturn -1;\n+\n+\t\tpackwriter_write(w, buffer, r);\n+\t\tto_read -= r;\n+\t}\n+\n+\tif (progress)\n+\t\tfprintf(stderr, \" done.\\n\");\n+\tassert(to_read == 0);\n+\treturn 0;\n+}\n+\n+static void write_initial_packfile(struct packed_git *p, struct packwriter *w)\n+{\n+\tunsigned int n;\n+\tint source_fd = git_open_noatime(p->pack_name);\n+\n+\tif (copy_packfile(source_fd, w) < 0)\n+\t\tdie_errno(\"failed to copy '%s'\", p->pack_name);\n+\tclose(source_fd);\n+\n+\tload_index_or_die(p);\n+\tpreallocate_write_slab(p->num_objects);\n+\n+\tif (progress)\n+\t\tprogress_state = start_progress(\"Indexing main packfile\", p->num_objects);\n+\n+\tfor (n = 0; n < p->num_objects; ++n) {\n+\t\tconst unsigned char *sha1 = nth_packed_object_sha1(p, n);\n+\t\tconst off_t offset = nth_packed_object_offset(p, n);\n+\t\tconst uint32_t crc32 = nth_packed_object_crc32(p, n);\n+\t\tadd_to_write_list(sha1, offset, crc32, OBJ_BAD);\n+\t\tdisplay_progress(progress_state, n + 1);\n+\t}\n+\n+\tstop_progress(&progress_state);\n+\tclose_pack_index(p);\n+\n+\twritten_list = xmalloc(p->num_objects * sizeof(struct packed_git *));\n+\twritten_nr = p->num_objects;\n+\tfor (n = 0; n < written_nr; ++n)\n+\t\twritten_list[n] = (struct pack_idx_entry *)(&written_slab_current->entries[n]);\n+}\n+\n+static void append_packfile(struct packed_git *p, struct packwriter *w)\n+{\n+\tstruct pack_window *w_curs = NULL;\n+\tstruct pack_revindex *revidx;\n+\n+\tunsigned int n;\n+\n+\tload_index_or_die(p);\n+\tpreallocate_write_slab(p->num_objects);\n+\trevidx = revindex_for_pack(p);\n+\n+\tif (progress)\n+\t\tprogress_state = start_progress(\"Appending packfile\", p->num_objects);\n+\n+\tfor (n = 0; n < p->num_objects; ++n) {\n+\t\tstruct revindex_entry *reventry = &revidx->revindex[n];\n+\t\tconst unsigned char *sha1 = nth_packed_object_sha1(p, reventry[0].nr);\n+\t\tconst off_t offset_in_pack = sha1_index_find_offset(sha1);\n+\n+\t\tif (!offset_in_pack) {\n+\t\t\tconst off_t offset = packwriter_total(w);\n+\n+\t\t\tenum object_type real_type = OBJ_BAD;\n+\t\t\tuint32_t crc32;\n+\t\t\tint rewrite_header;\n+\n+\t\t\tpackwriter_crc32_start(w);\n+\t\t\trewrite_header = append_object_1(reventry, w, p, &w_curs, &real_type);\n+\t\t\tcrc32 = packwriter_crc32_end(w);\n+\n+\t\t\tif (!rewrite_header && crc32 != nth_packed_object_crc32(p, reventry[0].nr))\n+\t\t\t\tdie(\"crc32 check failed for %s\", sha1_to_hex(sha1));\n+\n+\t\t\tadd_to_write_list(sha1, offset, crc32, real_type);\n+\t\t} else {\n+\t\t\tadd_to_skipped_list(reventry[0].offset, offset_in_pack);\n+\t\t}\n+\n+\t\tdisplay_progress(progress_state, n + 1);\n+\t}\n+\n+\tstop_progress(&progress_state);\n+\tunuse_pack(&w_curs);\n+\tclose_pack_windows(p);\n+\tclose_pack_index(p);\n+\n+\tsha1_index_update();\n+\tskipped_nr = 0;\n+\treused_chunks_nr = 0;\n+}\n+\n+static void write_packs(void)\n+{\n+\tstruct packwriter w;\n+\tunsigned int i;\n+\n+\tpackwriter_init(&w);\n+\twrite_initial_packfile(all_packfiles[0], &w);\n+\n+\tfor (i = 1; i < all_packfiles_nr; ++i)\n+\t\tappend_packfile(all_packfiles[i], &w);\n+\n+\t/* finalize pack */\n+\t{\n+\t\tunsigned char sha1[20];\n+\t\tstruct strbuf tmpname = STRBUF_INIT;\n+\n+\t\tfixup_pack_header_footer(w.fd, sha1, w.tmp->filename.buf, written_nr, NULL, 0);\n+\t\tclose(w.fd);\n+\n+\t\tstrbuf_addf(&tmpname, \"%s-\", base_name);\n+\n+\t\tfinish_tmp_packfile(&tmpname, w.tmp->filename.buf,\n+\t\t\t\twritten_list, written_nr,\n+\t\t\t\t&pack_idx_opts, sha1);\n+\n+\t\tif (write_bitmap_index) {\n+\t\t\tstrbuf_addf(&tmpname, \"%s.bitmap\", sha1_to_hex(sha1));\n+\t\t\tbitmap_rewrite_existing(\n+\t\t\t\tall_packfiles[0],\n+\t\t\t\twritten_list, written_nr,\n+\t\t\t\tpackwriter_total(&w),\n+\t\t\t\tsha1, tmpname.buf);\n+\t\t}\n+\n+\t\tstrbuf_release(&tmpname);\n+\t\tputs(sha1_to_hex(sha1));\n+\t}\n+}\n+\n+void pack_fast_grow_typemaps(struct packed_git *p, struct ewah_bitmap **typemaps)\n+{\n+\tuint32_t n;\n+\tsize_t pos = p->num_objects;\n+\tstruct write_slab *slab = written_slab_root;\n+\n+\tassert(slab->nr == p->num_objects);\n+\tassert(slab->next);\n+\tslab = slab->next;\n+\n+\twhile (slab) {\n+\t\tfor (n = 0; n < slab->nr; ++n) {\n+\t\t\tconst enum object_type real_type = slab->entries[n].real_type;\n+\t\t\tassert(real_type >= OBJ_COMMIT && real_type <= OBJ_TAG);\n+\t\t\tewah_set(typemaps[real_type - 1], pos++);\n+\t\t}\n+\t\tslab = slab->next;\n+\t}\n+}\n+\n+int cmd_pack_fast(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option pack_fast_options[] = {\n+\t\tOPT_SET_INT('q', \"quiet\", &progress,\n+\t\t\t    N_(\"do not show progress meter\"), 0),\n+\t\tOPT_SET_INT(0, \"progress\", &progress,\n+\t\t\t    N_(\"show progress meter\"), 1),\n+\t\tOPT_BOOL(0, \"skip-largest\", &skip_largest,\n+\t\t\t N_(\"do not pack the largest packfile in the repository\")),\n+\t\tOPT_END(),\n+\t};\n+\n+\treset_pack_idx_option(&pack_idx_opts);\n+\tprogress = isatty(2);\n+\targc = parse_options(argc, argv, prefix, pack_fast_options,\n+\t\t\t     pack_usage, 0);\n+\n+\tif (argc) {\n+\t\tbase_name = argv[0];\n+\t\targc--;\n+\t}\n+\n+\tfind_packfiles();\n+\twrite_packs();\n+\treturn 0;\n+}\ndiff --git a/cache.h b/cache.h\nindex 6f53962bf..1a13961bd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1336,6 +1336,11 @@ extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsign\n extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n extern int unpack_object_header(struct packed_git *, struct pack_window **, off_t *, unsigned long *);\n+extern off_t get_delta_base(struct packed_git *p,\n+\t\t\t    struct pack_window **w_curs,\n+\t\t\t    off_t *curpos,\n+\t\t\t    enum object_type type,\n+\t\t\t    off_t delta_obj_offset);\n \n /*\n  * Iterate over the files in the loose-object parts of the object\ndiff --git a/git.c b/git.c\nindex 40f9df089..d81bd4469 100644\n--- a/git.c\n+++ b/git.c\n@@ -440,6 +440,7 @@ static struct cmd_struct commands[] = {\n \t{ \"name-rev\", cmd_name_rev, RUN_SETUP },\n \t{ \"notes\", cmd_notes, RUN_SETUP },\n \t{ \"pack-objects\", cmd_pack_objects, RUN_SETUP },\n+\t{ \"pack-fast\", cmd_pack_fast, RUN_SETUP },\n \t{ \"pack-redundant\", cmd_pack_redundant, RUN_SETUP },\n \t{ \"pack-refs\", cmd_pack_refs, RUN_SETUP },\n \t{ \"patch-id\", cmd_patch_id },\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex c05d1386a..449715f02 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -505,23 +505,39 @@ void bitmap_writer_set_checksum(unsigned char *sha1)\n \thashcpy(writer.pack_checksum, sha1);\n }\n \n+static struct sha1file *bitmap_file_new(char *tmp_file, size_t len)\n+{\n+\tint fd = odb_mkstemp(tmp_file, len, \"pack/tmp_bitmap_XXXXXX\");\n+\n+\tif (fd < 0)\n+\t\tdie_errno(\"unable to create '%s'\", tmp_file);\n+\n+\treturn sha1fd(fd, tmp_file);\n+}\n+\n+static void bitmap_file_close(struct sha1file *f, const char *tmp_file, const char *dest)\n+{\n+\tsha1close(f, NULL, CSUM_FSYNC);\n+\n+\tif (adjust_shared_perm(tmp_file))\n+\t\tdie_errno(\"unable to make temporary bitmap file readable\");\n+\n+\tif (rename(tmp_file, dest))\n+\t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", dest);\n+}\n+\n void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\t\t  uint32_t index_nr,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options)\n {\n-\tstatic char tmp_file[PATH_MAX];\n \tstatic uint16_t default_version = 1;\n \tstatic uint16_t flags = BITMAP_OPT_FULL_DAG;\n+\tchar tmp_file[PATH_MAX];\n \tstruct sha1file *f;\n-\n \tstruct bitmap_disk_header header;\n \n-\tint fd = odb_mkstemp(tmp_file, sizeof(tmp_file), \"pack/tmp_bitmap_XXXXXX\");\n-\n-\tif (fd < 0)\n-\t\tdie_errno(\"unable to create '%s'\", tmp_file);\n-\tf = sha1fd(fd, tmp_file);\n+\tf = bitmap_file_new(tmp_file, sizeof(tmp_file));\n \n \tmemcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));\n \theader.version = htons(default_version);\n@@ -539,11 +555,138 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \tif (options & BITMAP_OPT_HASH_CACHE)\n \t\twrite_hash_cache(f, index, index_nr);\n \n-\tsha1close(f, NULL, CSUM_FSYNC);\n+\tbitmap_file_close(f, tmp_file, filename);\n+}\n \n-\tif (adjust_shared_perm(tmp_file))\n-\t\tdie_errno(\"unable to make temporary bitmap file readable\");\n+static void *try_load_bitmap(struct packed_git *p, size_t *_size_out)\n+{\n+\tvoid *reused_bitmap;\n+\tsize_t reused_bitmap_size;\n+\n+\tint fd;\n+\tstruct stat st;\n+\tchar *idx_name;\n+\n+\tidx_name = pack_bitmap_filename(p);\n+\tfd = git_open_noatime(idx_name);\n+\tfree(idx_name);\n+\n+\tif (fd < 0)\n+\t\treturn NULL;\n+\n+\tif (fstat(fd, &st)) {\n+\t\tclose(fd);\n+\t\treturn NULL;\n+\t}\n+\n+\treused_bitmap_size = xsize_t(st.st_size);\n+\treused_bitmap = xmmap(NULL, reused_bitmap_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\n+\t*_size_out = reused_bitmap_size;\n+\treturn reused_bitmap;\n+}\n+\n+extern void pack_fast_grow_typemaps(struct packed_git *p, struct ewah_bitmap **typemaps);\n+\n+static size_t rewrite_type_maps(struct sha1file *f,\n+\tstruct packed_git *p, unsigned char *original_map, size_t original_size, size_t pos)\n+{\n+\tstruct ewah_bitmap *typemaps[4];\n+\tint r, i;\n+\n+\tfor (i = 0; i < 4; ++i) {\n+\t\ttypemaps[i] = ewah_pool_new();\n+\t\tr = ewah_read_mmap(typemaps[i], original_map + pos, original_size - pos);\n+\t\tif (r < 0)\n+\t\t\tdie(\"failed to read bitmap index\");\n+\t\tpos += r;\n+\t}\n+\n+\tpack_fast_grow_typemaps(p, typemaps);\n+\n+\tfor (i = 0; i < 4; ++i) {\n+\t\tdump_bitmap(f, typemaps[i]);\n+\t\tewah_pool_free(typemaps[i]);\n+\t}\n+\n+\treturn pos;\n+}\n+\n+static size_t rewrite_bitmaps(struct sha1file *f,\n+\tstruct packed_git *p, unsigned char *original_map, size_t original_size, size_t pos,\n+\tuint32_t entry_count, struct pack_idx_entry **index, uint32_t index_nr)\n+{\n+\tuint32_t i;\n+\n+\tfor (i = 0; i < entry_count; ++i) {\n+\t\tconst unsigned char *sha1;\n+\t\tuint32_t src_idx, src_buffer_len, total_len;\n+\t\tint new_idx;\n+\n+\t\tsrc_idx = get_be32(original_map + pos);\n+\t\tpos += 4;\n+\n+\t\tsha1 = nth_packed_object_sha1(p, src_idx);\n+\t\tnew_idx = sha1_pos(sha1, index, index_nr, sha1_access);\n+\t\tsha1write_be32(f, (uint32_t)new_idx);\n+\n+\t\tsrc_buffer_len = get_be32(original_map + pos + 2 + 4);\n+\t\ttotal_len = (3 * 4) + (src_buffer_len * 8);\n+\n+\t\tsha1write(f, original_map + pos, 2 + total_len);\n+\t\tpos += 2 + total_len;\n+\n+\t\tif (pos > original_size)\n+\t\t\tdie(\"unexpected end of file\");\n+\t}\n+\n+\treturn pos;\n+}\n+\n+void bitmap_rewrite_existing(\n+\tstruct packed_git *p,\n+\tstruct pack_idx_entry **index,\n+\tuint32_t index_nr,\n+\toff_t pack_offset,\n+\tconst unsigned char *pack_sha1,\n+\tconst char *filename)\n+{\n+\tchar tmp_file[PATH_MAX];\n+\tstruct sha1file *f;\n+\n+\tunsigned char *original_map;\n+\tsize_t original_size, pos = 0;\n+\tstruct bitmap_disk_header header;\n+\n+\toriginal_map = try_load_bitmap(p, &original_size);\n+\tif (!original_map || original_size < sizeof(header) + 20)\n+\t\treturn;\n+\n+\tmemcpy(&header, original_map, sizeof(header));\n+\thashcpy(header.checksum, pack_sha1);\n+\n+\tif (memcmp(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE)) != 0)\n+\t\tdie(\"existing bitmap for '%s' is corrupted\", p->pack_name);\n+\n+\tif (ntohs(header.version) != 1)\n+\t\tdie(\"existing bitmap for '%s' has an unsupported version\", p->pack_name);\n+\n+\tf = bitmap_file_new(tmp_file, sizeof(tmp_file));\n+\n+\tsha1write(f, &header, sizeof(header));\n+\tpos = sizeof(header);\n+\tpos = rewrite_type_maps(f, p, original_map, original_size, pos);\n+\tpos = rewrite_bitmaps(f, p, original_map, original_size, pos,\n+\t\t\tntohl(header.entry_count), index, index_nr);\n+\n+\tif (ntohs(header.options) & BITMAP_OPT_HASH_CACHE) {\n+\t\tuint32_t i, zero = 0;\n+\t\tsha1write(f, original_map + pos, p->num_objects * 4);\n+\t\tfor (i = p->num_objects; i < index_nr; ++i)\n+\t\t\tsha1write(f, &zero, 4);\n+\t\tpos += (p->num_objects * 4);\n+\t}\n \n-\tif (rename(tmp_file, filename))\n-\t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", filename);\n+\tbitmap_file_close(f, tmp_file, filename);\n }\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 637770af8..ee361fa6a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -250,7 +250,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \treturn 0;\n }\n \n-static char *pack_bitmap_filename(struct packed_git *p)\n+char *pack_bitmap_filename(struct packed_git *p)\n {\n \tchar *idx_name;\n \tint len;\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 0adcef77b..398523dbb 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -34,6 +34,7 @@ typedef int (*show_reachable_fn)(\n \tstruct packed_git *found_pack,\n \toff_t found_offset);\n \n+char *pack_bitmap_filename(struct packed_git *p);\n int prepare_bitmap_git(void);\n void count_bitmap_commit_list(uint32_t *commits, uint32_t *trees, uint32_t *blobs, uint32_t *tags);\n void traverse_bitmap_commit_list(show_reachable_fn show_reachable);\n@@ -53,5 +54,12 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\t\t  uint32_t index_nr,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options);\n+void bitmap_rewrite_existing(\n+\tstruct packed_git *p,\n+\tstruct pack_idx_entry **index,\n+\tuint32_t index_nr,\n+\toff_t pack_offset,\n+\tconst unsigned char *pack_sha1,\n+\tconst char *filename);\n \n #endif\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 72289696d..bcd447f16 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1821,7 +1821,7 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \treturn get_delta_hdr_size(&data, delta_head+sizeof(delta_head));\n }\n \n-static off_t get_delta_base(struct packed_git *p,\n+off_t get_delta_base(struct packed_git *p,\n \t\t\t\t    struct pack_window **w_curs,\n \t\t\t\t    off_t *curpos,\n \t\t\t\t    enum object_type type,\n@@ -1936,7 +1936,7 @@ static int retry_bad_packed_offset(struct packed_git *p, off_t obj_offset)\n \n #define POI_STACK_PREALLOC 64\n \n-static enum object_type packed_to_object_type(struct packed_git *p,\n+enum object_type packed_to_object_type(struct packed_git *p,\n \t\t\t\t\t      off_t obj_offset,\n \t\t\t\t\t      enum object_type type,\n \t\t\t\t\t      struct pack_window **w_curs,\n"},{"id":"311152","messageId":"1486580742.1938.52.camel@novalis.org","threadId":"44719","inReplyTo":"xmqqtw84wpag.fsf@gitster.mtv.corp.google.com","subject":"Re: \"disabling bitmap writing, as some objects are not being packed\"?","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2017-02-08T19:05:42Z","receivedAt":"2017-02-09T07:12:34Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2017-02-08 at 09:44 -0800, Junio C Hamano wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> > On second thought, perhaps gc.autoDetach should default to false if\n> > there's no tty, since its main point it to stop breaking interactive\n> > usage. That would make the server side happy (no tty there).\n> \n> Sounds like an idea, but wouldn't that keep the end-user coming over\n> the network waiting after accepting a push until the GC completes, I\n> wonder.  If an impatient user disconnects, would that end up killing\n> an ongoing GC?  etc.\n\nRegardless, it's impolite to keep the user waiting. So, I think we\nshould just not write the \"too many unreachable loose objects\" message\nif auto-gc is on.  Does that sound OK?\n\n\n"}]}