git repack --max-pack-size broken in git-next

git repack --max-pack-size broken in git-next

[Siddharth Agarwal]

With git-next, the --max-pack-size option to git repack doesn't work.

With git at b139ac2, `git repack --max-pack-size=1g` says

error: option `max-pack-size' expects a numerical value

while `git repack --max-pack-size=1073741824` says

error: unknown option `max_pack_size=1073741824'

I bisected this down to a1bbc6c, which rewrote git repack in C.

Re: git repack --max-pack-size broken in git-next

[Junio C Hamano]

Siddharth Agarwal <sid0@fb.com> writes:

> With git-next, the --max-pack-size option to git repack doesn't work.
>
> With git at b139ac2, `git repack --max-pack-size=1g` says
>
> error: option `max-pack-size' expects a numerical value

Thanks, Siddharth.

It seems that we have a hand-crafted parser outside parse-options
framework in pack-objects, and the scripted git-repack used to pass
this option without interpreting itself.

We probably should lift the OPT_ULONG() implementation out of
builtin/pack-objects.c and move it to parse-options.[ch] and use it
in the reimplementation of repack.

And probably use it in other places where these "integers in
human-friendly units" may make sense, but that is a separate topic.

Re: git repack --max-pack-size broken in git-next

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Siddharth Agarwal <sid0@fb.com> writes:
>
>> With git-next, the --max-pack-size option to git repack doesn't work.
>>
>> With git at b139ac2, `git repack --max-pack-size=1g` says
>>
>> error: option `max-pack-size' expects a numerical value
>
> Thanks, Siddharth.
>
> It seems that we have a hand-crafted parser outside parse-options
> framework in pack-objects, and the scripted git-repack used to pass
> this option without interpreting itself.
>
> We probably should lift the OPT_ULONG() implementation out of
> builtin/pack-objects.c and move it to parse-options.[ch] and use it
> in the reimplementation of repack.
>
> And probably use it in other places where these "integers in
> human-friendly units" may make sense, but that is a separate topic.

The first step may look something like this (totally untested)..

 builtin/pack-objects.c | 22 +++-------------------
 parse-options.c        | 17 +++++++++++++++++
 parse-options.h        |  3 +++
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 541667f..b264f1f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2416,23 +2416,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_ulong(const struct option *opt,
-			      const char *arg, int unset)
-{
-	if (unset)
-		die(_("option %s does not accept negative form"),
-		    opt->long_name);
-
-	if (!git_parse_ulong(arg, opt->value))
-		die(_("unable to parse value '%s' for option %s"),
-		    arg, opt->long_name);
-	return 0;
-}
-
-#define OPT_ULONG(s, l, v, h) \
-	{ OPTION_CALLBACK, (s), (l), (v), "n", (h),	\
-	  PARSE_OPT_NONEG, option_parse_ulong }
-
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
@@ -2462,8 +2445,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 N_("ignore packed objects")),
 		OPT_INTEGER(0, "window", &window,
 			    N_("limit pack window by objects")),
-		OPT_ULONG(0, "window-memory", &window_memory_limit,
-			  N_("limit pack window by memory in addition to object limit")),
+		{ OPTION_ULONG, 0, "window-memory", &window_memory_limit, N_("n"),
+		  N_("limit pack window by memory in addition to object limit"),
+		  PARSE_OPT_HUMAN_NUMBER },
 		OPT_INTEGER(0, "depth", &depth,
 			    N_("maximum length of delta chain allowed in the resulting pack")),
 		OPT_BOOL(0, "reuse-delta", &reuse_delta,
diff --git a/parse-options.c b/parse-options.c
index 7b8d3fa..3a47538 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p,
 			return opterror(opt, "expects a numerical value", flags);
 		return 0;
 
+	case OPTION_ULONG:
+		if (unset) {
+			*(unsigned long *)opt->value = 0;
+		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
+			*(unsigned long *)opt->value = opt->defval;
+		} else if (get_arg(p, opt, flags, &arg)) {
+			return -1;
+		} else if (opt->flags & PARSE_OPT_HUMAN_NUMBER) {
+			if (!git_parse_ulong(arg, (unsigned long *)opt->value))
+				return opterror(opt, "expects a numerical value", flags);
+		} else {
+			*(unsigned long *)opt->value = strtoul(arg, (char **)&s, 10);
+			if (*s)
+				return opterror(opt, "expects a numerical value", flags);
+		}
+		return 0;
+
 	default:
 		die("should not happen, someone must be hit on the forehead");
 	}
