{"thread":{"id":"42163","subject":"[PATCH] pack-objects: warn on split packs disabling bitmaps","startedAt":"2016-04-27T21:53:24Z","lastAt":"2016-04-28T08:02:28Z","messageCount":6,"participants":["Eric Wong","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"284754","messageId":"20160427215324.GA22165@dcvr.yhbt.net","threadId":"42163","inReplyTo":null,"subject":"[PATCH] pack-objects: warn on split packs disabling bitmaps","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-27T21:53:24Z","receivedAt":"2016-04-27T21:53:24Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"It can be tempting for a server admin to want a stable set of\nlong-lived packs for dumb clients; but also want to enable\nbitmaps to serve smart clients more quickly.\n\nUnfortunately, such a configuration is impossible;\nso at least warn users of this incompatibility since\ncommit 21134714787a02a37da15424d72c0119b2b8ed71\n(\"pack-objects: turn off bitmaps when we split packs\").\n\nTested the warning by inspecting the output of:\n\n\tmake -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v\n\nSigned-off-by: Eric Wong \n---\n Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of\n our ML archives, soonish. I can serve terabytes of dumb HTTP\n traffic all day long without breaking a sweat; but smart\n packing of big repos worries me; especially when feeding\n slow clients and having to leave processes running\n (or buffering pack output to disk). So perhaps I'll teach\n my HTTP server play dumb whenever CPU/memory usage is high.\n\n Documentation/config.txt | 11 +++++++----\n Documentation/git-pack-objects.txt | 3 ++-\n Documentation/git-repack.txt | 9 ++++++---\n builtin/pack-objects.c | 9 ++++++++-\n 4 files changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 42d2b50..ec31170 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2156,8 +2156,10 @@ pack.packSizeLimit::\n \tThe maximum size of a pack. This setting only affects\n \tpacking to a file when repacking, i.e. the git:// protocol\n \tis unaffected. It can be overridden by the `--max-pack-size`\n-\toption of linkgit:git-repack[1]. The minimum size allowed is\n-\tlimited to 1 MiB. The default is unlimited.\n+\toption of linkgit:git-repack[1]. Reaching this limit prevents\n+\tpack bitmaps from being written when `repack.writeBitmaps`\n+\tis configured. The minimum size allowed is limited to 1 MiB.\n+\tThe default is unlimited.\n \tCommon unit suffixes of 'k', 'm', or 'g' are\n \tsupported.\n \n@@ -2557,8 +2559,9 @@ repack.writeBitmaps::\n \tobjects to disk (e.g., when `git repack -a` is run). This\n \tindex can speed up the \"counting objects\" phase of subsequent\n \tpacks created for clones and fetches, at the cost of some disk\n-\tspace and extra time spent on the initial repack. Defaults to\n-\tfalse.\n+\tspace and extra time spent on the initial repack. This has\n+\tno effect if `pack.packSizeLimit` is configured and reached.\n+\tDefaults to false.\n \n rerere.autoUpdate::\n \tWhen set to true, `git-rerere` updates the index with the\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex bbea529..19cdcd0 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -110,7 +110,8 @@ base-name::\n --max-pack-size=::\n \tMaximum size of each output pack file. The size can be suffixed with\n \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n-\tIf specified, multiple packfiles may be created.\n+\tIf specified, multiple packfiles may be created, which also\n+\tprevents the creation of a bitmap index.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex af230d0..2065812 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -106,7 +106,8 @@ other objects in that pack they already have locally.\n --max-pack-size=::\n \tMaximum size of each output pack file. The size can be suffixed with\n \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n-\tIf specified, multiple packfiles may be created.\n+\tIf specified, multiple packfiles may be created, causing\n+\t`--write-bitmap-index` and `repack.writeBitmaps` to be ignored.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \n@@ -115,7 +116,9 @@ other objects in that pack they already have locally.\n \tWrite a reachability bitmap index as part of the repack. This\n \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n \tmust be able to refer to all reachable objects. This option\n-\toverrides the setting of `pack.writeBitmaps`.\n+\toverrides the setting of `repack.writeBitmaps`. This option\n+\thas no effect if a multiple packfiles are created due to\n+\treaching `pack.packSizeLimit` or `--max-pack-size`.\n \n --pack-kept-objects::\n \tInclude objects in `.keep` files when repacking. Note that we\n@@ -123,7 +126,7 @@ other objects in that pack they already have locally.\n \tThis means that we may duplicate objects, but this makes the\n \toption safe to use when there are concurrent pushes or fetches.\n \tThis option is generally only useful if you are writing bitmaps\n-\twith `-b` or `pack.writeBitmaps`, as it ensures that the\n+\twith `-b` or `repack.writeBitmaps`, as it ensures that the\n \tbitmapped packfile has the necessary objects.\n \n Configuration\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a27de5b..b6664ce 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -759,6 +759,10 @@ static off_t write_reused_pack(struct sha1file *f)\n \treturn reuse_packfile_offset - sizeof(struct pack_header);\n }\n \n+static const char no_split_warning[] = N_(\n+\"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n+);\n+\n static void write_pack_file(void)\n {\n \tuint32_t i = 0, j;\n@@ -813,7 +817,10 @@ static void write_pack_file(void)\n \t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n \t\t\t\t\t\t nr_written, sha1, offset);\n \t\t\tclose(fd);\n-\t\t\twrite_bitmap_index = 0;\n+\t\t\tif (write_bitmap_index) {\n+\t\t\t\twarning(_(no_split_warning));\n+\t\t\t\twrite_bitmap_index = 0;\n+\t\t\t}\n \t\t}\n \n \t\tif (!pack_to_stdout) {\n-- \nEW\n"},{"id":"284760","messageId":"xmqqfuu67j9t.fsf@gitster.mtv.corp.google.com","threadId":"42163","inReplyTo":"20160427215324.GA22165@dcvr.yhbt.net","subject":"Re: [PATCH] pack-objects: warn on split packs disabling bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-27T22:56:46Z","receivedAt":"2016-04-27T22:56:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> It can be tempting for a server admin to want a stable set of\n> long-lived packs for dumb clients; but also want to enable\n> bitmaps to serve smart clients more quickly.\n>\n> Unfortunately, such a configuration is impossible;\n> so at least warn users of this incompatibility since\n> commit 21134714787a02a37da15424d72c0119b2b8ed71\n> (\"pack-objects: turn off bitmaps when we split packs\").\n>\n> Tested the warning by inspecting the output of:\n>\n> \tmake -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v\n\nWhile I do not think the update in this patch is wrong, this makes\nme wonder what happens to a server admin who wants a stable set of\nlong-lived objects in a pack, and other objects in another pack that\nis frequently recreated, by first packing objects needed for the\nlatest released version into a single pack and marking it with .keep\nand then running \"git repack\" to create the second pack for the\nremainder.\n\nThere is no automatic split involved, so we would get a bitmap file\nfor each of these two packs. Would that pose a problem? The pack\nwith the newer part of the history would not satisfy \"all of the\nreachable objects are in the same pack\" condition.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 42d2b50..ec31170 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2156,8 +2156,10 @@ pack.packSizeLimit::\n> \tThe maximum size of a pack. This setting only affects\n> \tpacking to a file when repacking, i.e. the git:// protocol\n> \tis unaffected. It can be overridden by the `--max-pack-size`\n> -\toption of linkgit:git-repack[1]. The minimum size allowed is\n> -\tlimited to 1 MiB. The default is unlimited.\n> +\toption of linkgit:git-repack[1]. Reaching this limit prevents\n> +\tpack bitmaps from being written when `repack.writeBitmaps`\n> +\tis configured. The minimum size allowed is limited to 1 MiB.\n> +\tThe default is unlimited.\n\nThis is not wrong per-se, but I find the additional text overly\nverbose. The resulting text has at least three issues:\n\n - This sets maximum. It does not say what happens when the maximum\n is reached (\"packing fails\" is a valid expectation). We should\n spell out that when the maximum is reached, we will create\n multiple packfiles.\n\n - It is not \"reaching this limit\" that prevents bitmaps from being\n written. It is \"we will create multiple packfiles\" that does.\n\n - Even when repack.writeBitmaps is not configured, but bitmap is\n requested via the command line option \"repack -b\", bitmap\n generation is disabled once we end up creating multiple\n packfiles.\n\n> @@ -2557,8 +2559,9 @@ repack.writeBitmaps::\n> \tobjects to disk (e.g., when `git repack -a` is run). This\n> \tindex can speed up the \"counting objects\" phase of subsequent\n> \tpacks created for clones and fetches, at the cost of some disk\n> -\tspace and extra time spent on the initial repack. Defaults to\n> -\tfalse.\n> +\tspace and extra time spent on the initial repack. This has\n> +\tno effect if `pack.packSizeLimit` is configured and reached.\n> +\tDefaults to false.\n\nThis has the opposite issue as the third point above. We have to\nignore repack.writeBitmaps when we end up splitting a pack,\nregardless of the reason why we split was pack.packSizeLimit or by\nan explicit request from the command line.\n\nAlso to future-proof these two paragraphs (and those that are\ntouched by later parts of this patch), it may even be a good idea to\nomit the mention of packsizelimit altogether. We may invent a new\nand different reason why we end up producing multiple packfiles,\nother than packsizelimit. What this patch wants to make readers\naware is that when multiple packs are generated, bitmap generation\nis disabled. Other details (e.g. how the user asks us to create\nmultiple packs, either via command line or configuration, or how the\nuse asks us to generate bitmaps) are of lessor importance.\n\n> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\n> index bbea529..19cdcd0 100644\n> --- a/Documentation/git-pack-objects.txt\n> +++ b/Documentation/git-pack-objects.txt\n> @@ -110,7 +110,8 @@ base-name::\n> --max-pack-size=::\n> \tMaximum size of each output pack file. The size can be suffixed with\n> \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n> -\tIf specified, multiple packfiles may be created.\n> +\tIf specified, multiple packfiles may be created, which also\n> +\tprevents the creation of a bitmap index.\n\nThis is a good update, judging with the yardstick I set in the\nprevious paragraph in this review.\n\n> \tThe default is unlimited, unless the config variable\n> \t`pack.packSizeLimit` is set.\n> \n> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> index af230d0..2065812 100644\n> --- a/Documentation/git-repack.txt\n> +++ b/Documentation/git-repack.txt\n> @@ -106,7 +106,8 @@ other objects in that pack they already have locally.\n> --max-pack-size=::\n> \tMaximum size of each output pack file. The size can be suffixed with\n> \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n> -\tIf specified, multiple packfiles may be created.\n> +\tIf specified, multiple packfiles may be created, causing\n> +\t`--write-bitmap-index` and `repack.writeBitmaps` to be ignored.\n\nAnd this is not. Just say \"bitmap index is not created\".\n\n> @@ -115,7 +116,9 @@ other objects in that pack they already have locally.\n> \tWrite a reachability bitmap index as part of the repack. This\n> \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n> \tmust be able to refer to all reachable objects. This option\n> -\toverrides the setting of `pack.writeBitmaps`.\n> +\toverrides the setting of `repack.writeBitmaps`. This option\n> +\thas no effect if a multiple packfiles are created due to\n> +\treaching `pack.packSizeLimit` or `--max-pack-size`.\n\nDropping \"due to ...\" makes it perfect.\n"},{"id":"284764","messageId":"20160428021509.GB9707@sigill.intra.peff.net","threadId":"42163","inReplyTo":"xmqqfuu67j9t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pack-objects: warn on split packs disabling bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T02:15:09Z","receivedAt":"2016-04-28T02:15:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 27, 2016 at 03:56:46PM -0700, Junio C Hamano wrote:\n\n> Eric Wong writes:\n> \n> > It can be tempting for a server admin to want a stable set of\n> > long-lived packs for dumb clients; but also want to enable\n> > bitmaps to serve smart clients more quickly.\n> >\n> > Unfortunately, such a configuration is impossible;\n> > so at least warn users of this incompatibility since\n> > commit 21134714787a02a37da15424d72c0119b2b8ed71\n> > (\"pack-objects: turn off bitmaps when we split packs\").\n> >\n> > Tested the warning by inspecting the output of:\n> >\n> > \tmake -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v\n> \n> While I do not think the update in this patch is wrong, this makes\n> me wonder what happens to a server admin who wants a stable set of\n> long-lived objects in a pack, and other objects in another pack that\n> is frequently recreated, by first packing objects needed for the\n> latest released version into a single pack and marking it with .keep\n> and then running \"git repack\" to create the second pack for the\n> remainder.\n> \n> There is no automatic split involved, so we would get a bitmap file\n> for each of these two packs. Would that pose a problem? The pack\n> with the newer part of the history would not satisfy \"all of the\n> reachable objects are in the same pack\" condition.\n\nYou will not get two bitmaps in such a case. In add_object_entry(), if\nwe exclude an object via want_object_in_pack(), we will issue a warning\nand turn off bitmaps. That includes finding that one of the reachable\nobjects we would need is in a .keep pack.\n\nYou could in theory construct a broken non-closed bitmap by feeding an\narbitrary set of objects to pack-objects, but we turn off bitmap writing\nentirely unless --all is used. So the test in add_object_entry() should\nbe sufficient for all cases there (it's actually too conservative; it's\npossible that the object is not reachable from the other objects that\nare going into the pack, but this is so impractical that it's not even\nworth trying to catch).\n\nThe split-pack check from 211347147 had to come separately, because we\nonly find out about the split much later during the write phase (and\nagain, it is too conservative; it's _possible_ that the objects being\nsplit aren't reachable from anything in the previous pack, but it's\nexceedingly unlikely and not worth caring about).\n\nAdding a warning as Eric's patch does is a definite improvement, and\nsomething I should have done in the original (we _could_ just use the\nsame no_closure_warning, as that is technically what is going on, but I\nthink it is nice to mention pack-splitting, which is more likely to lead\nthe user in the right direction to fixing it).\n\n> [discussion of doc fixes]\n\nI do not mind overly if pack-splitting mentions disabling bitmaps. But I\nthink it may be simpler to keep the documentation in the bitmap section,\nrather than trying to cross-reference in both directions. It is the\nbitmap code which is not prepared to work with pack-splits (and other\nthings, like .keep), not the other way around.\n\n-Peff\n"},{"id":"284765","messageId":"20160428022514.GC9707@sigill.intra.peff.net","threadId":"42163","inReplyTo":"20160427215324.GA22165@dcvr.yhbt.net","subject":"Re: [PATCH] pack-objects: warn on split packs disabling bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T02:25:14Z","receivedAt":"2016-04-28T02:25:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 27, 2016 at 09:53:24PM +0000, Eric Wong wrote:\n\n> It can be tempting for a server admin to want a stable set of\n> long-lived packs for dumb clients; but also want to enable\n> bitmaps to serve smart clients more quickly.\n> \n> Unfortunately, such a configuration is impossible;\n> so at least warn users of this incompatibility since\n> commit 21134714787a02a37da15424d72c0119b2b8ed71\n> (\"pack-objects: turn off bitmaps when we split packs\").\n> \n> Tested the warning by inspecting the output of:\n> \n> \tmake -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v\n\nI think the intent and code in your patch is fine; looks like doc\nspecifics are being discussed elsewhere.\n\nBut I did want to mention one thing, which is that long-lived split\npacks are a tradeoff, even for dumb clients. The pack format cannot do\ndeltas between packs, so the sum of your split packs is larger than a\nsingle pack would be. That's a good thing for somebody who cloned\nearlier, and wants to only a few small packs on top. But it's much worse\nfor somebody who wants to do a fresh clone, and has to grab all of the\npacks either way.\n\n> Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of\n> our ML archives, soonish. I can serve terabytes of dumb HTTP\n> traffic all day long without breaking a sweat; but smart\n> packing of big repos worries me; especially when feeding\n> slow clients and having to leave processes running\n> (or buffering pack output to disk). So perhaps I'll teach\n> my HTTP server play dumb whenever CPU/memory usage is high.\n\nYeah, CPU and memory load for serving large clones is a problem. Memory\nespecially scales with number of objects (because we keep the whole\npacking list in memory for the entirety of the write). At GitHub, we\nhave some changes to try to serve things verbatim from the on-disk pack\nwithout even creating an in-memory list of objects (it's just a bitmap\nof which objects in the packfile to send), and that reduces CPU and\nmemory load quite a bit. Cleaning up and submitting those patches has\nbeen on my todo list for a while, but I just haven't gotten to it. I'm\nof course happy to share the messy state if you want to pick through it\nyourself.\n\n-Peff\n"},{"id":"284775","messageId":"20160428072854.GA5252@dcvr.yhbt.net","threadId":"42163","inReplyTo":"xmqqfuu67j9t.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] pack-objects: warn on split packs disabling bitmaps","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-28T07:28:55Z","receivedAt":"2016-04-28T07:28:55Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> Eric Wong writes:\n> > +++ b/Documentation/git-pack-objects.txt\n> > @@ -110,7 +110,8 @@ base-name::\n> > --max-pack-size=::\n> > \tMaximum size of each output pack file. The size can be suffixed with\n> > \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n> > -\tIf specified, multiple packfiles may be created.\n> > +\tIf specified, multiple packfiles may be created, which also\n> > +\tprevents the creation of a bitmap index.\n> \n> This is a good update, judging with the yardstick I set in the\n> previous paragraph in this review.\n\nThanks for the review; made some adjustments and have v2 below.\n\n> > --- a/Documentation/git-repack.txt\n> > +++ b/Documentation/git-repack.txt\n> > @@ -106,7 +106,8 @@ other objects in that pack they already have locally.\n> > --max-pack-size=::\n> > \tMaximum size of each output pack file. The size can be suffixed with\n> > \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n> > -\tIf specified, multiple packfiles may be created.\n> > +\tIf specified, multiple packfiles may be created, causing\n> > +\t`--write-bitmap-index` and `repack.writeBitmaps` to be ignored.\n> \n> And this is not. Just say \"bitmap index is not created\".\n\nAh, I've now synced the same --max-pack-size doc from\ngit-pack-objects.txt you liked into v2 below.\n\nI worded my original differently between pack-objects and repack\nsince I figured repack is more likely used by end users;\nand perhaps warranted an explanation that didn't require\ndescribing the actual problem...\n\nBut I suppose \"repack\" isn't commonly called anymore, either.\n\n> > @@ -115,7 +116,9 @@ other objects in that pack they already have locally.\n> > \tWrite a reachability bitmap index as part of the repack. This\n> > \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n> > \tmust be able to refer to all reachable objects. This option\n> > -\toverrides the setting of `pack.writeBitmaps`.\n> > +\toverrides the setting of `repack.writeBitmaps`. This option\n> > +\thas no effect if a multiple packfiles are created due to\n> > +\treaching `pack.packSizeLimit` or `--max-pack-size`.\n> \n> Dropping \"due to ...\" makes it perfect.\n\nDone, along with:\n\n\ts/effect if a multiple/effect when multiple/\n\n\"if a\" was definitely a typo, \"if\" is probably alright, but\nI suspect \"when\" is even better.\n\n-------------8<-------------\nSubject: [PATCH] pack-objects: warn on split packs disabling bitmaps\n\nIt can be tempting for a server admin to want a stable set of\nlong-lived packs for dumb clients; but also want to enable\nbitmaps to serve smart clients more quickly.\n\nUnfortunately, such a configuration is impossible;\nso at least warn users of this incompatibility since\ncommit 21134714787a02a37da15424d72c0119b2b8ed71\n(\"pack-objects: turn off bitmaps when we split packs\").\n\nTested the warning by inspecting the output of:\n\n\tmake -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v\n\nSigned-off-by: Eric Wong \n---\n Documentation/config.txt | 12 ++++++++----\n Documentation/git-pack-objects.txt | 3 ++-\n Documentation/git-repack.txt | 8 +++++---\n builtin/pack-objects.c | 9 ++++++++-\n 4 files changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3d8bc97..3ea3372 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2165,8 +2165,11 @@ pack.packSizeLimit::\n \tThe maximum size of a pack. This setting only affects\n \tpacking to a file when repacking, i.e. the git:// protocol\n \tis unaffected. It can be overridden by the `--max-pack-size`\n-\toption of linkgit:git-repack[1]. The minimum size allowed is\n-\tlimited to 1 MiB. The default is unlimited.\n+\toption of linkgit:git-repack[1]. Reaching this limit results\n+\tin the creation of multiple packfiles; which in turn prevents\n+\tbitmaps from being created.\n+\tThe minimum size allowed is limited to 1 MiB.\n+\tThe default is unlimited.\n \tCommon unit suffixes of 'k', 'm', or 'g' are\n \tsupported.\n \n@@ -2566,8 +2569,9 @@ repack.writeBitmaps::\n \tobjects to disk (e.g., when `git repack -a` is run). This\n \tindex can speed up the \"counting objects\" phase of subsequent\n \tpacks created for clones and fetches, at the cost of some disk\n-\tspace and extra time spent on the initial repack. Defaults to\n-\tfalse.\n+\tspace and extra time spent on the initial repack. This has\n+\tno effect if multiple packfiles are created.\n+\tDefaults to false.\n \n rerere.autoUpdate::\n \tWhen set to true, `git-rerere` updates the index with the\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex bbea529..19cdcd0 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -110,7 +110,8 @@ base-name::\n --max-pack-size=::\n \tMaximum size of each output pack file. The size can be suffixed with\n \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n-\tIf specified, multiple packfiles may be created.\n+\tIf specified, multiple packfiles may be created, which also\n+\tprevents the creation of a bitmap index.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex af230d0..b9c02ce 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -106,7 +106,8 @@ other objects in that pack they already have locally.\n --max-pack-size=::\n \tMaximum size of each output pack file. The size can be suffixed with\n \t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n-\tIf specified, multiple packfiles may be created.\n+\tIf specified, multiple packfiles may be created, which also\n+\tprevents the creation of a bitmap index.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\n \n@@ -115,7 +116,8 @@ other objects in that pack they already have locally.\n \tWrite a reachability bitmap index as part of the repack. This\n \tonly makes sense when used with `-a` or `-A`, as the bitmaps\n \tmust be able to refer to all reachable objects. This option\n-\toverrides the setting of `pack.writeBitmaps`.\n+\toverrides the setting of `repack.writeBitmaps`. This option\n+\thas no effect if multiple packfiles are created.\n \n --pack-kept-objects::\n \tInclude objects in `.keep` files when repacking. Note that we\n@@ -123,7 +125,7 @@ other objects in that pack they already have locally.\n \tThis means that we may duplicate objects, but this makes the\n \toption safe to use when there are concurrent pushes or fetches.\n \tThis option is generally only useful if you are writing bitmaps\n-\twith `-b` or `pack.writeBitmaps`, as it ensures that the\n+\twith `-b` or `repack.writeBitmaps`, as it ensures that the\n \tbitmapped packfile has the necessary objects.\n \n Configuration\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a27de5b..b6664ce 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -759,6 +759,10 @@ static off_t write_reused_pack(struct sha1file *f)\n \treturn reuse_packfile_offset - sizeof(struct pack_header);\n }\n \n+static const char no_split_warning[] = N_(\n+\"disabling bitmap writing, packs are split due to pack.packSizeLimit\"\n+);\n+\n static void write_pack_file(void)\n {\n \tuint32_t i = 0, j;\n@@ -813,7 +817,10 @@ static void write_pack_file(void)\n \t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n \t\t\t\t\t\t nr_written, sha1, offset);\n \t\t\tclose(fd);\n-\t\t\twrite_bitmap_index = 0;\n+\t\t\tif (write_bitmap_index) {\n+\t\t\t\twarning(_(no_split_warning));\n+\t\t\t\twrite_bitmap_index = 0;\n+\t\t\t}\n \t\t}\n \n \t\tif (!pack_to_stdout) {\n-- \nEW\n"},{"id":"284778","messageId":"20160428080228.GB5252@dcvr.yhbt.net","threadId":"42163","inReplyTo":"20160428022514.GC9707@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: warn on split packs disabling bitmaps","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-28T08:02:28Z","receivedAt":"2016-04-28T08:02:28Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Wed, Apr 27, 2016 at 09:53:24PM +0000, Eric Wong wrote:\n> \n> > It can be tempting for a server admin to want a stable set of\n> > long-lived packs for dumb clients; but also want to enable\n> > bitmaps to serve smart clients more quickly.\n\n> But I did want to mention one thing, which is that long-lived split\n> packs are a tradeoff, even for dumb clients. The pack format cannot do\n> deltas between packs, so the sum of your split packs is larger than a\n> single pack would be. That's a good thing for somebody who cloned\n> earlier, and wants to only a few small packs on top. But it's much worse\n> for somebody who wants to do a fresh clone, and has to grab all of the\n> packs either way.\n\nDefinitely a trade off, but a fresh clone with packs might only\nbe (at worst) doubling or tripling bandwidth use on both sides?\n\nHowever, the CPU/memory cost of packing is at least an order of\nmagnitude (more likely several orders of magnitude) more\nexpensive on the server. The client most likely won't care\nabout CPU/memory usage, though.\n\n> > Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of\n> > our ML archives, soonish. I can serve terabytes of dumb HTTP\n> > traffic all day long without breaking a sweat; but smart\n> > packing of big repos worries me; especially when feeding\n> > slow clients and having to leave processes running\n> > (or buffering pack output to disk). So perhaps I'll teach\n> > my HTTP server play dumb whenever CPU/memory usage is high.\n> \n> Yeah, CPU and memory load for serving large clones is a problem. Memory\n> especially scales with number of objects (because we keep the whole\n> packing list in memory for the entirety of the write). At GitHub, we\n> have some changes to try to serve things verbatim from the on-disk pack\n> without even creating an in-memory list of objects (it's just a bitmap\n> of which objects in the packfile to send), and that reduces CPU and\n> memory load quite a bit. Cleaning up and submitting those patches has\n> been on my todo list for a while, but I just haven't gotten to it. I'm\n> of course happy to share the messy state if you want to pick through it\n> yourself.\n\nSure thing! I can't promise I'll have time, either, but being\nable to serve packs verbatim would be great; especially if you\ncould multiplex it with epoll/kqueue for folks on slow pipes\n(and maybe use sendfile, but perhaps that's not worth the effort\n with TLS everywhere nowadays).\n\nI was also wondering if fresh clones could be memoized entirely.\n"}]}