Re: Silly "git gc" UI issue.

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Apr 19, 2018, 02:29 UTC
Message-ID: <CA+55aFztDdB9tVHREhQ7T0COs7p9ng81XfAHZCL3rx9WT2ecEQ@mail.gmail.com>
In-Reply-To: <xmqqfu3seyad.fsf@gitster-ct.c.googlers.com>

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

 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)