Silly "git gc" UI issue.

Silly "git gc" UI issue.

[Linus Torvalds]

Ok, this is ridiculous, but I've done it several times, so I thought
I'd finally mention it to somebody on the git list that may care:

  "My name is Linus, and I'm a klutz".

what does that have to do with anything?

Now, imagine you're a klutz. Imagine you want to clean up your .git
directory. Combine those things, and what do you get?

You get this:

   git gc --prune=npw

Yeah, that "npw" should be "now", which is where the klutz thing comes in.

It turns out that git reacts ridiculously badly to this.

I'm just assuming that everybody else is scarily competent if I'm the
first to have reported this.

                  Linus

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> You get this:
>
>    git gc --prune=npw
>
> Yeah, that "npw" should be "now", which is where the klutz thing comes in.
>
> It turns out that git reacts ridiculously badly to this.

    $ git gc --prune=npw
    Counting objects: 10, done.
    Delta compression using up to 8 threads.
    Compressing objects: 100% (3/3), done.
    Writing objects: 100% (10/10), done.
    Total 10 (delta 2), reused 10 (delta 2)
    error: failed to run prune

It turns out that prune silently goes away given a bad expiry

    $ git prune --expire=nyah ; echo $?
    129

Regardless of your originai "git gc" issue, we should make "prune"
say something on this error.  And when we do so, I would think that
error message will come before the final "error: failed to run
prune".

Or perhaps we do so and then squelch "error: failed to run prune",
trusting that a corrected "git prune" will always say something when
it fails.

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> It turns out that prune silently goes away given a bad expiry
>
>     $ git prune --expire=nyah ; echo $?
>     129
>
> Regardless of your originai "git gc" issue, we should make "prune"
> say something on this error.  And when we do so, I would think that
> error message will come before the final "error: failed to run
> prune".
>
> Or perhaps we do so and then squelch "error: failed to run prune",
> trusting that a corrected "git prune" will always say something when
> it fails.

It turns out that

    $ git worktree prune --expire=nyah

shares the same issue.  I'll take a look at OPT_EXPIRY_DATE() thing.

Re: Silly "git gc" UI issue.

[Junio C Hamano]

A few commands that parse --expire=<time> command line option
behaves silly 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).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I do not expect this to be the final version (not just it lacks
   tests, but I haven't even run existing tests with the change
   yet), but I think I diagnosed the root cause correctly, at least.

 parse-options-cb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index c6679cb2cd..872627eafe 100644
--- 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,

Re: Silly "git gc" UI issue.

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Wed, Apr 18, 2018 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> A few commands that parse --expire=<time> command line option
> behaves silly when given nonsense input.  For example

So this patch definitely improves on the error message.

But look at what happens for the kernel:

    [torvalds@i7 linux]$ time git gc --prune=npw
    Counting objects: 6006319, done.
    Delta compression using up to 8 threads.
    Compressing objects: 100% (912166/912166), done.
    Writing objects: 100% (6006319/6006319), done.
    Total 6006319 (delta 5050577), reused 6006319 (delta 5050577)
    fatal: malformed expiration date 'npw'
    error: failed to run prune

    real        1m4.376s
    user        0m59.963s
    sys         0m5.182s



Yes, I get that nice "malformed expiration date 'npw'" error, but I
get it after 64 seconds has passed.

So i think builtin/gc.c should use this same parse_expiry_date()
parse_opt_expiry_date_cb() thing for its timestamp parsing.

It does actually seem to do that for the gc_log_expire value that it
loads from the config file.

Maybe something like the attached patch? Then I get:

    [torvalds@i7 linux]$ time git gc --prune=npw
    fatal: Failed to parse prune expiry value npw

    real        0m0.004s
    user        0m0.002s
    sys         0m0.002s

and you could smush it into your commit (if you want my sign-off, take it)

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 785 bytes --]

 builtin/gc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124ea..a4b20aaaf 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -354,6 +354,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")),
@@ -392,6 +393,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 (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)

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Yes, I get that nice "malformed expiration date 'npw'" error, but I
> get it after 64 seconds has passed.

