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

[Junio C Hamano <gitster@pobox.com>]

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), \