parse-options: support unit factors in `OPT_INTEGER()`

There are two main differences between `OPT_INTEGER()` and
`OPT_MAGNITUDE()`:

  - The former parses signed integers whereas the latter parses unsigned
    integers.

  - The latter parses unit factors like 'k', 'm' or 'g'.

While the first difference makes obvious sense, there isn't really a
good reason why signed integers shouldn't support unit factors, too.

This inconsistency will also become a bit of a problem with subsequent
commits, where we will fix a couple of callsites that pass an unsigned
integer to `OPT_INTEGER()`. There are three options:

  - We could adapt those users to instead pass a signed integer, but
    this would needlessly extend the range of accepted integer values.

  - We could convert them to use `OPT_MAGNITUDE()`, as it only accepts
    unsigned integers. But now we have the inconsistency that we also
    start to accept unit factors.

  - We could introduce `OPT_UNSIGNED()` as equivalent to `OPT_INTEGER()`
    so that it knows to only accept unsigned integers without unit
    suffix.

Introducing a whole new option type feels a bit excessive. There also
isn't really a good reason why `OPT_INTEGER()` cannot be extended to
also accept unit factors: all valid values passed to such options cannot
have a unit factors right now, so there wouldn't be any ambiguity.

Refactor `OPT_INTEGER()` to use `git_parse_int()`, which knows to
interpret unit factors. This removes the inconsistency between the
signed and unsigned options so that we can easily fix up callsites that
pass the wrong integer type right now.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc
index 61fa6ee167833a454bfa01b827577feac15b5f1d..63acfb419bd23a09ed096acda8ac32dfbc877b30 100644 (file)
--- a/Documentation/technical/api-parse-options.adoc
+++ b/Documentation/technical/api-parse-options.adoc
@@ -211,8 +211,10 @@ There are some macros to easily define options:
        Use of `--no-option` will clear the list of preceding values.
 
 `OPT_INTEGER(short, long, &int_var, description)`::
-       Introduce an option with integer argument.
-       The integer is put into `int_var`.
+       Introduce an option with integer argument. The argument must be a
+       integer and may include a suffix of 'k', 'm' or 'g' to
+       scale the provided value by 1024, 1024^2 or 1024^3 respectively.
+       The scaled value is put into `int_var`.
 
 `OPT_MAGNITUDE(short, long, &unsigned_long_var, description)`::
        Introduce an option with a size argument. The argument must be a

diff --git a/parse-options.c b/parse-options.c
index 35fbb3b0d633dfce5938ea801ca1850d0cded4bb..b287436e81a3f988521ed78a6b876d9216ce2fad 100644 (file)
--- a/parse-options.c
+++ b/parse-options.c
@@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
                                          enum opt_parsed flags,
                                          const char **argp)
 {
-       const char *s, *arg;
+       const char *arg;
        const int unset = flags & OPT_UNSET;
        int err;
 
@@ -185,9 +185,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
                if (!*arg)
                        return error(_("%s expects a numerical value"),
                                     optname(opt, flags));
-               *(int *)opt->value = strtol(arg, (char **)&s, 10);
-               if (*s)
-                       return error(_("%s expects a numerical value"),
+               if (!git_parse_int(arg, opt->value))
+                       return error(_("%s expects an integer value"
+                                      " with an optional k/m/g suffix"),
                                     optname(opt, flags));
                return 0;
 

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 2fe3522305f64d0f8366d160d4c03de81c4a210a..0c538c4b4376c7728c43204bb6c0b1ea8bc04e9b 100755 (executable)
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -111,7 +111,9 @@ test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
-test_expect_success 'OPT_INT() negative' 'check integer: -2345 -i -2345'
+test_expect_success 'OPT_INTEGER() negative' 'check integer: -2345 -i -2345'
+test_expect_success 'OPT_INTEGER() kilo' 'check integer: 239616 -i 234k'
+test_expect_success 'OPT_INTEGER() negative kilo' 'check integer: -239616 -i -234k'
 
 test_expect_success 'OPT_MAGNITUDE() simple' '
        check magnitude: 2345678 -m 2345678