parseopt: handle malformed --expire arguments more nicely

A few commands that parse --expire=<time> command line option behave
sillily when given nonsense input.  For example

$ git prune --no-expire
Segmentation falut
$ git prune --expire=npw; echo $?
129

Both come from parse_opt_expiry_date_cb().

The former is because the function is not prepared to see arg==NULL
(for "--no-expire", it is a norm; "--expire" at the end of the
command line could be made to pass NULL, if it is told that the
argument is optional, but we don't so we do not have to worry about
that case).

The latter is because it does not check the value returned from the
underlying parse_expiry_date().

This seems to be a recent regression introduced while we attempted
to avoid spewing the entire usage message when given a correct
option but with an invalid value at 3bb0923f ("parse-options: do not
show usage upon invalid option value", 2018-03-22).  Before that, we
didn't fail silently but showed a full usage help (which arguably is
not all that better).

Also catch this error early when "git gc --prune=<expiration>" is
misspelled by doing a dummy parsing before the main body of "gc"
that is time consuming even begins.  Otherwise, we'd spend time to
pack objects and then later have "git prune" first notice the error.
Aborting "gc" in the middle that way is not harmful but is ugly and
can be avoided.

Helped-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/gc.c b/builtin/gc.c
index 841782c8593186a538750453806d23e7f76a8ff8..90a10679714d000c0a4b386963def520d3f5d61c 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -353,6 +353,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
        const char *name;
        pid_t pid;
        int daemonized = 0;
+       timestamp_t dummy;
 
        struct option builtin_gc_options[] = {
                OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
        if (argc > 0)
                usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+       if (prune_expire && parse_expiry_date(prune_expire, &dummy))
+               die(_("failed to parse prune expiry value %s"), prune_expire);
+
        if (aggressive) {
                argv_array_push(&repack, "-f");
                if (aggressive_depth > 0)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index c6679cb2cdee15981ee9ef31c402c358a3727d1e..0f9f311a7a93350a584125c90da643fce96c34e7 100644 (file)
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
 int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
                             int unset)
 {
-       return parse_expiry_date(arg, (timestamp_t *)opt->value);
+       if (unset)
+               arg = "never";
+       if (parse_expiry_date(arg, (timestamp_t *)opt->value))
+               die(_("malformed expiration date '%s'"), arg);
+       return 0;
 }
 
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6694c19a1eecf10117b843bbacbc5bb47924c9c8..af69cdc112c7f5667fc4c9069137716aee44becc 100755 (executable)
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -320,4 +320,14 @@ test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '
        test_cmp expected actual
 '
 
+test_expect_success 'prune: handle expire option correctly' '
+       test_must_fail git prune --expire 2>error &&
+       test_i18ngrep "requires a value" error &&
+
+       test_must_fail git prune --expire=nyah 2>error &&
+       test_i18ngrep "malformed expiration" error &&
+
+       git prune --no-expire
+'
+
 test_done