From: Junio C Hamano <gitster@pobox•com>
To: <stefanbeller@googlemail•com>
Cc: <git@vger•kernel.org>
Subject: Re: git repack --max-pack-size broken in git-next
Date: Tue, 21 Jan 2014 15:26:23 -0800 [thread overview]
Message-ID: <xmqqsisgsyqo.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <xmqqzjmprlbq.fsf@gitster.dls.corp.google.com> (Junio C. Hamano's message of "Tue, 21 Jan 2014 15:01:29 -0800")
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), \
next prev parent reply other threads:[~2014-01-21 23:26 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-21 22:48 git repack --max-pack-size broken in git-next Siddharth Agarwal
2014-01-21 23:01 ` Junio C Hamano
2014-01-21 23:26 ` Junio C Hamano [this message]
2014-01-22 19:58 ` [PATCH v2 0/2] Fix "repack --window-memory=4g" regression in 1.8.4 Junio C Hamano
2014-01-22 19:58 ` [PATCH v2 1/2] parse-options: refactor human-friendly-integer parser out of pack-objects Junio C Hamano
2014-01-22 19:58 ` [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size Junio C Hamano
2014-01-23 1:06 ` Jeff King
2014-01-23 1:26 ` Jeff King
2014-01-23 1:27 ` [PATCH 1/3] repack: fix typo in max-pack-size option Jeff King
2014-01-23 1:28 ` [PATCH 2/3] repack: make parsed string options const-correct Jeff King
2014-01-23 1:30 ` [PATCH 3/3] repack: propagate pack-objects options as strings Jeff King
2014-01-23 1:38 ` Jeff King
2014-01-23 18:37 ` [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size Junio C Hamano
2014-01-22 22:33 ` [PATCH v2 0/2] Fix "repack --window-memory=4g" regression in 1.8.4 Stefan Beller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsisgsyqo.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=stefanbeller@googlemail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox