Re: [PATCH] fast-import: Stream very large blobs directly to pack

"Shawn O. Pearce" [off-list ref] writes:

Well, well, well....

The documentation says this is counted in bytes, but somehow neither of us
found the above " * 1024 * 1024" suspicious.

Shouldn't it be at least like this?  It would probably be a good idea to
use git_parse_ulong() or somesuch while we are at it.

 fast-import.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index ca21082..ea1ac0f 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2800,7 +2800,7 @@ static int parse_one_option(const char *option)
 	if (!prefixcmp(option, "max-pack-size=")) {
 		option_max_pack_size(option + 14);
 	} else if (!prefixcmp(option, "big-file-threshold=")) {
-		big_file_threshold = strtoumax(option + 19, NULL, 0) * 1024 * 1024;
+		big_file_threshold = strtoumax(option + 19, NULL, 0);
 	} else if (!prefixcmp(option, "depth=")) {
 		option_depth(option + 6);
 	} else if (!prefixcmp(option, "active-branches=")) {

Junio C Hamano [off-list ref] wrote:

In my v3 patch I thought I replaced this code with:

+               else if (!prefixcmp(a, "--big-file-threshold=")) {
+                       unsigned long v;
+                       if (!git_parse_ulong(a + 21, &v))
+                               usage(fast_import_usage);
+                       big_file_threshold = v;

So we relied on git_parse_ulong to handle unit suffixes as well.

-- 
Shawn.

On Wed, 3 Feb 2010, Junio C Hamano wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

The strtoumax call got messed up.  Squash this into your merge:

diff --git a/fast-import.c b/fast-import.c
index e6ebcf6..9c65a24 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2800,7 +2800,7 @@ static int parse_one_option(const char *option)
 	if (!prefixcmp(option, "max-pack-size=")) {
 		option_max_pack_size(option + 14);
 	} else if (!prefixcmp(option, "big-file-threshold=")) {
-		big_file_threshold = strtoumax(option + 21, NULL, 0) * 1024 * 1024;
+		big_file_threshold = strtoumax(option + 19, NULL, 0) * 1024 * 1024;
 	} else if (!prefixcmp(option, "depth=")) {
 		option_depth(option + 6);
 	} else if (!prefixcmp(option, "active-branches=")) {
 

quoted

You may want to add the new option to the output from "cmd -h" and
probably description of the configuration in the doc before any of this
gets official.

I'll send an additional patch in a minute with these documentation
related updates.

Well, well, well....

The documentation says this is counted in bytes, but somehow neither of us
found the above " * 1024 * 1024" suspicious.

Shouldn't it be at least like this?  It would probably be a good idea to
use git_parse_ulong() or somesuch while we are at it.

Yes, definitely.  I'm about to post a patch moving --max-pack-size in 
that direction too.  I just had to fix a couple other unsuspected issues 
to get there though.  Patches will follow shortly.


Nicolas