# pack.packSizeLimit, safety checks 11 messages from 2010-02-01 to 2010-02-04. Participants: Sergio, Nicolas Pitre, Johannes Sixt, Shawn O. Pearce, Junio C Hamano. Thread: https://gitlist.dev/t/22467 ## Sergio, 2010-02-01 09:20 Subject: pack.packSizeLimit, safety checks Message-ID: URL: https://gitlist.dev/e/loom.20100201T101056-232%40post.gmane.org ``` Hi, documentation about pack.packSizeLimit says: The default maximum size of a pack. This setting only affects packing to a file, i.e. the git:// protocol is unaffected. It can be overridden by the --max-pack-size option of git-repack(1). I would suggest clarifying it into The default maximum size of a pack in bytes. This setting only affects packing to a file, i.e. the git:// protocol is unaffected. It can be overridden by the --max-pack-size option of git-repack(1). Since --max-pack-size takes MB and one might be tempted to assume that the same is valid for pack.packSizeLimit. Also note that some safety check on pack.packSizeLimit could probably be desirable to avoid an unreasonably small limit. For instance: Assume that pack.packSizeLimit is set to 1 (believing it would be 1MB, but it is in fact 1B). With this at the first git gc every object goes in its own pack. You realize the mistake, you fix pack.packSizeLimit to 1000000, but at this point you cannot go back since git gc cannot run anymore (too many open files). Maybe, considering all the possible use cases of pack.packSizeLimit could help finding a reasonable lower bound. ``` ## Nicolas Pitre, 2010-02-01 16:11 Subject: Re: pack.packSizeLimit, safety checks Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1002011100550.1681%40xanadu.home In-Reply-To: ``` On Mon, 1 Feb 2010, Sergio wrote: > Hi, > > documentation about pack.packSizeLimit > > says: > > The default maximum size of a pack. This setting only affects packing to a file, > i.e. the git:// protocol is unaffected. It can be overridden by the > --max-pack-size option of git-repack(1). > > I would suggest clarifying it into > > The default maximum size of a pack in bytes. This setting only affects packing > to a file, i.e. the git:// protocol is unaffected. It can be overridden by the > --max-pack-size option of git-repack(1). > > Since --max-pack-size takes MB and one might be tempted to assume that the same > is valid for pack.packSizeLimit. Grrrrr. This is a terrible discrepency given that all the other arguments in Git are always byte based, with the optional k/m/g suffix, by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to be in line with all the rest and have it accept bytes instead of MB. And of course I'd push such a change to be included in v1.7.0 along with the other incompatible fixes. Your suggested precision above is still worth it of course. > Also note that some safety check on pack.packSizeLimit could probably be > desirable to avoid an unreasonably small limit. For instance: > > Assume that pack.packSizeLimit is set to 1 (believing it would be 1MB, but it is > in fact 1B). With this at the first git gc every object goes in its own pack. > You realize the mistake, you fix pack.packSizeLimit to 1000000, but at this > point you cannot go back since git gc cannot run anymore (too many open files). That's a totally orthogonal issue. There are other ways to get into trouble with too many open files and that deserves a fix of its own (such as limiting the number of simultaneous opened packs). Nicolas ``` ## Johannes Sixt, 2010-02-01 16:26 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <4B6700CF.1090106@viscovery.net> URL: https://gitlist.dev/e/4B6700CF.1090106%40viscovery.net In-Reply-To: ``` Nicolas Pitre schrieb: > Grrrrr. This is a terrible discrepency given that all the other > arguments in Git are always byte based, with the optional k/m/g suffix, > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to > be in line with all the rest and have it accept bytes instead of MB. > And of course I'd push such a change to be included in v1.7.0 along with > the other incompatible fixes. While at it, also change --big-file-threshold that fast-import learnt the other day... -- Hannes ``` ## Shawn O. Pearce, 2010-02-01 16:28 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <20100201162816.GA9394@spearce.org> URL: https://gitlist.dev/e/20100201162816.GA9394%40spearce.org In-Reply-To: <4B6700CF.1090106@viscovery.net> ``` Johannes Sixt wrote: > Nicolas Pitre schrieb: > > Grrrrr. This is a terrible discrepency given that all the other > > arguments in Git are always byte based, with the optional k/m/g suffix, > > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to > > be in line with all the rest and have it accept bytes instead of MB. > > And of course I'd push such a change to be included in v1.7.0 along with > > the other incompatible fixes. > > While at it, also change --big-file-threshold that fast-import learnt the > other day... Yup. WTF was I thinking when I did megabytes as the default unit on the command line... -- Shawn. ``` ## Junio C Hamano, 2010-02-01 17:19 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <7vvdeg50x4.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdeg50x4.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nicolas Pitre writes: > Grrrrr. This is a terrible discrepency given that all the other > arguments in Git are always byte based, with the optional k/m/g suffix, > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to > be in line with all the rest and have it accept bytes instead of MB. > And of course I'd push such a change to be included in v1.7.0 along with > the other incompatible fixes. All of the "other incompatible" changes had ample leading time for transition with warnings and all. I am afraid that doing this "unit change" is way too late for 1.7.0, and it makes me somewhat unhappy to hear such a suggestion. It belittles all the careful planning that has been done for these other changes to help protect the users from transition pain. Introduce --max-pack-megabytes that is a synonym for --max-pack-size for now, and warn when --max-pack-size is used; warn that --max-pack-size will count in bytes in 1.8.0. Ship 1.7.0 with that change. --max-pack-bytes can also be added if you feel like, while at it. But changing the unit --max-pack-size counts in to bytes in 1.7.0 feels a bit too irresponsible for the existing users. ``` ## Junio C Hamano, 2010-02-01 17:20 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <7vr5p450vk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr5p450vk.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100201162816.GA9394@spearce.org> ``` "Shawn O. Pearce" writes: > Yup. WTF was I thinking when I did megabytes as the default unit > on the command line... That thing is new, so it is worth fixing before 1.7.0 final. Please make it so. Thanks. ``` ## Nicolas Pitre, 2010-02-01 18:04 Subject: Re: pack.packSizeLimit, safety checks Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1002011240510.1681%40xanadu.home In-Reply-To: <7vvdeg50x4.fsf@alter.siamese.dyndns.org> ``` On Mon, 1 Feb 2010, Junio C Hamano wrote: > Nicolas Pitre writes: > > > Grrrrr. This is a terrible discrepency given that all the other > > arguments in Git are always byte based, with the optional k/m/g suffix, > > by using git_parse_ulong(). So IMHO I'd just change --max-pack-size to > > be in line with all the rest and have it accept bytes instead of MB. > > And of course I'd push such a change to be included in v1.7.0 along with > > the other incompatible fixes. > > All of the "other incompatible" changes had ample leading time for > transition with warnings and all. > > I am afraid that doing this "unit change" is way too late for 1.7.0, and > it makes me somewhat unhappy to hear such a suggestion. It belittles all > the careful planning that has been done for these other changes to help > protect the users from transition pain. > > Introduce --max-pack-megabytes that is a synonym for --max-pack-size for > now, and warn when --max-pack-size is used; warn that --max-pack-size will > count in bytes in 1.8.0. Ship 1.7.0 with that change. --max-pack-bytes > can also be added if you feel like, while at it. > > But changing the unit --max-pack-size counts in to bytes in 1.7.0 feels > a bit too irresponsible for the existing users. Thing is... I don't know if the --max-pack-size argument is really that used. I'd expect people relying on that feature to use the config variable instead, given that 'git gc' has no idea about --max-pack-size anyway. People using the --max-pack-size argument directly are probably doing so only to experiment with it, and then setting the config variable, probably using the wrong unit. The fact that such a discrepancy just came to our attention after all the time this feature has existed is certainly a good indicator of its popularity. I understand the really unfortunate timing for such a change. OTOH there is a big advantage to bundle as much incompatibilities together at the same time, as people will be prepared for such things already. While I share your concern for advance warning and such, I think those concerns are worth an effort proportional to the depth of user exposure. Like for the THREADED_DELTA_SEARCH case, I'm wondering how much pain if at all might be saved with a transition plan vs the cost of maintaining that plan and carrying the discrepancy further. Nicolas ``` ## Junio C Hamano, 2010-02-01 18:45 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <7vvdeg23sm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvdeg23sm.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nicolas Pitre writes: > While I share your concern for advance warning and such, I think those > concerns are worth an effort proportional to the depth of user exposure. > Like for the THREADED_DELTA_SEARCH case, I'm wondering how much pain if > at all might be saved with a transition plan vs the cost of maintaining > that plan and carrying the discrepancy further. Absolutely. I just wanted to hear that --max-pack-size command line option is not widely used and it is Ok to change it. ``` ## Junio C Hamano, 2010-02-04 02:14 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <7v1vh1zr10.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vh1zr10.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nicolas Pitre writes: > Thing is... I don't know if the --max-pack-size argument is really that > used. I'd expect people relying on that feature to use the config > variable instead,... I suspect one of us need to be careful not to forget this thing... -- >8 -- Subject: pack-objects --max-pack-size= counts in bytes The --window-memory argument and pack.packsizelimit configuration used by the same program counted in bytes and honored the standard k/m/g suffixes. Make this option do the same for consistency. Signed-off-by: Junio C Hamano --- Documentation/RelNotes-1.7.0.txt | 6 ++++++ Documentation/git-pack-objects.txt | 3 ++- builtin-pack-objects.c | 7 +++---- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt index 323ae54..adf8824 100644 --- a/Documentation/RelNotes-1.7.0.txt +++ b/Documentation/RelNotes-1.7.0.txt @@ -46,6 +46,12 @@ Notes on behaviour change environment, and diff.*.command and diff.*.textconv in the config file. + * "git pack-objects --max-pack-size=" used to count in megabytes, + which was inconsistent with its corresponding configuration + variable and other options the command takes. Now it counts in bytes + and allows standard k/m/g suffixes to be given. + + Updates since v1.6.6 -------------------- diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt index 097a147..fdaf775 100644 --- a/Documentation/git-pack-objects.txt +++ b/Documentation/git-pack-objects.txt @@ -106,7 +106,8 @@ base-name:: default. --max-pack-size=:: - Maximum size of each output packfile, expressed in MiB. + Maximum size of each output packfile, expressed in bytes. The + size can be suffixed with "k", "m", or "g". If specified, multiple packfiles may be created. The default is unlimited, unless the config variable `pack.packSizeLimit` is set. diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 4a41547..33e11d7 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -2203,11 +2203,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) continue; } if (!prefixcmp(arg, "--max-pack-size=")) { - char *end; - pack_size_limit_cfg = 0; - pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024; - if (!arg[16] || *end) + unsigned long ul = 0; + if (!git_parse_ulong(arg + 16, &ul)) usage(pack_usage); + pack_size_limit_cfg = ul; continue; } if (!prefixcmp(arg, "--window=")) { ``` ## Nicolas Pitre, 2010-02-04 02:31 Subject: Re: pack.packSizeLimit, safety checks Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1002032130270.1681%40xanadu.home In-Reply-To: <7v1vh1zr10.fsf@alter.siamese.dyndns.org> ``` On Wed, 3 Feb 2010, Junio C Hamano wrote: > Nicolas Pitre writes: > > > Thing is... I don't know if the --max-pack-size argument is really that > > used. I'd expect people relying on that feature to use the config > > variable instead,... > > I suspect one of us need to be careful not to forget this thing... Please hold on. I think I have a better patch. Nicolas ``` ## Junio C Hamano, 2010-02-04 02:34 Subject: Re: pack.packSizeLimit, safety checks Message-ID: <7vzl3pwwxk.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzl3pwwxk.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Nicolas Pitre writes: >> I suspect one of us need to be careful not to forget this thing... > > Please hold on. I think I have a better patch. Ok, I dropped both fast-import.c mismerge fix and the one you are responding to. ```