{"thread":{"id":"35703","subject":"git repack --max-pack-size broken in git-next","startedAt":"2014-01-21T22:48:07Z","lastAt":"2014-01-23T18:37:01Z","messageCount":14,"participants":["Siddharth Agarwal","Junio C Hamano","Stefan Beller","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233469","messageId":"52DEF927.7090005@fb.com","threadId":"35703","inReplyTo":null,"subject":"git repack --max-pack-size broken in git-next","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-21T22:48:07Z","receivedAt":"2014-01-21T22:48:07Z","isPatch":false,"sender":{"key":"sid0@fb.com","avatar":null},"body":"With git-next, the --max-pack-size option to git repack doesn't work.\n\nWith git at b139ac2, `git repack --max-pack-size=1g` says\n\nerror: option `max-pack-size' expects a numerical value\n\nwhile `git repack --max-pack-size=1073741824` says\n\nerror: unknown option `max_pack_size=1073741824'\n\nI bisected this down to a1bbc6c, which rewrote git repack in C.\n"},{"id":"233474","messageId":"xmqqzjmprlbq.fsf@gitster.dls.corp.google.com","threadId":"35703","inReplyTo":"52DEF927.7090005@fb.com","subject":"Re: git repack --max-pack-size broken in git-next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T23:01:29Z","receivedAt":"2014-01-21T23:01:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Agarwal writes:\n\n> With git-next, the --max-pack-size option to git repack doesn't work.\n>\n> With git at b139ac2, `git repack --max-pack-size=1g` says\n>\n> error: option `max-pack-size' expects a numerical value\n\nThanks, Siddharth.\n\nIt seems that we have a hand-crafted parser outside parse-options\nframework in pack-objects, and the scripted git-repack used to pass\nthis option without interpreting itself.\n\nWe probably should lift the OPT_ULONG() implementation out of\nbuiltin/pack-objects.c and move it to parse-options.[ch] and use it\nin the reimplementation of repack.\n\nAnd probably use it in other places where these \"integers in\nhuman-friendly units\" may make sense, but that is a separate topic.\n"},{"id":"233488","messageId":"xmqqsisgsyqo.fsf@gitster.dls.corp.google.com","threadId":"35703","inReplyTo":"xmqqzjmprlbq.fsf@gitster.dls.corp.google.com","subject":"Re: git repack --max-pack-size broken in git-next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T23:26:23Z","receivedAt":"2014-01-21T23:26:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Siddharth Agarwal writes:\n>\n>> With git-next, the --max-pack-size option to git repack doesn't work.\n>>\n>> With git at b139ac2, `git repack --max-pack-size=1g` says\n>>\n>> error: option `max-pack-size' expects a numerical value\n>\n> Thanks, Siddharth.\n>\n> It seems that we have a hand-crafted parser outside parse-options\n> framework in pack-objects, and the scripted git-repack used to pass\n> this option without interpreting itself.\n>\n> We probably should lift the OPT_ULONG() implementation out of\n> builtin/pack-objects.c and move it to parse-options.[ch] and use it\n> in the reimplementation of repack.\n>\n> And probably use it in other places where these \"integers in\n> human-friendly units\" may make sense, but that is a separate topic.\n\nThe first step may look something like this (totally untested)..\n\n builtin/pack-objects.c | 22 +++-------------------\n parse-options.c | 17 +++++++++++++++++\n parse-options.h | 3 +++\n 3 files changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 541667f..b264f1f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2416,23 +2416,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_ulong(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tdie(_(\"option %s does not accept negative form\"),\n-\t\t opt->long_name);\n-\n-\tif (!git_parse_ulong(arg, opt->value))\n-\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n-\t\t arg, opt->long_name);\n-\treturn 0;\n-}\n-\n-#define OPT_ULONG(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), \"n\", (h),\t\\\n-\t PARSE_OPT_NONEG, option_parse_ulong }\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -2462,8 +2445,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"ignore packed objects\")),\n \t\tOPT_INTEGER(0, \"window\", &window,\n \t\t\t N_(\"limit pack window by objects\")),\n-\t\tOPT_ULONG(0, \"window-memory\", &window_memory_limit,\n-\t\t\t N_(\"limit pack window by memory in addition to object limit\")),\n+\t\t{ OPTION_ULONG, 0, \"window-memory\", &window_memory_limit, N_(\"n\"),\n+\t\t N_(\"limit pack window by memory in addition to object limit\"),\n+\t\t PARSE_OPT_HUMAN_NUMBER },\n \t\tOPT_INTEGER(0, \"depth\", &depth,\n \t\t\t N_(\"maximum length of delta chain allowed in the resulting pack\")),\n \t\tOPT_BOOL(0, \"reuse-delta\", &reuse_delta,\ndiff --git a/parse-options.c b/parse-options.c\nindex 7b8d3fa..3a47538 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -180,6 +180,23 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n \t\treturn 0;\n \n+\tcase OPTION_ULONG:\n+\t\tif (unset) {\n+\t\t\t*(unsigned long *)opt->value = 0;\n+\t\t} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n+\t\t\t*(unsigned long *)opt->value = opt->defval;\n+\t\t} else if (get_arg(p, opt, flags, &arg)) {\n+\t\t\treturn -1;\n+\t\t} else if (opt->flags & PARSE_OPT_HUMAN_NUMBER) {\n+\t\t\tif (!git_parse_ulong(arg, (unsigned long *)opt->value))\n+\t\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n+\t\t} else {\n+\t\t\t*(unsigned long *)opt->value = strtoul(arg, (char **)&s, 10);\n+\t\t\tif (*s)\n+\t\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n+\t\t}\n+\t\treturn 0;\n+\n \tdefault:\n \t\tdie(\"should not happen, someone must be hit on the forehead\");\n \t}\ndiff --git a/parse-options.h b/parse-options.h\nindex d670cb9..6a3cce0 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -17,6 +17,7 @@ enum parse_opt_type {\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n+\tOPTION_ULONG,\n \tOPTION_CALLBACK,\n \tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n@@ -38,6 +39,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 16,\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n+\tPARSE_OPT_HUMAN_NUMBER = 128,\n \tPARSE_OPT_SHELL_EVAL = 256\n };\n \n@@ -133,6 +135,7 @@ struct option {\n #define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n+#define OPT_ULONG(s, l, v, h) { OPTION_ULONG, (s), (l), (v), N_(\"n\"), (h) }\n #define OPT_STRING(s, l, v, a, h) { OPTION_STRING, (s), (l), (v), (a), (h) }\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n \t\t\t\t { OPTION_CALLBACK, (s), (l), (v), (a), \\\n"},{"id":"233531","messageId":"1390420685-18449-1-git-send-email-gitster@pobox.com","threadId":"35703","inReplyTo":"xmqqzjmprlbq.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/2] Fix \"repack --window-memory=4g\" regression in 1.8.4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T19:58:03Z","receivedAt":"2014-01-22T19:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The command line parser was broken when the command was\nreimplemented in C in two ways. It incorrectly limited the value\nrange of window-memory and max-pack-size to \"int\", and also stopped\ngrokking the unit suffixes like 2g/400m/8k.\n\nThese two patches apply on top of 35c14176 (Reword repack\ndocumentation to no longer state it's a script, 2013-10-19) and\nlater can be merged down to maint-1.8.4 track and upwards.\n\nJunio C Hamano (2):\n parse-options: refactor human-friendly-integer parser out of pack-objects\n repack: accept larger window-memory and max-pack-size\n\n builtin/pack-objects.c | 25 ++++---------------------\n builtin/repack.c | 12 ++++++------\n parse-options.c | 17 +++++++++++++++++\n parse-options.h | 5 +++++\n 4 files changed, 32 insertions(+), 27 deletions(-)\n\n-- \n1.9-rc0-151-ga5225c0\n"},{"id":"233532","messageId":"1390420685-18449-2-git-send-email-gitster@pobox.com","threadId":"35703","inReplyTo":"1390420685-18449-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/2] parse-options: refactor human-friendly-integer parser out of pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T19:58:04Z","receivedAt":"2014-01-22T19:58:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We only had code to understand unit suffixes such as g/m/k (as in\n2g/400m/8k) in the configuration parser but not in the command line\noption parser. \"git pack-objects\" worked it around by having a\ncustom extension to the parse-options API; make it available to\nother callers.\n\nThe new macro is not called OPT_ULONG() but OPT_HUM_ULONG(), as it\nwould be bizzarre to have ULONG that understands human friendly\nunits while INTEGER that does not. It is not named with a shorter\n\"OPT_HULONG\", primarily to avoid having to name a future parallel\nfor parsing human friendly integer \"OPT_HINT\".\n\nSigned-off-by: Junio C Hamano \n---\n builtin/pack-objects.c | 25 ++++---------------------\n parse-options.c | 17 +++++++++++++++++\n parse-options.h | 5 +++++\n 3 files changed, 26 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f069462..2fa8e1e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2417,23 +2417,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_ulong(const struct option *opt,\n-\t\t\t const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tdie(_(\"option %s does not accept negative form\"),\n-\t\t opt->long_name);\n-\n-\tif (!git_parse_ulong(arg, opt->value))\n-\t\tdie(_(\"unable to parse value '%s' for option %s\"),\n-\t\t arg, opt->long_name);\n-\treturn 0;\n-}\n-\n-#define OPT_ULONG(s, l, v, h) \\\n-\t{ OPTION_CALLBACK, (s), (l), (v), \"n\", (h),\t\\\n-\t PARSE_OPT_NONEG, option_parse_ulong }\n-\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n@@ -2455,16 +2438,16 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 0, \"index-version\", NULL, N_(\"version[,offset]\"),\n \t\t N_(\"write the pack index file in the specified idx format version\"),\n \t\t 0, option_parse_index_version },\n-\t\tOPT_ULONG(0, \"max-pack-size\", &pack_size_limit,\n-\t\t\t N_(\"maximum size of each output pack file\")),\n+\t\tOPT_HUM_ULONG(0, \"max-pack-size\", &pack_size_limit,\n+\t\t\t N_(\"maximum size of each output pack file\")),\n \t\tOPT_BOOL(0, \"local\", &local,\n \t\t\t N_(\"ignore borrowed objects from alternate object store\")),\n \t\tOPT_BOOL(0, \"incremental\", &incremental,\n \t\t\t N_(\"ignore packed objects\")),\n \t\tOPT_INTEGER(0, \"window\", &window,\n \t\t\t N_(\"limit pack window by objects\")),\n-\t\tOPT_ULONG(0, \"window-memory\", &window_memory_limit,\n-\t\t\t N_(\"limit pack window by memory in addition to object limit\")),\n+\t\tOPT_HUM_ULONG(0, \"window-memory\", &window_memory_limit,\n+\t\t N_(\"limit pack window by memory in addition to object limit\")),\n \t\tOPT_INTEGER(0, \"depth\", &depth,\n \t\t\t N_(\"maximum length of delta chain allowed in the resulting pack\")),\n \t\tOPT_BOOL(0, \"reuse-delta\", &reuse_delta,\ndiff --git a/parse-options.c b/parse-options.c\nindex c2cbca2..948ade7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -136,6 +136,23 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n \t\treturn 0;\n \n+\tcase OPTION_ULONG:\n+\t\tif (unset) {\n+\t\t\t*(unsigned long *)opt->value = 0;\n+\t\t} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {\n+\t\t\t*(unsigned long *)opt->value = opt->defval;\n+\t\t} else if (get_arg(p, opt, flags, &arg)) {\n+\t\t\treturn -1;\n+\t\t} else if (opt->flags & PARSE_OPT_HUMAN_NUMBER) {\n+\t\t\tif (!git_parse_ulong(arg, (unsigned long *)opt->value))\n+\t\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n+\t\t} else {\n+\t\t\t*(unsigned long *)opt->value = strtoul(arg, (char **)&s, 10);\n+\t\t\tif (*s)\n+\t\t\t\treturn opterror(opt, \"expects a numerical value\", flags);\n+\t\t}\n+\t\treturn 0;\n+\n \tdefault:\n \t\tdie(\"should not happen, someone must be hit on the forehead\");\n \t}\ndiff --git a/parse-options.h b/parse-options.h\nindex 9b94596..d65ecdb 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -16,6 +16,7 @@ enum parse_opt_type {\n \t/* options with arguments (usually) */\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n+\tOPTION_ULONG,\n \tOPTION_CALLBACK,\n \tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n@@ -40,6 +41,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_LASTARG_DEFAULT = 16,\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n+\tPARSE_OPT_HUMAN_NUMBER = 128,\n \tPARSE_OPT_SHELL_EVAL = 256\n };\n \n@@ -131,6 +133,9 @@ struct option {\n #define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n \t\t\t\t (h), PARSE_OPT_NOARG, NULL, (p) }\n #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(\"n\"), (h) }\n+#define OPT_ULONG(s, l, v, h) { OPTION_ULONG, (s), (l), (v), N_(\"n\"), (h) }\n+#define OPT_HUM_ULONG(s, l, v, h) { OPTION_ULONG, (s), (l), (v), N_(\"n\"), (h), \\\n+\t\t\t\t PARSE_OPT_HUMAN_NUMBER }\n #define OPT_STRING(s, l, v, a, h) { OPTION_STRING, (s), (l), (v), (a), (h) }\n #define OPT_STRING_LIST(s, l, v, a, h) \\\n \t\t\t\t { OPTION_CALLBACK, (s), (l), (v), (a), \\\n-- \n1.9-rc0-151-ga5225c0\n"},{"id":"233533","messageId":"1390420685-18449-3-git-send-email-gitster@pobox.com","threadId":"35703","inReplyTo":"1390420685-18449-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/2] repack: accept larger window-memory and max-pack-size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-22T19:58:05Z","receivedAt":"2014-01-22T19:58:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These quantities can be larger than an int. Use ulong to express\nthem like the underlying pack-objects does, and also parse them with\nthe human-friendly unit suffixes.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/repack.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a0ff5c7..8ce396b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -130,9 +130,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint pack_everything = 0;\n \tint delete_redundant = 0;\n \tchar *unpack_unreachable = NULL;\n-\tint window = 0, window_memory = 0;\n+\tint window = 0;\n \tint depth = 0;\n-\tint max_pack_size = 0;\n+\tunsigned long max_pack_size = 0, window_memory = 0;\n \tint no_reuse_delta = 0, no_reuse_object = 0;\n \tint no_update_server_info = 0;\n \tint quiet = 0;\n@@ -159,11 +159,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n \t\tOPT_INTEGER(0, \"window\", &window,\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_INTEGER(0, \"window-memory\", &window_memory,\n+\t\tOPT_HUM_ULONG(0, \"window-memory\", &window_memory,\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n \t\tOPT_INTEGER(0, \"depth\", &depth,\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_INTEGER(0, \"max-pack-size\", &max_pack_size,\n+\t\tOPT_HUM_ULONG(0, \"max-pack-size\", &max_pack_size,\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_END()\n \t};\n@@ -187,11 +187,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (window)\n \t\targv_array_pushf(&cmd_args, \"--window=%u\", window);\n \tif (window_memory)\n-\t\targv_array_pushf(&cmd_args, \"--window-memory=%u\", window_memory);\n+\t\targv_array_pushf(&cmd_args, \"--window-memory=%lu\", window_memory);\n \tif (depth)\n \t\targv_array_pushf(&cmd_args, \"--depth=%u\", depth);\n \tif (max_pack_size)\n-\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%u\", max_pack_size);\n+\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%lu\", max_pack_size);\n \tif (no_reuse_delta)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n-- \n1.9-rc0-151-ga5225c0\n"},{"id":"233555","messageId":"52E04741.6040403@googlemail.com","threadId":"35703","inReplyTo":"1390420685-18449-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 0/2] Fix \"repack --window-memory=4g\" regression in 1.8.4","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2014-01-22T22:33:37Z","receivedAt":"2014-01-22T22:33:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 22.01.2014 20:58, Junio C Hamano wrote:\n> The command line parser was broken when the command was\n> reimplemented in C in two ways. It incorrectly limited the value\n> range of window-memory and max-pack-size to \"int\", and also stopped\n> grokking the unit suffixes like 2g/400m/8k.\n> \n> These two patches apply on top of 35c14176 (Reword repack\n> documentation to no longer state it's a script, 2013-10-19) and\n> later can be merged down to maint-1.8.4 track and upwards.\n> \n> Junio C Hamano (2):\n> parse-options: refactor human-friendly-integer parser out of pack-objects\n> repack: accept larger window-memory and max-pack-size\n> \n> builtin/pack-objects.c | 25 ++++---------------------\n> builtin/repack.c | 12 ++++++------\n> parse-options.c | 17 +++++++++++++++++\n> parse-options.h | 5 +++++\n> 4 files changed, 32 insertions(+), 27 deletions(-)\n> \n\nI recall we had a discussion about parsing as the shell script\njust passed them on without altering the argument, while the new\nc implementation parses the numbers already before passing them on.\n\nJunio,\nthanks for such a quick patch. I'd currently have only little time\nfor open source contributions.\n\nThe patches seems reasonable to me.\n\nThanks,\nStefan\n"},{"id":"233572","messageId":"20140123010642.GB17254@sigill.intra.peff.net","threadId":"35703","inReplyTo":"1390420685-18449-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:06:42Z","receivedAt":"2014-01-23T01:06:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2014 at 11:58:05AM -0800, Junio C Hamano wrote:\n\n> These quantities can be larger than an int. Use ulong to express\n> them like the underlying pack-objects does, and also parse them with\n> the human-friendly unit suffixes.\n\nHrm. I think that is a valid strategy, but...\n\n> -\tint max_pack_size = 0;\n> +\tunsigned long max_pack_size = 0, window_memory = 0;\n\nHere we must use the correct C type...\n\n> -\t\tOPT_INTEGER(0, \"window-memory\", &window_memory,\n> +\t\tOPT_HUM_ULONG(0, \"window-memory\", &window_memory,\n\nAnd here use the correct parser...\n\n> \tif (window_memory)\n> -\t\targv_array_pushf(&cmd_args, \"--window-memory=%u\", window_memory);\n> +\t\targv_array_pushf(&cmd_args, \"--window-memory=%lu\", window_memory);\n\nAnd here use the correct format string...\n\nAll of which must match what pack-objects does, or we risk a further\nbreak (though I do not guess it will change from ulong anytime soon).\nThe original shell version worked because they were all strings. We do\nnot care about the numeric value here, and are just forwarding the value\nalong to pack-objects. Why not just use a string?\n\nThe only advantage I can think of is that this gives us slightly earlier\nerror detection for \"git repack --window-memory=bogosity\".\n\nBut I think there is a subtle problem. Here (and elsewhere) we use the\nparsed value of \"0\" as a sentinel. I think that is OK for\n--max-pack-size, where \"0\" is not a reasonable value. But git-repack(1)\nsays:\n\n --window-memory=0 makes memory usage unlimited, which is the default.\n\nWhat does:\n\n git config pack.windowMemory 256m\n git repack --window-memory=0\n\ndo? It should override the config, but I think it does not with your\npatch (nor with the current code). Using a string would fix that (though\nyou could also fix it by using a different sentinel, like ULONG_MAX).\n\n> \tif (max_pack_size)\n> -\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%u\", max_pack_size);\n> +\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%lu\", max_pack_size);\n\nThese underscores are interesting:\n\n $ git pack-objects --max_pack_size=256m\n error: unknown option `max_pack_size=256m'\n\nI get the feeling the test suite does not cover this feature very well.\n:)\n\n-Peff\n"},{"id":"233573","messageId":"20140123012656.GC17254@sigill.intra.peff.net","threadId":"35703","inReplyTo":"20140123010642.GB17254@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:26:56Z","receivedAt":"2014-01-23T01:26:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote:\n\n> But I think there is a subtle problem. Here (and elsewhere) we use the\n> parsed value of \"0\" as a sentinel. I think that is OK for\n> --max-pack-size, where \"0\" is not a reasonable value. But git-repack(1)\n> says:\n> \n> --window-memory=0 makes memory usage unlimited, which is the default.\n> \n> What does:\n> \n> git config pack.windowMemory 256m\n> git repack --window-memory=0\n> \n> do? It should override the config, but I think it does not with your\n> patch (nor with the current code). Using a string would fix that (though\n> you could also fix it by using a different sentinel, like ULONG_MAX).\n\nHere is a series that does that (and fixes the other issue I found). It\nwould probably be nice to test these things, but checking that they\nactually had an impact is tricky (how do you know that --window-memory\ndid the right thing?).\n\n [1/3]: repack: fix typo in max-pack-size option\n [2/3]: repack: make parsed string options const-correct\n [3/3]: repack: propagate pack-objects options as strings\n\n-Peff\n"},{"id":"233574","messageId":"20140123012752.GA19472@sigill.intra.peff.net","threadId":"35703","inReplyTo":"20140123012656.GC17254@sigill.intra.peff.net","subject":"[PATCH 1/3] repack: fix typo in max-pack-size option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:27:52Z","receivedAt":"2014-01-23T01:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see \"--max-pack-size\", we accidentally propagated\nthis to pack-objects as \"--max_pack_size\", which does not\nwork at all.\n\nSigned-off-by: Jeff King \n---\n builtin/repack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ba66c6e..528725b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (depth)\n \t\targv_array_pushf(&cmd_args, \"--depth=%u\", depth);\n \tif (max_pack_size)\n-\t\targv_array_pushf(&cmd_args, \"--max_pack_size=%u\", max_pack_size);\n+\t\targv_array_pushf(&cmd_args, \"--max-pack-size=%u\", max_pack_size);\n \tif (no_reuse_delta)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233575","messageId":"20140123012830.GB19472@sigill.intra.peff.net","threadId":"35703","inReplyTo":"20140123012656.GC17254@sigill.intra.peff.net","subject":"[PATCH 2/3] repack: make parsed string options const-correct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:28:30Z","receivedAt":"2014-01-23T01:28:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we use OPT_STRING to parse an option, we get back a\npointer into the argv array, which should be \"const char *\".\nThe compiler doesn't notice because it gets passed through a\n\"void *\" in the option struct.\n\nSigned-off-by: Jeff King \n---\nNot a big deal, but just for consistency with the next patch.\n\n builtin/repack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 528725b..7f89c7c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -129,7 +129,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t/* variables to be filled by option parsing */\n \tint pack_everything = 0;\n \tint delete_redundant = 0;\n-\tchar *unpack_unreachable = NULL;\n+\tconst char *unpack_unreachable = NULL;\n \tint window = 0, window_memory = 0;\n \tint depth = 0;\n \tint max_pack_size = 0;\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233576","messageId":"20140123013008.GC19472@sigill.intra.peff.net","threadId":"35703","inReplyTo":"20140123012656.GC17254@sigill.intra.peff.net","subject":"[PATCH 3/3] repack: propagate pack-objects options as strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:30:13Z","receivedAt":"2014-01-23T01:30:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the original shell version of git-repack, any options\ndestined for pack-objects were left as strings, and passed\nas a whole. Since the C rewrite in commit a1bbc6c (repack:\nrewrite the shell script in C, 2013-09-15), we now parse\nthese values to integers internally, then reformat the\nintegers when passing the option to pack-objects.\n\nThis has the advantage that we catch format errors earlier\n(i.e., when repack is invoked, rather than when pack-objects\nis invoked).\n\nIt has three disadvantages, though:\n\n 1. Our internal data types may not be the right size. In\n the case of \"--window-memory\" and \"--max-pack-size\",\n these are \"unsigned long\" in pack-objects, but we can\n only represent a regular \"int\".\n\n 2. Our parsing routines might not be the same as those of\n pack-objects. For the two options above, pack-objects\n understands \"100m\" to mean \"100 megabytes\", but repack\n does not.\n\n 3. We have to keep a sentinel value to know whether it is\n worth passing the option along. In the case of\n \"--window-memory\", we currently do not pass it if the\n value is \"0\". But that is a meaningful value to\n pack-objects, where it overrides any configured value.\n\nWe can fix all of these by simply passing the strings from\nthe user along to pack-objects verbatim. This does not\nactually fix anything for \"--depth\" or \"--window\", but these\nare converted, too, for consistency.\n\nSigned-off-by: Jeff King \n---\nSo we lose the advantage listed above. But I think the simplicity and\nfuture-proofing is worth it (and in this case, we basically don't do\nanything _except_ invoke pack-objects, so it is not like we do a bunch\nof early work that has to be undone when we find out that the option is\nbogus later on).\n\n builtin/repack.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 7f89c7c..6284846 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -130,9 +130,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint pack_everything = 0;\n \tint delete_redundant = 0;\n \tconst char *unpack_unreachable = NULL;\n-\tint window = 0, window_memory = 0;\n-\tint depth = 0;\n-\tint max_pack_size = 0;\n+\tconst char *window = NULL, *window_memory = NULL;\n+\tconst char *depth = NULL;\n+\tconst char *max_pack_size = NULL;\n \tint no_reuse_delta = 0, no_reuse_object = 0;\n \tint no_update_server_info = 0;\n \tint quiet = 0;\n@@ -157,13 +157,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"pass --local to git-pack-objects\")),\n \t\tOPT_STRING(0, \"unpack-unreachable\", &unpack_unreachable, N_(\"approxidate\"),\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n-\t\tOPT_INTEGER(0, \"window\", &window,\n+\t\tOPT_STRING(0, \"window\", &window, N_(\"n\"),\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_INTEGER(0, \"window-memory\", &window_memory,\n+\t\tOPT_STRING(0, \"window-memory\", &window_memory, N_(\"bytes\"),\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n-\t\tOPT_INTEGER(0, \"depth\", &depth,\n+\t\tOPT_STRING(0, \"depth\", &depth, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_INTEGER(0, \"max-pack-size\", &max_pack_size,\n+\t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_END()\n \t};\n@@ -185,13 +185,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\n \tif (window)\n-\t\targv_array_pushf(&cmd_args, \"--window=%u\", window);\n+\t\targv_array_pushf(&cmd_args, \"--window=%s\", window);\n \tif (window_memory)\n-\t\targv_array_pushf(&cmd_args, \"--window-memory=%u\", window_memory);\n+\t\targv_array_pushf(&cmd_args, \"--window-memory=%s\", window_memory);\n \tif (depth)\n-\t\targv_array_pushf(&cmd_args, \"--depth=%u\", depth);\n+\t\targv_array_pushf(&cmd_args, \"--depth=%s\", depth);\n \tif (max_pack_size)\n-\t\targv_array_pushf(&cmd_args, \"--max-pack-size=%u\", max_pack_size);\n+\t\targv_array_pushf(&cmd_args, \"--max-pack-size=%s\", max_pack_size);\n \tif (no_reuse_delta)\n \t\targv_array_pushf(&cmd_args, \"--no-reuse-delta\");\n \tif (no_reuse_object)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233577","messageId":"20140123013847.GD17254@sigill.intra.peff.net","threadId":"35703","inReplyTo":"20140123013008.GC19472@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] repack: propagate pack-objects options as strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T01:38:47Z","receivedAt":"2014-01-23T01:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2014 at 08:30:13PM -0500, Jeff King wrote:\n\n> -\t\tOPT_INTEGER(0, \"window\", &window,\n> +\t\tOPT_STRING(0, \"window\", &window, N_(\"n\"),\n> \t\t\t\tN_(\"size of the window used for delta compression\")),\n\nBy the way, the old code with OPT_INTEGER would always say \"n\" here, so\nthere is no change to \"git repack -h\" output here...\n\n> - OPT_INTEGER(0, \"max-pack-size\", &max_pack_size,\n> + OPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\n> N_(\"maximum size of each packfile\")),\n\n...but this one will now say:\n\n --max-pack-size \n maximum size of each packfile\n\nI think that is more descriptive, but pack-objects does just say \"n\". I\nam OK with it either way.\n\n-Peff\n"},{"id":"233610","messageId":"xmqqd2jiv92q.fsf@gitster.dls.corp.google.com","threadId":"35703","inReplyTo":"20140123012656.GC17254@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-23T18:37:01Z","receivedAt":"2014-01-23T18:37:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 22, 2014 at 08:06:42PM -0500, Jeff King wrote:\n>\n>> But I think there is a subtle problem. Here (and elsewhere) we use the\n>> parsed value of \"0\" as a sentinel. I think that is OK for\n>> --max-pack-size, where \"0\" is not a reasonable value. But git-repack(1)\n>> says:\n>> \n>> --window-memory=0 makes memory usage unlimited, which is the default.\n>> \n>> What does:\n>> \n>> git config pack.windowMemory 256m\n>> git repack --window-memory=0\n>> \n>> do? It should override the config, but I think it does not with your\n>> patch (nor with the current code). Using a string would fix that (though\n>> you could also fix it by using a different sentinel, like ULONG_MAX).\n>\n> Here is a series that does that (and fixes the other issue I found). It\n> would probably be nice to test these things, but checking that they\n> actually had an impact is tricky (how do you know that --window-memory\n> did the right thing?).\n>\n> [1/3]: repack: fix typo in max-pack-size option\n> [2/3]: repack: make parsed string options const-correct\n> [3/3]: repack: propagate pack-objects options as strings\n>\n> -Peff\n\nSounds sensible; will chuck the hum-ulong series and replace with\nthese patches.\n\nThanks.\n"}]}