{"thread":{"id":"16286","subject":"pack.packSizeLimit and --max-pack-size not working?","startedAt":"2008-11-12T15:12:17Z","lastAt":"2008-11-12T21:42:24Z","messageCount":7,"participants":["Jon Nelson","Nicolas Pitre","Junio C Hamano","Nanako Shiraishi"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"95568","messageId":"cccedfc60811120712o7fcbf648l9f4b8e6f52e50e39@mail.gmail.com","threadId":"16286","inReplyTo":null,"subject":"pack.packSizeLimit and --max-pack-size not working?","fromName":"Jon Nelson","fromEmail":"jnelson@jamponi.net","sentAt":"2008-11-12T15:12:17Z","receivedAt":"2008-11-12T15:12:17Z","isPatch":false,"sender":{"key":"jnelson@jamponi.net","avatar":null},"body":"I'm using 1.6.0.4 and I've found some weird behavior with\npack.packSizeLimit and/or --max-pack-size.\n\nInitially, I thought I could just use pack.packSizeLimit and set it to\n(say) 1 to try to limit the size of individual packfiles to 1MiB or\nless. That does not appear to be working.\n\nIn one case I performed the following set of commands:\n\n# set pack.packSizeLimit to 20\ngit config --global pack.packSizeLimit 20\n\n# verify that it's 20\ngit config --get pack.packSizeLimit # verify it's 20\n\n# run gc --prune\ngit gc --prune\n\n# show the packfiles\n# I find a *single* 65MB packfile, not a series\n# of 20MB (or less) packfiles.\nls -la .git/objects/pack/*.pack\n\n# try repack -ad\ngit repack -ad\n\n# I find a *single* 65MB packfile, not a series\n# of 20MB (or less) packfiles.\nls -la .git/objects/pack/*.pack\n\n\nSo it would appear that the pack.packSizeLimit param\nis just being ignored??\n\nThen I tested using --max-pack-size explicitly. This works, to a degree.\n\ngit repack -ad --max-pack-size 20\n\n# the following shows *4* pack files none larger\n# than (about) 20MB\nls -la .git/objects/pack/*.pack\n\n# try again with 3MB. This also works.\ngit repack -ad --max-pack-size 3\nfind .git/objects/pack -name '*.pack' -size +3M -ls # nothing\n\n# try again with 1MB. This does NOT work.\ngit repack -ad --max-pack-size 1\n\n# here, I find a *single* 65MB pack file again:\nfind .git/objects/pack -name '*.pack' -size +1M -ls\n\nAm I doing something completely wrong with pack.packSizeLimit?\nWhat is going on with --max-pack-size in the 1MB case?\n\n\n-- \nJon\n"},{"id":"95575","messageId":"alpine.LFD.2.00.0811121109420.27509@xanadu.home","threadId":"16286","inReplyTo":"cccedfc60811120712o7fcbf648l9f4b8e6f52e50e39@mail.gmail.com","subject":"[PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-12T16:17:22Z","receivedAt":"2008-11-12T16:17:22Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"First, pack.packSizeLimit and --max-pack-size didn't use the same base\nunit which was confusing.  They both use MiB now.\n\nAlso, if the limit was sufficiently low, having a single object written\ncould bust the limit (by design), but caused the remaining allowed size \nto go negative for subsequent objects, which for an unsigned variable is \na rather huge limit.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\nOn Wed, 12 Nov 2008, Jon Nelson wrote:\n\n> I'm using 1.6.0.4 and I've found some weird behavior with\n> pack.packSizeLimit and/or --max-pack-size.\n> \n> Initially, I thought I could just use pack.packSizeLimit and set it to\n> (say) 1 to try to limit the size of individual packfiles to 1MiB or\n> less. That does not appear to be working.\n> \n> In one case I performed the following set of commands:\n> \n> # set pack.packSizeLimit to 20\n> git config --global pack.packSizeLimit 20\n> \n> # verify that it's 20\n> git config --get pack.packSizeLimit # verify it's 20\n> \n> # run gc --prune\n> git gc --prune\n> \n> # show the packfiles\n> # I find a *single* 65MB packfile, not a series\n> # of 20MB (or less) packfiles.\n> ls -la .git/objects/pack/*.pack\n> \n> # try repack -ad\n> git repack -ad\n> \n> # I find a *single* 65MB packfile, not a series\n> # of 20MB (or less) packfiles.\n> ls -la .git/objects/pack/*.pack\n> \n> \n> So it would appear that the pack.packSizeLimit param\n> is just being ignored??\n> \n> Then I tested using --max-pack-size explicitly. This works, to a degree.\n> \n> git repack -ad --max-pack-size 20\n> \n> # the following shows *4* pack files none larger\n> # than (about) 20MB\n> ls -la .git/objects/pack/*.pack\n> \n> # try again with 3MB. This also works.\n> git repack -ad --max-pack-size 3\n> find .git/objects/pack -name '*.pack' -size +3M -ls # nothing\n> \n> # try again with 1MB. This does NOT work.\n> git repack -ad --max-pack-size 1\n> \n> # here, I find a *single* 65MB pack file again:\n> find .git/objects/pack -name '*.pack' -size +1M -ls\n> \n> Am I doing something completely wrong with pack.packSizeLimit?\n> What is going on with --max-pack-size in the 1MB case?\n\nDoes this fix it for you?\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 32dcd64..e7808b8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1036,9 +1036,9 @@ you can use linkgit:git-index-pack[1] on the *.pack file to regenerate\n the `{asterisk}.idx` file.\n \n pack.packSizeLimit::\n-\tThe default maximum size of a pack.  This setting only affects\n-\tpacking to a file, i.e. the git:// protocol is unaffected.  It\n-\tcan be overridden by the `\\--max-pack-size` option of\n+\tThe default maximum size of a pack, expressed in MiB.  This\n+\tsetting only affects packing to a file, i.e. the git:// protocol is\n+\tunaffected. It can be overridden by the `\\--max-pack-size` option of\n \tlinkgit:git-repack[1].\n \n pager.<cmd>::\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 0c4649c..fdee9c6 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -245,8 +245,12 @@ static unsigned long write_object(struct sha1file *f,\n \ttype = entry->type;\n \n \t/* write limit if limited packsize and not first object */\n-\tlimit = pack_size_limit && nr_written ?\n-\t\t\tpack_size_limit - write_offset : 0;\n+\tif (!pack_size_limit || !nr_written)\n+\t\tlimit = 0;\n+\telse if (pack_size_limit <= write_offset)\n+\t\tlimit = 1;\n+\telse\n+\t\tlimit = pack_size_limit - write_offset;\n \n \tif (!entry->delta)\n \t\tusable_delta = 0;\t/* no delta */\n@@ -1844,7 +1848,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.packsizelimit\")) {\n-\t\tpack_size_limit_cfg = git_config_ulong(k, v);\n+\t\tpack_size_limit_cfg = git_config_ulong(k, v) * 1024 * 1024;\n \t\treturn 0;\n \t}\n \treturn git_default_config(k, v, cb);\n"},{"id":"95595","messageId":"7vk5b8q1l4.fsf@gitster.siamese.dyndns.org","threadId":"16286","inReplyTo":"alpine.LFD.2.00.0811121109420.27509@xanadu.home","subject":"Re: [PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-12T17:46:31Z","receivedAt":"2008-11-12T17:46:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> First, pack.packSizeLimit and --max-pack-size didn't use the same base\n> unit which was confusing.  They both use MiB now.\n>\n> Also, if the limit was sufficiently low, having a single object written\n> could bust the limit (by design), but caused the remaining allowed size \n> to go negative for subsequent objects, which for an unsigned variable is \n> a rather huge limit.\n>\n> Signed-off-by: Nicolas Pitre <nico@cam.org>\n> ---\n\n> @@ -1844,7 +1848,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n>  \t\treturn 0;\n>  \t}\n>  \tif (!strcmp(k, \"pack.packsizelimit\")) {\n> -\t\tpack_size_limit_cfg = git_config_ulong(k, v);\n> +\t\tpack_size_limit_cfg = git_config_ulong(k, v) * 1024 * 1024;\n\nThe fix to tweak the limit for subsequent split pack is a good thing to\nhave, but this change would break existing repositories where people\nspecified 20971520 (or worse yet \"20m\") to limit the size to 20MB.\n\nI think --max-pack-size is what should be fixed to use git_parse_ulong()\nto match the configuration, if you find the discrepancy disturbing.\n"},{"id":"95599","messageId":"cccedfc60811120958u3e1625e4y4ffee1d073102ab8@mail.gmail.com","threadId":"16286","inReplyTo":"alpine.LFD.2.00.0811121109420.27509@xanadu.home","subject":"Re: [PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Jon Nelson","fromEmail":"jnelson@jamponi.net","sentAt":"2008-11-12T17:58:29Z","receivedAt":"2008-11-12T17:58:29Z","isPatch":true,"sender":{"key":"jnelson@jamponi.net","avatar":null},"body":"On Wed, Nov 12, 2008 at 10:17 AM, Nicolas Pitre <nico@cam.org> wrote:\n> First, pack.packSizeLimit and --max-pack-size didn't use the same base\n> unit which was confusing.  They both use MiB now.\n>\n> Also, if the limit was sufficiently low, having a single object written\n> could bust the limit (by design), but caused the remaining allowed size\n> to go negative for subsequent objects, which for an unsigned variable is\n> a rather huge limit.\n>\n> Signed-off-by: Nicolas Pitre <nico@cam.org>\n\nThe patch does appear to resolve the issue!\n\n-- \nJon\n"},{"id":"95612","messageId":"alpine.LFD.2.00.0811121255330.27509@xanadu.home","threadId":"16286","inReplyTo":"7vk5b8q1l4.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-12T18:23:58Z","receivedAt":"2008-11-12T18:23:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"First, pack.packSizeLimit and --max-pack-size didn't use the same base\nunit which was confusing.  They both use suffixable value arguments now.\n\nAlso, if the limit was sufficiently low, having a single object written\ncould bust the limit (by design), but caused the remaining allowed size\nto go negative for subsequent objects, which for an unsigned variable is\na rather huge limit.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\nOn Wed, 12 Nov 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > First, pack.packSizeLimit and --max-pack-size didn't use the same base\n> > unit which was confusing.  They both use MiB now.\n> >\n> > Also, if the limit was sufficiently low, having a single object written\n> > could bust the limit (by design), but caused the remaining allowed size \n> > to go negative for subsequent objects, which for an unsigned variable is \n> > a rather huge limit.\n> >\n> > Signed-off-by: Nicolas Pitre <nico@cam.org>\n> > ---\n> \n> > @@ -1844,7 +1848,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n> >  \t\treturn 0;\n> >  \t}\n> >  \tif (!strcmp(k, \"pack.packsizelimit\")) {\n> > -\t\tpack_size_limit_cfg = git_config_ulong(k, v);\n> > +\t\tpack_size_limit_cfg = git_config_ulong(k, v) * 1024 * 1024;\n> \n> The fix to tweak the limit for subsequent split pack is a good thing to\n> have, but this change would break existing repositories where people\n> specified 20971520 (or worse yet \"20m\") to limit the size to 20MB.\n> \n> I think --max-pack-size is what should be fixed to use git_parse_ulong()\n> to match the configuration, if you find the discrepancy disturbing.\n\nFair enough.\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 1c4fb43..d60409a 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -104,10 +104,11 @@ base-name::\n \tdefault.\n \n --max-pack-size=<n>::\n-\tMaximum size of each output packfile, expressed in MiB.\n+\tMaximum size of each output packfile.\n \tIf specified,  multiple packfiles may be created.\n \tThe default is unlimited, unless the config variable\n-\t`pack.packSizeLimit` is set.\n+\t`pack.packSizeLimit` is set. The size can be suffixed with\n+\t\"k\", \"m\", or \"g\".\n \n --incremental::\n \tThis flag causes an object already in a pack ignored\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 0c4649c..9c4333b 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -75,7 +75,7 @@ static int allow_ofs_delta;\n static const char *base_name;\n static int progress = 1;\n static int window = 10;\n-static uint32_t pack_size_limit, pack_size_limit_cfg;\n+static unsigned long pack_size_limit, pack_size_limit_cfg;\n static int depth = 50;\n static int delta_search_threads = 1;\n static int pack_to_stdout;\n@@ -245,8 +245,12 @@ static unsigned long write_object(struct sha1file *f,\n \ttype = entry->type;\n \n \t/* write limit if limited packsize and not first object */\n-\tlimit = pack_size_limit && nr_written ?\n-\t\t\tpack_size_limit - write_offset : 0;\n+\tif (!pack_size_limit || !nr_written)\n+\t\tlimit = 0;\n+\telse if (pack_size_limit <= write_offset)\n+\t\tlimit = 1;\n+\telse\n+\t\tlimit = pack_size_limit - write_offset;\n \n \tif (!entry->delta)\n \t\tusable_delta = 0;\t/* no delta */\n@@ -2103,11 +2107,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(arg, \"--max-pack-size=\")) {\n-\t\t\tchar *end;\n-\t\t\tpack_size_limit_cfg = 0;\n-\t\t\tpack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n-\t\t\tif (!arg[16] || *end)\n+\t\t\tif (!git_parse_ulong(arg+16, &pack_size_limit))\n \t\t\t\tusage(pack_usage);\n+\t\t\telse\n+\t\t\t\tpack_size_limit_cfg = 0;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(arg, \"--window=\")) {\n"},{"id":"95627","messageId":"7vmyg4myf6.fsf@gitster.siamese.dyndns.org","threadId":"16286","inReplyTo":"alpine.LFD.2.00.0811121255330.27509@xanadu.home","subject":"Re: [PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-12T21:23:09Z","receivedAt":"2008-11-12T21:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> @@ -2103,11 +2107,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t\t\tcontinue;\n>  \t\t}\n>  \t\tif (!prefixcmp(arg, \"--max-pack-size=\")) {\n> -\t\t\tchar *end;\n> -\t\t\tpack_size_limit_cfg = 0;\n> -\t\t\tpack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n> -\t\t\tif (!arg[16] || *end)\n> +\t\t\tif (!git_parse_ulong(arg+16, &pack_size_limit))\n>  \t\t\t\tusage(pack_usage);\n> +\t\t\telse\n> +\t\t\t\tpack_size_limit_cfg = 0;\n>  \t\t\tcontinue;\n>  \t\t}\n>  \t\tif (!prefixcmp(arg, \"--window=\")) {\n\nHmm, an unrelated funniness here is why the code even needs to futz with\nthe value read from the configuration.  Later in the code we have:\n\n\tif (!pack_to_stdout && !pack_size_limit)\n\t\tpack_size_limit = pack_size_limit_cfg;\n\nand the intent seems to be:\n\n - if there is nothing on the command line, use config;\n - if there is something on the command line, ignore config.\n\nBut if you have a configured limit and would want to override from the\ncommand line, this won't work.\n\nI will apply the wraparound avoidance as a separate \"pure fix\" patch to\n'maint' first.\n\nBesides, --max-pack-size has been defined as megabytes for the entirety of\nits existence, and I am a bit worried about changing the semantics at this\npoint without any warning.  I realize that I am worried too much for\npeople with a script that give an explicit --max-pack-size=1 to obtain a\nset of split packs that would fit on floppies, who probably would not\nexist ;-)\n"},{"id":"95629","messageId":"20081113064224.6117@nanako3.lavabit.com","threadId":"16286","inReplyTo":"7vk5b8q1l4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fix pack.packSizeLimit and --max-pack-size handling","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2008-11-12T21:42:24Z","receivedAt":"2008-11-12T21:42:24Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano <gitster@pobox.com>:\n\n> I think --max-pack-size is what should be fixed to use git_parse_ulong()\n> to match the configuration, if you find the discrepancy disturbing.\n\nBut doesn't that break existing scripts and habits?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"}]}