{"thread":{"id":"22467","subject":"pack.packSizeLimit, safety checks","startedAt":"2010-02-01T09:20:42Z","lastAt":"2010-02-04T02:34:47Z","messageCount":11,"participants":["Sergio","Nicolas Pitre","Johannes Sixt","Shawn O. Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"133236","messageId":"loom.20100201T101056-232@post.gmane.org","threadId":"22467","inReplyTo":null,"subject":"pack.packSizeLimit, safety checks","fromName":"Sergio","fromEmail":"sergio.callegari@gmail.com","sentAt":"2010-02-01T09:20:42Z","receivedAt":"2010-02-01T09:20:42Z","isPatch":false,"sender":{"key":"sergio.callegari@gmail.com","avatar":"https://gravatar.com/avatar/c98f41317e0422c1e630385de0e3970227b8e5ad15f35ba8586066467cc833bc?d=mp&s=160"},"body":"Hi,\n\ndocumentation about pack.packSizeLimit\n\nsays:\n\nThe default maximum size of a pack. This setting only affects packing to a file,\ni.e. the git:// protocol is unaffected. It can be overridden by the\n--max-pack-size option of git-repack(1).\n\nI would suggest clarifying it into\n\nThe default maximum size of a pack in bytes. This setting only affects packing\nto a file, i.e. the git:// protocol is unaffected. It can be overridden by the\n--max-pack-size option of git-repack(1).\n\nSince --max-pack-size takes MB and one might be tempted to assume that the same\nis valid for pack.packSizeLimit.\n\nAlso note that some safety check on pack.packSizeLimit could probably be\ndesirable to avoid an unreasonably small limit. For instance:\n\nAssume that pack.packSizeLimit is set to 1 (believing it would be 1MB, but it is\nin fact 1B). With this at the first git gc every object goes in its own pack.\nYou realize the mistake, you fix pack.packSizeLimit to 1000000, but at this\npoint you cannot go back since git gc cannot run anymore (too many open files).\n\nMaybe, considering all the possible use cases of pack.packSizeLimit could help\nfinding a reasonable lower bound.\n"},{"id":"133258","messageId":"alpine.LFD.2.00.1002011100550.1681@xanadu.home","threadId":"22467","inReplyTo":"loom.20100201T101056-232@post.gmane.org","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-01T16:11:55Z","receivedAt":"2010-02-01T16:11:55Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 1 Feb 2010, Sergio wrote:\n\n> Hi,\n> \n> documentation about pack.packSizeLimit\n> \n> says:\n> \n> The default maximum size of a pack. This setting only affects packing to a file,\n> i.e. the git:// protocol is unaffected. It can be overridden by the\n> --max-pack-size option of git-repack(1).\n> \n> I would suggest clarifying it into\n> \n> The default maximum size of a pack in bytes. This setting only affects packing\n> to a file, i.e. the git:// protocol is unaffected. It can be overridden by the\n> --max-pack-size option of git-repack(1).\n> \n> Since --max-pack-size takes MB and one might be tempted to assume that the same\n> is valid for pack.packSizeLimit.\n\nGrrrrr. This is a terrible discrepency given that all the other \narguments in Git are always byte based, with the optional k/m/g suffix, \nby using git_parse_ulong(). So IMHO I'd just change --max-pack-size to \nbe in line with all the rest and have it accept bytes instead of MB. \nAnd of course I'd push such a change to be included in v1.7.0 along with \nthe other incompatible fixes.\n\nYour suggested precision above is still worth it of course.\n\n> Also note that some safety check on pack.packSizeLimit could probably be\n> desirable to avoid an unreasonably small limit. For instance:\n> \n> Assume that pack.packSizeLimit is set to 1 (believing it would be 1MB, but it is\n> in fact 1B). With this at the first git gc every object goes in its own pack.\n> You realize the mistake, you fix pack.packSizeLimit to 1000000, but at this\n> point you cannot go back since git gc cannot run anymore (too many open files).\n\nThat's a totally orthogonal issue. There are other ways to get into \ntrouble with too many open files and that deserves a fix of its own \n(such as limiting the number of simultaneous opened packs).\n\n\nNicolas\n"},{"id":"133260","messageId":"4B6700CF.1090106@viscovery.net","threadId":"22467","inReplyTo":"alpine.LFD.2.00.1002011100550.1681@xanadu.home","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-01T16:26:55Z","receivedAt":"2010-02-01T16:26:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Pitre schrieb:\n> Grrrrr. This is a terrible discrepency given that all the other \n> arguments in Git are always byte based, with the optional k/m/g suffix, \n> by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to \n> be in line with all the rest and have it accept bytes instead of MB. \n> And of course I'd push such a change to be included in v1.7.0 along with \n> the other incompatible fixes.\n\nWhile at it, also change --big-file-threshold that fast-import learnt the\nother day...\n\n-- Hannes\n"},{"id":"133261","messageId":"20100201162816.GA9394@spearce.org","threadId":"22467","inReplyTo":"4B6700CF.1090106@viscovery.net","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-01T16:28:16Z","receivedAt":"2010-02-01T16:28:16Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt wrote:\n> Nicolas Pitre schrieb:\n> > Grrrrr. This is a terrible discrepency given that all the other \n> > arguments in Git are always byte based, with the optional k/m/g suffix, \n> > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to \n> > be in line with all the rest and have it accept bytes instead of MB. \n> > And of course I'd push such a change to be included in v1.7.0 along with \n> > the other incompatible fixes.\n> \n> While at it, also change --big-file-threshold that fast-import learnt the\n> other day...\n\nYup. WTF was I thinking when I did megabytes as the default unit\non the command line...\n\n-- \nShawn.\n"},{"id":"133265","messageId":"7vvdeg50x4.fsf@alter.siamese.dyndns.org","threadId":"22467","inReplyTo":"alpine.LFD.2.00.1002011100550.1681@xanadu.home","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-01T17:19:19Z","receivedAt":"2010-02-01T17:19:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> Grrrrr. This is a terrible discrepency given that all the other \n> arguments in Git are always byte based, with the optional k/m/g suffix, \n> by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to \n> be in line with all the rest and have it accept bytes instead of MB. \n> And of course I'd push such a change to be included in v1.7.0 along with \n> the other incompatible fixes.\n\nAll of the \"other incompatible\" changes had ample leading time for\ntransition with warnings and all.\n\nI am afraid that doing this \"unit change\" is way too late for 1.7.0, and\nit makes me somewhat unhappy to hear such a suggestion. It belittles all\nthe careful planning that has been done for these other changes to help\nprotect the users from transition pain.\n\nIntroduce --max-pack-megabytes that is a synonym for --max-pack-size for\nnow, and warn when --max-pack-size is used; warn that --max-pack-size will\ncount in bytes in 1.8.0. Ship 1.7.0 with that change. --max-pack-bytes\ncan also be added if you feel like, while at it.\n\nBut changing the unit --max-pack-size counts in to bytes in 1.7.0 feels\na bit too irresponsible for the existing users.\n"},{"id":"133266","messageId":"7vr5p450vk.fsf@alter.siamese.dyndns.org","threadId":"22467","inReplyTo":"20100201162816.GA9394@spearce.org","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-01T17:20:15Z","receivedAt":"2010-02-01T17:20:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Yup. WTF was I thinking when I did megabytes as the default unit\n> on the command line...\n\nThat thing is new, so it is worth fixing before 1.7.0 final.\nPlease make it so.\n\nThanks.\n"},{"id":"133272","messageId":"alpine.LFD.2.00.1002011240510.1681@xanadu.home","threadId":"22467","inReplyTo":"7vvdeg50x4.fsf@alter.siamese.dyndns.org","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-01T18:04:35Z","receivedAt":"2010-02-01T18:04:35Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 1 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > Grrrrr. This is a terrible discrepency given that all the other \n> > arguments in Git are always byte based, with the optional k/m/g suffix, \n> > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to \n> > be in line with all the rest and have it accept bytes instead of MB. \n> > And of course I'd push such a change to be included in v1.7.0 along with \n> > the other incompatible fixes.\n> \n> All of the \"other incompatible\" changes had ample leading time for\n> transition with warnings and all.\n> \n> I am afraid that doing this \"unit change\" is way too late for 1.7.0, and\n> it makes me somewhat unhappy to hear such a suggestion. It belittles all\n> the careful planning that has been done for these other changes to help\n> protect the users from transition pain.\n> \n> Introduce --max-pack-megabytes that is a synonym for --max-pack-size for\n> now, and warn when --max-pack-size is used; warn that --max-pack-size will\n> count in bytes in 1.8.0. Ship 1.7.0 with that change. --max-pack-bytes\n> can also be added if you feel like, while at it.\n> \n> But changing the unit --max-pack-size counts in to bytes in 1.7.0 feels\n> a bit too irresponsible for the existing users.\n\nThing is... I don't know if the --max-pack-size argument is really that \nused. I'd expect people relying on that feature to use the config \nvariable instead, given that 'git gc' has no idea about --max-pack-size \nanyway. People using the --max-pack-size argument directly are probably \ndoing so only to experiment with it, and then setting the config \nvariable, probably using the wrong unit. The fact that such a \ndiscrepancy just came to our attention after all the time this feature \nhas existed is certainly a good indicator of its popularity.\n\nI understand the really unfortunate timing for such a change. OTOH \nthere is a big advantage to bundle as much incompatibilities together at \nthe same time, as people will be prepared for such things already.\n\nWhile I share your concern for advance warning and such, I think those \nconcerns are worth an effort proportional to the depth of user exposure. \nLike for the THREADED_DELTA_SEARCH case, I'm wondering how much pain if \nat all might be saved with a transition plan vs the cost of maintaining \nthat plan and carrying the discrepancy further.\n\n\nNicolas\n"},{"id":"133279","messageId":"7vvdeg23sm.fsf@alter.siamese.dyndns.org","threadId":"22467","inReplyTo":"alpine.LFD.2.00.1002011240510.1681@xanadu.home","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-01T18:45:29Z","receivedAt":"2010-02-01T18:45:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> While I share your concern for advance warning and such, I think those \n> concerns are worth an effort proportional to the depth of user exposure. \n> Like for the THREADED_DELTA_SEARCH case, I'm wondering how much pain if \n> at all might be saved with a transition plan vs the cost of maintaining \n> that plan and carrying the discrepancy further.\n\nAbsolutely. I just wanted to hear that --max-pack-size command line\noption is not widely used and it is Ok to change it.\n"},{"id":"133569","messageId":"7v1vh1zr10.fsf@alter.siamese.dyndns.org","threadId":"22467","inReplyTo":"alpine.LFD.2.00.1002011240510.1681@xanadu.home","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-04T02:14:03Z","receivedAt":"2010-02-04T02:14:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> Thing is... I don't know if the --max-pack-size argument is really that \n> used. I'd expect people relying on that feature to use the config \n> variable instead,...\n\nI suspect one of us need to be careful not to forget this thing...\n\n-- >8 --\nSubject: pack-objects --max-pack-size= counts in bytes\n\nThe --window-memory argument and pack.packsizelimit configuration used by\nthe same program counted in bytes and honored the standard k/m/g suffixes.\nMake this option do the same for consistency.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/RelNotes-1.7.0.txt | 6 ++++++\n Documentation/git-pack-objects.txt | 3 ++-\n builtin-pack-objects.c | 7 +++----\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt\nindex 323ae54..adf8824 100644\n--- a/Documentation/RelNotes-1.7.0.txt\n+++ b/Documentation/RelNotes-1.7.0.txt\n@@ -46,6 +46,12 @@ Notes on behaviour change\n environment, and diff.*.command and diff.*.textconv in the config\n file.\n \n+ * \"git pack-objects --max-pack-size=\" used to count in megabytes,\n+ which was inconsistent with its corresponding configuration\n+ variable and other options the command takes. Now it counts in bytes\n+ and allows standard k/m/g suffixes to be given.\n+\n+\n Updates since v1.6.6\n --------------------\n \ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 097a147..fdaf775 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -106,7 +106,8 @@ base-name::\n \tdefault.\n \n --max-pack-size=::\n-\tMaximum size of each output packfile, expressed in MiB.\n+\tMaximum size of each output packfile, expressed in bytes. The\n+\tsize can be suffixed with \"k\", \"m\", or \"g\".\n \tIf specified, multiple packfiles may be created.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 4a41547..33e11d7 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2203,11 +2203,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\tunsigned long ul = 0;\n+\t\t\tif (!git_parse_ulong(arg + 16, &ul))\n \t\t\t\tusage(pack_usage);\n+\t\t\tpack_size_limit_cfg = ul;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(arg, \"--window=\")) {\n"},{"id":"133574","messageId":"alpine.LFD.2.00.1002032130270.1681@xanadu.home","threadId":"22467","inReplyTo":"7v1vh1zr10.fsf@alter.siamese.dyndns.org","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T02:31:15Z","receivedAt":"2010-02-04T02:31:15Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 3 Feb 2010, Junio C Hamano wrote:\n\n> Nicolas Pitre writes:\n> \n> > Thing is... I don't know if the --max-pack-size argument is really that \n> > used. I'd expect people relying on that feature to use the config \n> > variable instead,...\n> \n> I suspect one of us need to be careful not to forget this thing...\n\nPlease hold on. I think I have a better patch.\n\n\nNicolas\n"},{"id":"133576","messageId":"7vzl3pwwxk.fsf@alter.siamese.dyndns.org","threadId":"22467","inReplyTo":"alpine.LFD.2.00.1002032130270.1681@xanadu.home","subject":"Re: pack.packSizeLimit, safety checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-04T02:34:47Z","receivedAt":"2010-02-04T02:34:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n>> I suspect one of us need to be careful not to forget this thing...\n>\n> Please hold on. I think I have a better patch.\n\nOk, I dropped both fast-import.c mismerge fix and the one you are\nresponding to.\n"}]}