Ah, that timing aspect of the issue didn't occur to me.  The patch
indeed is a reasonable workaround.

Thanks.

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Maybe something like the attached patch? Then I get:
> ...
>     [torvalds@i7 linux]$ time git gc --prune=npw
>     fatal: Failed to parse prune expiry value npw
>
>     real        0m0.004s
>     user        0m0.002s
>     sys         0m0.002s
>
> and you could smush it into your commit (if you want my sign-off, take it)
>
>               Linus
>
>  builtin/gc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3e67124ea..a4b20aaaf 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -354,6 +354,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")),
> @@ -392,6 +393,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 (parse_expiry_date(prune_expire, &dummy))
> +		die(_("Failed to parse prune expiry value %s"), prune_expire);
> +

At this point prune_expire could be NULL, so the if() needs a bit
tightening, but otherwise it looks good.

Here is the final one (at least for today).

-- >8 --
Subject: [PATCH] parseopt: handle malformed --expire arguments nicer

A few commands that parse --expire=<time> command line option behave
silly 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>
---
 builtin/gc.c       |  4 ++++
 parse-options-cb.c |  6 +++++-
 t/t5304-prune.sh   | 10 ++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c5eae0edf..858aa444e1 100644
--- 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 c6679cb2cd..872627eafe 100644
--- 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 6694c19a1e..af69cdc112 100755
--- 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
-- 
2.17.0-252-gfe0a9eaf31

Re: Silly "git gc" UI issue.

[Simon Ruderich]

On Thu, Apr 19, 2018 at 02:10:40PM +0900, Junio C Hamano wrote:
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index c6679cb2cd..872627eafe 100644
> --- 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;
>  }

Should this error get translated?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Simon Ruderich <simon@ruderich.org> writes:

> On Thu, Apr 19, 2018 at 02:10:40PM +0900, Junio C Hamano wrote:
>> diff --git a/parse-options-cb.c b/parse-options-cb.c
>> index c6679cb2cd..872627eafe 100644
>> --- 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;
>>  }
>
> Should this error get translated?

Sure.  The new test to check this codepath even protects itself from
such a translation by using test_i18ngrep, so this is safe to mark
for translation from day one.

Thanks.

-- >8 --
Subject: [PATCH v2] 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>
---

 * marking the new message in parse_opt_expiry_date_cb() function
   for i18n is the only change from the previous round.

 builtin/gc.c       |  4 ++++
 parse-options-cb.c |  6 +++++-
 t/t5304-prune.sh   | 10 ++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c5eae0edf..858aa444e1 100644
--- 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 c6679cb2cd..0f9f311a7a 100644
--- 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 6694c19a1e..af69cdc112 100755
--- 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

Re: Silly "git gc" UI issue.

[Christian Couder]

On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:

> @@ -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);

Micronit: I thought we prefer error messages to start with a lower
case letter, like:

               die(_("failed to parse prune expiry value %s"), prune_expire);

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> @@ -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);
>
> Micronit: I thought we prefer error messages to start with a lower
> case letter, like:
>
>                die(_("failed to parse prune expiry value %s"), prune_expire);

Thanks.

There is an existing "Failed..." already before the pre-context of
this hunk, which I'll fix with a preliminary clean-up patch.

Re: Silly "git gc" UI issue.

[Linus Torvalds]

On Wed, Apr 18, 2018 at 6:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Regardless of your originai "git gc" issue, we should make "prune"
> say something on this error.  And when we do so, I would think that
> error message will come before the final "error: failed to run
> prune".

So to me, the real failure is the fact that it spent a a lot of time
packing my repository before it then failed the prune at the end.

I don't actually mind the quality of the error message too much -
although it could be improved.

I mind the "oh, goddamnit, you just spent over a minute of CPU time on
my fairly high-end desktop, and _then_ you decided to tell me that I'm
a moron and couldn't type 'now' correctly".

So to me, the big deal would be that builtin/gc.c should validate the
date *before* it starts, instead of doing all that work, and then
executing "git prune" with invalid arguments..

                Linus