diff --git a/parse-options.h b/parse-options.h
index d670cb9..6a3cce0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -17,6 +17,7 @@ enum parse_opt_type {
 	/* options with arguments (usually) */
 	OPTION_STRING,
 	OPTION_INTEGER,
+	OPTION_ULONG,
 	OPTION_CALLBACK,
 	OPTION_LOWLEVEL_CALLBACK,
 	OPTION_FILENAME
@@ -38,6 +39,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
+	PARSE_OPT_HUMAN_NUMBER = 128,
 	PARSE_OPT_SHELL_EVAL = 256
 };
 
@@ -133,6 +135,7 @@ struct option {
 #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
+#define OPT_ULONG(s, l, v, h)       { OPTION_ULONG, (s), (l), (v), N_("n"), (h) }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \

[PATCH v2 0/2] Fix "repack --window-memory=4g" regression in 1.8.4

[Junio C Hamano]

The command line parser was broken when the command was
reimplemented in C in two ways.  It incorrectly limited the value
range of window-memory and max-pack-size to "int", and also stopped
grokking the unit suffixes like 2g/400m/8k.

These two patches apply on top of 35c14176 (Reword repack
documentation to no longer state it's a script, 2013-10-19) and
later can be merged down to maint-1.8.4 track and upwards.

Junio C Hamano (2):
  parse-options: refactor human-friendly-integer parser out of pack-objects
  repack: accept larger window-memory and max-pack-size

 builtin/pack-objects.c | 25 ++++---------------------
 builtin/repack.c       | 12 ++++++------
 parse-options.c        | 17 +++++++++++++++++
 parse-options.h        |  5 +++++
 4 files changed, 32 insertions(+), 27 deletions(-)

-- 
1.9-rc0-151-ga5225c0

[PATCH v2 1/2] parse-options: refactor human-friendly-integer parser out of pack-objects

[Junio C Hamano]

We only had code to understand unit suffixes such as g/m/k (as in
2g/400m/8k) in the configuration parser but not in the command line
option parser.  "git pack-objects" worked it around by having a
custom extension to the parse-options API; make it available to
other callers.

The new macro is not called OPT_ULONG() but OPT_HUM_ULONG(), as it
would be bizzarre to have ULONG that understands human friendly
units while INTEGER that does not.  It is not named with a shorter
"OPT_HULONG", primarily to avoid having to name a future parallel
for parsing human friendly integer "OPT_HINT".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pack-objects.c | 25 ++++---------------------
 parse-options.c        | 17 +++++++++++++++++
 parse-options.h        |  5 +++++
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..2fa8e1e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2417,23 +2417,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_ulong(const struct option *opt,
-			      const char *arg, int unset)
-{
-	if (unset)
-		die(_("option %s does not accept negative form"),
-		    opt->long_name);
-
-	if (!git_parse_ulong(arg, opt->value))
-		die(_("unable to parse value '%s' for option %s"),
-		    arg, opt->long_name);
-	return 0;
-}
-
-#define OPT_ULONG(s, l, v, h) \
-	{ OPTION_CALLBACK, (s), (l), (v), "n", (h),	\
-	  PARSE_OPT_NONEG, option_parse_ulong }
-
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
@@ -2455,16 +2438,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "index-version", NULL, N_("version[,offset]"),
 		  N_("write the pack index file in the specified idx format version"),
 		  0, option_parse_index_version },
-		OPT_ULONG(0, "max-pack-size", &pack_size_limit,
-			  N_("maximum size of each output pack file")),
+		OPT_HUM_ULONG(0, "max-pack-size", &pack_size_limit,
+			      N_("maximum size of each output pack file")),
 		OPT_BOOL(0, "local", &local,
 			 N_("ignore borrowed objects from alternate object store")),
 		OPT_BOOL(0, "incremental", &incremental,
 			 N_("ignore packed objects")),
 		OPT_INTEGER(0, "window", &window,
 			    N_("limit pack window by objects")),
-		OPT_ULONG(0, "window-memory", &window_memory_limit,
-			  N_("limit pack window by memory in addition to object limit")),
+		OPT_HUM_ULONG(0, "window-memory", &window_memory_limit,
+		      N_("limit pack window by memory in addition to object limit")),
 		OPT_INTEGER(0, "depth", &depth,
 			    N_("maximum length of delta chain allowed in the resulting pack")),
 		OPT_BOOL(0, "reuse-delta", &reuse_delta,
diff --git a/parse-options.c b/parse-options.c
index c2cbca2..948ade7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -136,6 +136,23 @@ static int get_value(struct parse_opt_ctx_t *p,
 			return opterror(opt, "expects a numerical value", flags);
 		return 0;
 
+	case OPTION_ULONG:
+		if (unset) {
+			*(unsigned long *)opt->value = 0;
+		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
+			*(unsigned long *)opt->value = opt->defval;
+		} else if (get_arg(p, opt, flags, &arg)) {
+			return -1;
+		} else if (opt->flags & PARSE_OPT_HUMAN_NUMBER) {
+			if (!git_parse_ulong(arg, (unsigned long *)opt->value))
+				return opterror(opt, "expects a numerical value", flags);
+		} else {
+			*(unsigned long *)opt->value = strtoul(arg, (char **)&s, 10);
+			if (*s)
+				return opterror(opt, "expects a numerical value", flags);
+		}
+		return 0;
+
 	default:
 		die("should not happen, someone must be hit on the forehead");
 	}
diff --git a/parse-options.h b/parse-options.h
index 9b94596..d65ecdb 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -16,6 +16,7 @@ enum parse_opt_type {
 	/* options with arguments (usually) */
 	OPTION_STRING,
 	OPTION_INTEGER,
+	OPTION_ULONG,
 	OPTION_CALLBACK,
 	OPTION_LOWLEVEL_CALLBACK,
 	OPTION_FILENAME
@@ -40,6 +41,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 16,
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
+	PARSE_OPT_HUMAN_NUMBER = 128,
 	PARSE_OPT_SHELL_EVAL = 256
 };
 
@@ -131,6 +133,9 @@ struct option {
 #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (p) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h) }
+#define OPT_ULONG(s, l, v, h)       { OPTION_ULONG, (s), (l), (v), N_("n"), (h) }
+#define OPT_HUM_ULONG(s, l, v, h)   { OPTION_ULONG, (s), (l), (v), N_("n"), (h), \
+				      PARSE_OPT_HUMAN_NUMBER }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \
-- 
1.9-rc0-151-ga5225c0

[PATCH v2 2/2] repack: accept larger window-memory and max-pack-size

[Junio C Hamano]

These quantities can be larger than an int.  Use ulong to express
them like the underlying pack-objects does, and also parse them with
the human-friendly unit suffixes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/repack.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index a0ff5c7..8ce396b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -130,9 +130,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	int pack_everything = 0;
 	int delete_redundant = 0;
 	char *unpack_unreachable = NULL;
-	int window = 0, window_memory = 0;
+	int window = 0;
 	int depth = 0;
-	int max_pack_size = 0;
+	unsigned long max_pack_size = 0, window_memory = 0;
 	int no_reuse_delta = 0, no_reuse_object = 0;
 	int no_update_server_info = 0;
 	int quiet = 0;
@@ -159,11 +159,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("with -A, do not loosen objects older than this")),
 		OPT_INTEGER(0, "window", &window,
 				N_("size of the window used for delta compression")),
-		OPT_INTEGER(0, "window-memory", &window_memory,
+		OPT_HUM_ULONG(0, "window-memory", &window_memory,
 				N_("same as the above, but limit memory size instead of entries count")),
 		OPT_INTEGER(0, "depth", &depth,
 				N_("limits the maximum delta depth")),
-		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+		OPT_HUM_ULONG(0, "max-pack-size", &max_pack_size,
 				N_("maximum size of each packfile")),
 		OPT_END()
 	};
@@ -187,11 +187,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (window)
 		argv_array_pushf(&cmd_args, "--window=%u", window);
 	if (window_memory)
-		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
+		argv_array_pushf(&cmd_args, "--window-memory=%lu", window_memory);
 	if (depth)
 		argv_array_pushf(&cmd_args, "--depth=%u", depth);
 	if (max_pack_size)
-		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
+		argv_array_pushf(&cmd_args, "--max_pack_size=%lu", max_pack_size);
 	if (no_reuse_delta)
 		argv_array_pushf(&cmd_args, "--no-reuse-delta");
 	if (no_reuse_object)
-- 
1.9-rc0-151-ga5225c0

Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size

[Jeff King]

On Wed, Jan 22, 2014 at 11:58:05AM -0800, Junio C Hamano wrote:

> These quantities can be larger than an int.  Use ulong to express
> them like the underlying pack-objects does, and also parse them with
> the human-friendly unit suffixes.

Hrm. I think that is a valid strategy, but...

> -	int max_pack_size = 0;
> +	unsigned long max_pack_size = 0, window_memory = 0;

Here we must use the correct C type...

> -		OPT_INTEGER(0, "window-memory", &window_memory,
> +		OPT_HUM_ULONG(0, "window-memory", &window_memory,

And here use the correct parser...

>  	if (window_memory)
> -		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
> +		argv_array_pushf(&cmd_args, "--window-memory=%lu", window_memory);

And here use the correct format string...

All of which must match what pack-objects does, or we risk a further
break (though I do not guess it will change from ulong anytime soon).
The original shell version worked because they were all strings. We do
not care about the numeric value here, and are just forwarding the value
along to pack-objects. Why not just use a string?

The only advantage I can think of is that this gives us slightly earlier
error detection for "git repack --window-memory=bogosity".

But I think there is a subtle problem. Here (and elsewhere) we use the
parsed value of "0" as a sentinel. I think that is OK for
--max-pack-size, where "0" is not a reasonable value. But git-repack(1)
says:

  --window-memory=0 makes memory usage unlimited, which is the default.

What does:

  git config pack.windowMemory 256m
  git repack --window-memory=0

do? It should override the config, but I think it does not with your
patch (nor with the current code). Using a string would fix that (though
you could also fix it by using a different sentinel, like ULONG_MAX).

>  	if (max_pack_size)
> -		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
> +		argv_array_pushf(&cmd_args, "--max_pack_size=%lu", max_pack_size);

These underscores are interesting:

  $ git pack-objects --max_pack_size=256m
  error: unknown option `max_pack_size=256m'

I get the feeling the test suite does not cover this feature very well.
:)

-Peff

Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size

[Jeff King]

On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote:

> But I think there is a subtle problem. Here (and elsewhere) we use the
> parsed value of "0" as a sentinel. I think that is OK for
> --max-pack-size, where "0" is not a reasonable value. But git-repack(1)
> says:
> 
>   --window-memory=0 makes memory usage unlimited, which is the default.
> 
> What does:
> 
>   git config pack.windowMemory 256m
>   git repack --window-memory=0
> 
> do? It should override the config, but I think it does not with your
> patch (nor with the current code). Using a string would fix that (though
> you could also fix it by using a different sentinel, like ULONG_MAX).

Here is a series that does that (and fixes the other issue I found). It
would probably be nice to test these things, but checking that they
actually had an impact is tricky (how do you know that --window-memory
did the right thing?).

  [1/3]: repack: fix typo in max-pack-size option
  [2/3]: repack: make parsed string options const-correct
  [3/3]: repack: propagate pack-objects options as strings

-Peff

[PATCH 1/3] repack: fix typo in max-pack-size option

[Jeff King]

When we see "--max-pack-size", we accidentally propagated
this to pack-objects as "--max_pack_size", which does not
work at all.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index ba66c6e..528725b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (depth)
 		argv_array_pushf(&cmd_args, "--depth=%u", depth);
 	if (max_pack_size)
-		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
+		argv_array_pushf(&cmd_args, "--max-pack-size=%u", max_pack_size);
 	if (no_reuse_delta)
 		argv_array_pushf(&cmd_args, "--no-reuse-delta");
 	if (no_reuse_object)
-- 
1.8.5.2.500.g8060133

[PATCH 2/3] repack: make parsed string options const-correct

[Jeff King]

When we use OPT_STRING to parse an option, we get back a
pointer into the argv array, which should be "const char *".
The compiler doesn't notice because it gets passed through a
"void *" in the option struct.

Signed-off-by: Jeff King <peff@peff.net>
---
Not a big deal, but just for consistency with the next patch.

 builtin/repack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 528725b..7f89c7c 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -129,7 +129,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	/* variables to be filled by option parsing */
 	int pack_everything = 0;
 	int delete_redundant = 0;
-	char *unpack_unreachable = NULL;
+	const char *unpack_unreachable = NULL;
 	int window = 0, window_memory = 0;
 	int depth = 0;
 	int max_pack_size = 0;
-- 
1.8.5.2.500.g8060133

[PATCH 3/3] repack: propagate pack-objects options as strings

[Jeff King]

In the original shell version of git-repack, any options
destined for pack-objects were left as strings, and passed
as a whole. Since the C rewrite in commit a1bbc6c (repack:
rewrite the shell script in C, 2013-09-15), we now parse
these values to integers internally, then reformat the
integers when passing the option to pack-objects.

This has the advantage that we catch format errors earlier
(i.e., when repack is invoked, rather than when pack-objects
is invoked).

It has three disadvantages, though:

  1. Our internal data types may not be the right size. In
     the case of "--window-memory" and "--max-pack-size",
     these are "unsigned long" in pack-objects, but we can
     only represent a regular "int".

  2. Our parsing routines might not be the same as those of
     pack-objects. For the two options above, pack-objects
     understands "100m" to mean "100 megabytes", but repack
     does not.

  3. We have to keep a sentinel value to know whether it is
     worth passing the option along. In the case of
     "--window-memory", we currently do not pass it if the
     value is "0". But that is a meaningful value to
     pack-objects, where it overrides any configured value.

We can fix all of these by simply passing the strings from
the user along to pack-objects verbatim. This does not
actually fix anything for "--depth" or "--window", but these
are converted, too, for consistency.

Signed-off-by: Jeff King <peff@peff.net>
---
So we lose the advantage listed above. But I think the simplicity and
future-proofing is worth it (and in this case, we basically don't do
anything _except_ invoke pack-objects, so it is not like we do a bunch
of early work that has to be undone when we find out that the option is
bogus later on).

 builtin/repack.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 7f89c7c..6284846 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -130,9 +130,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	int pack_everything = 0;
 	int delete_redundant = 0;
 	const char *unpack_unreachable = NULL;
-	int window = 0, window_memory = 0;
-	int depth = 0;
-	int max_pack_size = 0;
+	const char *window = NULL, *window_memory = NULL;
+	const char *depth = NULL;
+	const char *max_pack_size = NULL;
 	int no_reuse_delta = 0, no_reuse_object = 0;
 	int no_update_server_info = 0;
 	int quiet = 0;
@@ -157,13 +157,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				N_("pass --local to git-pack-objects")),
 		OPT_STRING(0, "unpack-unreachable", &unpack_unreachable, N_("approxidate"),
 				N_("with -A, do not loosen objects older than this")),
-		OPT_INTEGER(0, "window", &window,
+		OPT_STRING(0, "window", &window, N_("n"),
 				N_("size of the window used for delta compression")),
-		OPT_INTEGER(0, "window-memory", &window_memory,
+		OPT_STRING(0, "window-memory", &window_memory, N_("bytes"),
 				N_("same as the above, but limit memory size instead of entries count")),
-		OPT_INTEGER(0, "depth", &depth,
+		OPT_STRING(0, "depth", &depth, N_("n"),
 				N_("limits the maximum delta depth")),
-		OPT_INTEGER(0, "max-pack-size", &max_pack_size,
+		OPT_STRING(0, "max-pack-size", &max_pack_size, N_("bytes"),
 				N_("maximum size of each packfile")),
 		OPT_END()
 	};
@@ -185,13 +185,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	argv_array_push(&cmd_args, "--all");
 	argv_array_push(&cmd_args, "--reflog");
 	if (window)
-		argv_array_pushf(&cmd_args, "--window=%u", window);
+		argv_array_pushf(&cmd_args, "--window=%s", window);
 	if (window_memory)
-		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
+		argv_array_pushf(&cmd_args, "--window-memory=%s", window_memory);
 	if (depth)
-		argv_array_pushf(&cmd_args, "--depth=%u", depth);
+		argv_array_pushf(&cmd_args, "--depth=%s", depth);
 	if (max_pack_size)
-		argv_array_pushf(&cmd_args, "--max-pack-size=%u", max_pack_size);
+		argv_array_pushf(&cmd_args, "--max-pack-size=%s", max_pack_size);
 	if (no_reuse_delta)
 		argv_array_pushf(&cmd_args, "--no-reuse-delta");
 	if (no_reuse_object)
-- 
1.8.5.2.500.g8060133

Re: [PATCH 3/3] repack: propagate pack-objects options as strings

[Jeff King]

On Wed, Jan 22, 2014 at 08:30:13PM -0500, Jeff King wrote:

> -		OPT_INTEGER(0, "window", &window,
> +		OPT_STRING(0, "window", &window, N_("n"),
>  				N_("size of the window used for delta compression")),

By the way, the old code with OPT_INTEGER would always say "n" here, so
there is no change to "git repack -h" output here...

> -             OPT_INTEGER(0, "max-pack-size", &max_pack_size,
> +             OPT_STRING(0, "max-pack-size", &max_pack_size, N_("bytes"),
>                               N_("maximum size of each packfile")),

...but this one will now say:

    --max-pack-size <bytes>
                          maximum size of each packfile

I think that is more descriptive, but pack-objects does just say "n".  I
am OK with it either way.

-Peff

Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote:
>
>> But I think there is a subtle problem. Here (and elsewhere) we use the
>> parsed value of "0" as a sentinel. I think that is OK for
>> --max-pack-size, where "0" is not a reasonable value. But git-repack(1)
>> says:
>> 
>>   --window-memory=0 makes memory usage unlimited, which is the default.
>> 
>> What does:
>> 
>>   git config pack.windowMemory 256m
>>   git repack --window-memory=0
>> 
>> do? It should override the config, but I think it does not with your
>> patch (nor with the current code). Using a string would fix that (though
>> you could also fix it by using a different sentinel, like ULONG_MAX).
>
> Here is a series that does that (and fixes the other issue I found). It
> would probably be nice to test these things, but checking that they
> actually had an impact is tricky (how do you know that --window-memory
> did the right thing?).
>
>   [1/3]: repack: fix typo in max-pack-size option
>   [2/3]: repack: make parsed string options const-correct
>   [3/3]: repack: propagate pack-objects options as strings
>
> -Peff

Sounds sensible; will chuck the hum-ulong series and replace with
these patches.

Thanks.

Re: [PATCH v2 0/2] Fix "repack --window-memory=4g" regression in 1.8.4

[Stefan Beller]

On 22.01.2014 20:58, Junio C Hamano wrote:
> The command line parser was broken when the command was
> reimplemented in C in two ways.  It incorrectly limited the value
> range of window-memory and max-pack-size to "int", and also stopped
> grokking the unit suffixes like 2g/400m/8k.
> 
> These two patches apply on top of 35c14176 (Reword repack
> documentation to no longer state it's a script, 2013-10-19) and
> later can be merged down to maint-1.8.4 track and upwards.
> 
> Junio C Hamano (2):
>   parse-options: refactor human-friendly-integer parser out of pack-objects
>   repack: accept larger window-memory and max-pack-size
> 
>  builtin/pack-objects.c | 25 ++++---------------------
>  builtin/repack.c       | 12 ++++++------
>  parse-options.c        | 17 +++++++++++++++++
>  parse-options.h        |  5 +++++
>  4 files changed, 32 insertions(+), 27 deletions(-)
> 

I recall we had a discussion about parsing as the shell script
just passed them on without altering the argument, while the new
c implementation parses the numbers already before passing them on.

Junio,
thanks for such a quick patch. I'd currently have only little time
for open source contributions.

The patches seems reasonable to me.

Thanks,
Stefan