Re: Silly "git gc" UI issue.

[Simon Ruderich]

On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote:
> It turns out that prune silently goes away given a bad expiry
>
>     $ git prune --expire=nyah ; echo $?
>     129

I noticed that git log --since/--after/--before/--until have a
similar behavior and ignore date parsing errors in those options
completely. Is this expected or should we warn the user with
something like the following?

diff --git a/revision.c b/revision.c
index 4e0e193e57..e5ba6c7dfc 100644
--- a/revision.c
+++ b/revision.c
@@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_age = atoi(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
-		revs->max_age = approxidate(optarg);
+		int err = 0;
+		revs->max_age = approxidate_careful(optarg, &err);
+		if (err)
+			return error("--since: invalid time '%s'", optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("after", argv, &optarg))) {
-		revs->max_age = approxidate(optarg);
+		int err = 0;
+		revs->max_age = approxidate_careful(optarg, &err);
+		if (err)
+			return error("--after: invalid time '%s'", optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("min-age", argv, &optarg))) {
 		revs->min_age = atoi(optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("before", argv, &optarg))) {
-		revs->min_age = approxidate(optarg);
+		int err = 0;
+		revs->min_age = approxidate_careful(optarg, &err);
+		if (err)
+			return error("--before: invalid time '%s'", optarg);
 		return argcount;
 	} else if ((argcount = parse_long_opt("until", argv, &optarg))) {
-		revs->min_age = approxidate(optarg);
+		int err = 0;
+		revs->min_age = approxidate_careful(optarg, &err);
+		if (err)
+			return error("--until: invalid time '%s'");
 		return argcount;
 	} else if (!strcmp(arg, "--first-parent")) {
 		revs->first_parent_only = 1;

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Simon Ruderich <simon@ruderich.org> writes:

> On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote:
>> It turns out that prune silently goes away given a bad expiry
>>
>>     $ git prune --expire=nyah ; echo $?
>>     129
>
> I noticed that git log --since/--after/--before/--until have a
> similar behavior and ignore date parsing errors in those options
> completely. Is this expected or should we warn the user with
> something like the following?

I do not have a strong opinion on this, because I would expect that
"git log --since=nyah" to do whatever random things it may want to
do.

But I suspect I am a minority, and if we were to change the
established behaviour, I agree that erroring out using
approxidate_careful() is the right direction to go in.

Thanks.

> diff --git a/revision.c b/revision.c
> index 4e0e193e57..e5ba6c7dfc 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->max_age = atoi(optarg);
>  		return argcount;
>  	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
> -		revs->max_age = approxidate(optarg);
> +		int err = 0;
> +		revs->max_age = approxidate_careful(optarg, &err);
> +		if (err)
> +			return error("--since: invalid time '%s'", optarg);
>  		return argcount;
>  	} else if ((argcount = parse_long_opt("after", argv, &optarg))) {
> -		revs->max_age = approxidate(optarg);
> +		int err = 0;
> +		revs->max_age = approxidate_careful(optarg, &err);
> +		if (err)
> +			return error("--after: invalid time '%s'", optarg);
>  		return argcount;
>  	} else if ((argcount = parse_long_opt("min-age", argv, &optarg))) {
>  		revs->min_age = atoi(optarg);
>  		return argcount;
>  	} else if ((argcount = parse_long_opt("before", argv, &optarg))) {
> -		revs->min_age = approxidate(optarg);
> +		int err = 0;
> +		revs->min_age = approxidate_careful(optarg, &err);
> +		if (err)
> +			return error("--before: invalid time '%s'", optarg);
>  		return argcount;
>  	} else if ((argcount = parse_long_opt("until", argv, &optarg))) {
> -		revs->min_age = approxidate(optarg);
> +		int err = 0;
> +		revs->min_age = approxidate_careful(optarg, &err);
> +		if (err)
> +			return error("--until: invalid time '%s'");
>  		return argcount;
>  	} else if (!strcmp(arg, "--first-parent")) {
>  		revs->first_parent_only = 1;
>
> Regards
> Simon