{"thread":{"id":"48323","subject":"Silly \"git gc\" UI issue.","startedAt":"2018-04-19T01:46:04Z","lastAt":"2018-04-23T13:38:07Z","messageCount":14,"participants":["Linus Torvalds","Junio C Hamano","Simon Ruderich","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"345047","messageId":"CA+55aFxSZLuk++Dz6SonD+JhbbSDt9G9VcBx5f1CV=6nJC9hvg@mail.gmail.com","threadId":"48323","inReplyTo":null,"subject":"Silly \"git gc\" UI issue.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-04-19T01:45:59Z","receivedAt":"2018-04-19T01:46:04Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Ok, this is ridiculous, but I've done it several times, so I thought\nI'd finally mention it to somebody on the git list that may care:\n\n  \"My name is Linus, and I'm a klutz\".\n\nwhat does that have to do with anything?\n\nNow, imagine you're a klutz. Imagine you want to clean up your .git\ndirectory. Combine those things, and what do you get?\n\nYou get this:\n\n   git gc --prune=npw\n\nYeah, that \"npw\" should be \"now\", which is where the klutz thing comes in.\n\nIt turns out that git reacts ridiculously badly to this.\n\nI'm just assuming that everybody else is scarily competent if I'm the\nfirst to have reported this.\n\n                  Linus\n"},{"id":"345048","messageId":"xmqqr2ncezdc.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"CA+55aFxSZLuk++Dz6SonD+JhbbSDt9G9VcBx5f1CV=6nJC9hvg@mail.gmail.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T01:52:47Z","receivedAt":"2018-04-19T01:52:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> You get this:\n>\n>    git gc --prune=npw\n>\n> Yeah, that \"npw\" should be \"now\", which is where the klutz thing comes in.\n>\n> It turns out that git reacts ridiculously badly to this.\n\n    $ git gc --prune=npw\n    Counting objects: 10, done.\n    Delta compression using up to 8 threads.\n    Compressing objects: 100% (3/3), done.\n    Writing objects: 100% (10/10), done.\n    Total 10 (delta 2), reused 10 (delta 2)\n    error: failed to run prune\n\nIt turns out that prune silently goes away given a bad expiry\n\n    $ git prune --expire=nyah ; echo $?\n    129\n\nRegardless of your originai \"git gc\" issue, we should make \"prune\"\nsay something on this error.  And when we do so, I would think that\nerror message will come before the final \"error: failed to run\nprune\".\n\nOr perhaps we do so and then squelch \"error: failed to run prune\",\ntrusting that a corrected \"git prune\" will always say something when\nit fails.\n\n\n\n"},{"id":"345049","messageId":"xmqqmuy0ez8b.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"xmqqr2ncezdc.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T01:55:48Z","receivedAt":"2018-04-19T01:55:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> It turns out that prune silently goes away given a bad expiry\n>\n>     $ git prune --expire=nyah ; echo $?\n>     129\n>\n> Regardless of your originai \"git gc\" issue, we should make \"prune\"\n> say something on this error.  And when we do so, I would think that\n> error message will come before the final \"error: failed to run\n> prune\".\n>\n> Or perhaps we do so and then squelch \"error: failed to run prune\",\n> trusting that a corrected \"git prune\" will always say something when\n> it fails.\n\nIt turns out that\n\n    $ git worktree prune --expire=nyah\n\nshares the same issue.  I'll take a look at OPT_EXPIRY_DATE() thing.\n\n"},{"id":"345050","messageId":"xmqqfu3seyad.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"xmqqmuy0ez8b.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T02:16:10Z","receivedAt":"2018-04-19T02:16:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A few commands that parse --expire=<time> command line option\nbehaves silly when given nonsense input.  For example\n\n    $ git prune --no-expire\n    Segmentation falut\n    $ git prune --expire=npw; echo $?\n    129\n\nBoth come from parse_opt_expiry_date_cb().\n\nThe former is because the function is not prepared to see arg==NULL\n(for \"--no-expire\", it is a norm; \"--expire\" at the end of the\ncommand line could be made to pass NULL, if it is told that the\nargument is optional, but we don't so we do not have to worry about\nthat case).\n\nThe latter is because it does not check the value returned from  the\nunderlying parse_expiry_date().  \n\nThis seems to be a recent regression introduced while we attempted\nto avoid spewing the entire usage message when given a correct\noption but with an invalid value at 3bb0923f (\"parse-options: do not\nshow usage upon invalid option value\", 2018-03-22).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I do not expect this to be the final version (not just it lacks\n   tests, but I haven't even run existing tests with the change\n   yet), but I think I diagnosed the root cause correctly, at least.\n\n parse-options-cb.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex c6679cb2cd..872627eafe 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n {\n-\treturn parse_expiry_date(arg, (timestamp_t *)opt->value);\n+\tif (unset)\n+\t\targ = \"never\";\n+\tif (parse_expiry_date(arg, (timestamp_t *)opt->value))\n+\t\tdie(\"malformed expiration date '%s'\", arg);\n+\treturn 0;\n }\n \n int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n"},{"id":"345051","messageId":"CA+55aFxSjUmbrocEr=FX=VOJ_-rtbOPYJ9mXFm+J60FKYtw9Vw@mail.gmail.com","threadId":"48323","inReplyTo":"xmqqr2ncezdc.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-04-19T02:19:32Z","receivedAt":"2018-04-19T02:19:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Apr 18, 2018 at 6:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Regardless of your originai \"git gc\" issue, we should make \"prune\"\n> say something on this error.  And when we do so, I would think that\n> error message will come before the final \"error: failed to run\n> prune\".\n\nSo to me, the real failure is the fact that it spent a a lot of time\npacking my repository before it then failed the prune at the end.\n\nI don't actually mind the quality of the error message too much -\nalthough it could be improved.\n\nI mind the \"oh, goddamnit, you just spent over a minute of CPU time on\nmy fairly high-end desktop, and _then_ you decided to tell me that I'm\na moron and couldn't type 'now' correctly\".\n\nSo to me, the big deal would be that builtin/gc.c should validate the\ndate *before* it starts, instead of doing all that work, and then\nexecuting \"git prune\" with invalid arguments..\n\n                Linus\n"},{"id":"345053","messageId":"CA+55aFztDdB9tVHREhQ7T0COs7p9ng81XfAHZCL3rx9WT2ecEQ@mail.gmail.com","threadId":"48323","inReplyTo":"xmqqfu3seyad.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2018-04-19T02:29:47Z","receivedAt":"2018-04-19T02:29:52Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Apr 18, 2018 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> A few commands that parse --expire=<time> command line option\n> behaves silly when given nonsense input.  For example\n\nSo this patch definitely improves on the error message.\n\nBut look at what happens for the kernel:\n\n    [torvalds@i7 linux]$ time git gc --prune=npw\n    Counting objects: 6006319, done.\n    Delta compression using up to 8 threads.\n    Compressing objects: 100% (912166/912166), done.\n    Writing objects: 100% (6006319/6006319), done.\n    Total 6006319 (delta 5050577), reused 6006319 (delta 5050577)\n    fatal: malformed expiration date 'npw'\n    error: failed to run prune\n\n    real        1m4.376s\n    user        0m59.963s\n    sys         0m5.182s\n\n\n\nYes, I get that nice \"malformed expiration date 'npw'\" error, but I\nget it after 64 seconds has passed.\n\nSo i think builtin/gc.c should use this same parse_expiry_date()\nparse_opt_expiry_date_cb() thing for its timestamp parsing.\n\nIt does actually seem to do that for the gc_log_expire value that it\nloads from the config file.\n\nMaybe something like the attached patch? Then I get:\n\n    [torvalds@i7 linux]$ time git gc --prune=npw\n    fatal: Failed to parse prune expiry value npw\n\n    real        0m0.004s\n    user        0m0.002s\n    sys         0m0.002s\n\nand you could smush it into your commit (if you want my sign-off, take it)\n\n              Linus\n\n\n builtin/gc.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3e67124ea..a4b20aaaf 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -354,6 +354,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tconst char *name;\n \tpid_t pid;\n \tint daemonized = 0;\n+\ttimestamp_t dummy;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -392,6 +393,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (argc > 0)\n \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n \n+\tif (parse_expiry_date(prune_expire, &dummy))\n+\t\tdie(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n+\n \tif (aggressive) {\n \t\targv_array_push(&repack, \"-f\");\n \t\tif (aggressive_depth > 0)\n"},{"id":"345056","messageId":"xmqq4lk7g9ry.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"CA+55aFztDdB9tVHREhQ7T0COs7p9ng81XfAHZCL3rx9WT2ecEQ@mail.gmail.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T03:22:41Z","receivedAt":"2018-04-19T03:22:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Yes, I get that nice \"malformed expiration date 'npw'\" error, but I\n> get it after 64 seconds has passed.\n\nAh, that timing aspect of the issue didn't occur to me.  The patch\nindeed is a reasonable workaround.\n\nThanks.\n\n\n\n"},{"id":"345063","messageId":"xmqqh8o7eq7j.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"CA+55aFztDdB9tVHREhQ7T0COs7p9ng81XfAHZCL3rx9WT2ecEQ@mail.gmail.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-19T05:10:40Z","receivedAt":"2018-04-19T05:10:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Maybe something like the attached patch? Then I get:\n> ...\n>     [torvalds@i7 linux]$ time git gc --prune=npw\n>     fatal: Failed to parse prune expiry value npw\n>\n>     real        0m0.004s\n>     user        0m0.002s\n>     sys         0m0.002s\n>\n> and you could smush it into your commit (if you want my sign-off, take it)\n>\n>               Linus\n>\n>  builtin/gc.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 3e67124ea..a4b20aaaf 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -354,6 +354,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \tconst char *name;\n>  \tpid_t pid;\n>  \tint daemonized = 0;\n> +\ttimestamp_t dummy;\n>  \n>  \tstruct option builtin_gc_options[] = {\n>  \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n> @@ -392,6 +393,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \tif (argc > 0)\n>  \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n>  \n> +\tif (parse_expiry_date(prune_expire, &dummy))\n> +\t\tdie(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n> +\n\nAt this point prune_expire could be NULL, so the if() needs a bit\ntightening, but otherwise it looks good.\n\nHere is the final one (at least for today).\n\n-- >8 --\nSubject: [PATCH] parseopt: handle malformed --expire arguments nicer\n\nA few commands that parse --expire=<time> command line option behave\nsilly when given nonsense input.  For example\n\n    $ git prune --no-expire\n    Segmentation falut\n    $ git prune --expire=npw; echo $?\n    129\n\nBoth come from parse_opt_expiry_date_cb().\n\nThe former is because the function is not prepared to see arg==NULL\n(for \"--no-expire\", it is a norm; \"--expire\" at the end of the\ncommand line could be made to pass NULL, if it is told that the\nargument is optional, but we don't so we do not have to worry about\nthat case).\n\nThe latter is because it does not check the value returned from  the\nunderlying parse_expiry_date().\n\nThis seems to be a recent regression introduced while we attempted\nto avoid spewing the entire usage message when given a correct\noption but with an invalid value at 3bb0923f (\"parse-options: do not\nshow usage upon invalid option value\", 2018-03-22).  Before that, we\ndidn't fail silently but showed a full usage help (which arguably is\nnot all that better).\n\nAlso catch this error early when \"git gc --prune=<expiration>\" is\nmisspelled by doing a dummy parsing before the main body of \"gc\"\nthat is time consuming even begins.  Otherwise, we'd spend time to\npack objects and then later have \"git prune\" first notice the error.\nAborting \"gc\" in the middle that way is not harmful but is ugly and\ncan be avoided.\n\nHelped-by: Linus Torvalds <torvalds@linux-foundation.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/gc.c       |  4 ++++\n parse-options-cb.c |  6 +++++-\n t/t5304-prune.sh   | 10 ++++++++++\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3c5eae0edf..858aa444e1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -353,6 +353,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tconst char *name;\n \tpid_t pid;\n \tint daemonized = 0;\n+\ttimestamp_t dummy;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (argc > 0)\n \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n \n+\tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n+\t\tdie(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n+\n \tif (aggressive) {\n \t\targv_array_push(&repack, \"-f\");\n \t\tif (aggressive_depth > 0)\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex c6679cb2cd..872627eafe 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n {\n-\treturn parse_expiry_date(arg, (timestamp_t *)opt->value);\n+\tif (unset)\n+\t\targ = \"never\";\n+\tif (parse_expiry_date(arg, (timestamp_t *)opt->value))\n+\t\tdie(\"malformed expiration date '%s'\", arg);\n+\treturn 0;\n }\n \n int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 6694c19a1e..af69cdc112 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -320,4 +320,14 @@ test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'prune: handle expire option correctly' '\n+\ttest_must_fail git prune --expire 2>error &&\n+\ttest_i18ngrep \"requires a value\" error &&\n+\n+\ttest_must_fail git prune --expire=nyah 2>error &&\n+\ttest_i18ngrep \"malformed expiration\" error &&\n+\n+\tgit prune --no-expire\n+'\n+\n test_done\n-- \n2.17.0-252-gfe0a9eaf31\n\n"},{"id":"345084","messageId":"20180419103447.GA19591@ruderich.org","threadId":"48323","inReplyTo":"xmqqr2ncezdc.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2018-04-19T10:34:47Z","receivedAt":"2018-04-19T10:39:54Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote:\n> It turns out that prune silently goes away given a bad expiry\n>\n>     $ git prune --expire=nyah ; echo $?\n>     129\n\nI noticed that git log --since/--after/--before/--until have a\nsimilar behavior and ignore date parsing errors in those options\ncompletely. Is this expected or should we warn the user with\nsomething like the following?\n\ndiff --git a/revision.c b/revision.c\nindex 4e0e193e57..e5ba6c7dfc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_age = atoi(optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"since\", argv, &optarg))) {\n-\t\trevs->max_age = approxidate(optarg);\n+\t\tint err = 0;\n+\t\trevs->max_age = approxidate_careful(optarg, &err);\n+\t\tif (err)\n+\t\t\treturn error(\"--since: invalid time '%s'\", optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"after\", argv, &optarg))) {\n-\t\trevs->max_age = approxidate(optarg);\n+\t\tint err = 0;\n+\t\trevs->max_age = approxidate_careful(optarg, &err);\n+\t\tif (err)\n+\t\t\treturn error(\"--after: invalid time '%s'\", optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"min-age\", argv, &optarg))) {\n \t\trevs->min_age = atoi(optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"before\", argv, &optarg))) {\n-\t\trevs->min_age = approxidate(optarg);\n+\t\tint err = 0;\n+\t\trevs->min_age = approxidate_careful(optarg, &err);\n+\t\tif (err)\n+\t\t\treturn error(\"--before: invalid time '%s'\", optarg);\n \t\treturn argcount;\n \t} else if ((argcount = parse_long_opt(\"until\", argv, &optarg))) {\n-\t\trevs->min_age = approxidate(optarg);\n+\t\tint err = 0;\n+\t\trevs->min_age = approxidate_careful(optarg, &err);\n+\t\tif (err)\n+\t\t\treturn error(\"--until: invalid time '%s'\");\n \t\treturn argcount;\n \t} else if (!strcmp(arg, \"--first-parent\")) {\n \t\trevs->first_parent_only = 1;\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"345180","messageId":"xmqqlgdid99h.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"20180419103447.GA19591@ruderich.org","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-20T00:14:18Z","receivedAt":"2018-04-20T00:14:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich <simon@ruderich.org> writes:\n\n> On Thu, Apr 19, 2018 at 10:52:47AM +0900, Junio C Hamano wrote:\n>> It turns out that prune silently goes away given a bad expiry\n>>\n>>     $ git prune --expire=nyah ; echo $?\n>>     129\n>\n> I noticed that git log --since/--after/--before/--until have a\n> similar behavior and ignore date parsing errors in those options\n> completely. Is this expected or should we warn the user with\n> something like the following?\n\nI do not have a strong opinion on this, because I would expect that\n\"git log --since=nyah\" to do whatever random things it may want to\ndo.\n\nBut I suspect I am a minority, and if we were to change the\nestablished behaviour, I agree that erroring out using\napproxidate_careful() is the right direction to go in.\n\nThanks.\n\n> diff --git a/revision.c b/revision.c\n> index 4e0e193e57..e5ba6c7dfc 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1794,19 +1794,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\trevs->max_age = atoi(optarg);\n>  \t\treturn argcount;\n>  \t} else if ((argcount = parse_long_opt(\"since\", argv, &optarg))) {\n> -\t\trevs->max_age = approxidate(optarg);\n> +\t\tint err = 0;\n> +\t\trevs->max_age = approxidate_careful(optarg, &err);\n> +\t\tif (err)\n> +\t\t\treturn error(\"--since: invalid time '%s'\", optarg);\n>  \t\treturn argcount;\n>  \t} else if ((argcount = parse_long_opt(\"after\", argv, &optarg))) {\n> -\t\trevs->max_age = approxidate(optarg);\n> +\t\tint err = 0;\n> +\t\trevs->max_age = approxidate_careful(optarg, &err);\n> +\t\tif (err)\n> +\t\t\treturn error(\"--after: invalid time '%s'\", optarg);\n>  \t\treturn argcount;\n>  \t} else if ((argcount = parse_long_opt(\"min-age\", argv, &optarg))) {\n>  \t\trevs->min_age = atoi(optarg);\n>  \t\treturn argcount;\n>  \t} else if ((argcount = parse_long_opt(\"before\", argv, &optarg))) {\n> -\t\trevs->min_age = approxidate(optarg);\n> +\t\tint err = 0;\n> +\t\trevs->min_age = approxidate_careful(optarg, &err);\n> +\t\tif (err)\n> +\t\t\treturn error(\"--before: invalid time '%s'\", optarg);\n>  \t\treturn argcount;\n>  \t} else if ((argcount = parse_long_opt(\"until\", argv, &optarg))) {\n> -\t\trevs->min_age = approxidate(optarg);\n> +\t\tint err = 0;\n> +\t\trevs->min_age = approxidate_careful(optarg, &err);\n> +\t\tif (err)\n> +\t\t\treturn error(\"--until: invalid time '%s'\");\n>  \t\treturn argcount;\n>  \t} else if (!strcmp(arg, \"--first-parent\")) {\n>  \t\trevs->first_parent_only = 1;\n>\n> Regards\n> Simon\n"},{"id":"345191","messageId":"20180420072701.GB13462@ruderich.org","threadId":"48323","inReplyTo":"xmqqh8o7eq7j.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2018-04-20T07:27:01Z","receivedAt":"2018-04-20T07:27:08Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Apr 19, 2018 at 02:10:40PM +0900, Junio C Hamano wrote:\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> index c6679cb2cd..872627eafe 100644\n> --- a/parse-options-cb.c\n> +++ b/parse-options-cb.c\n> @@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n>  int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n>  \t\t\t     int unset)\n>  {\n> -\treturn parse_expiry_date(arg, (timestamp_t *)opt->value);\n> +\tif (unset)\n> +\t\targ = \"never\";\n> +\tif (parse_expiry_date(arg, (timestamp_t *)opt->value))\n> +\t\tdie(\"malformed expiration date '%s'\", arg);\n> +\treturn 0;\n>  }\n\nShould this error get translated?\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"345295","messageId":"xmqqlgdhb6ba.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"20180420072701.GB13462@ruderich.org","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-21T03:13:13Z","receivedAt":"2018-04-21T03:13:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich <simon@ruderich.org> writes:\n\n> On Thu, Apr 19, 2018 at 02:10:40PM +0900, Junio C Hamano wrote:\n>> diff --git a/parse-options-cb.c b/parse-options-cb.c\n>> index c6679cb2cd..872627eafe 100644\n>> --- a/parse-options-cb.c\n>> +++ b/parse-options-cb.c\n>> @@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n>>  int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n>>  \t\t\t     int unset)\n>>  {\n>> -\treturn parse_expiry_date(arg, (timestamp_t *)opt->value);\n>> +\tif (unset)\n>> +\t\targ = \"never\";\n>> +\tif (parse_expiry_date(arg, (timestamp_t *)opt->value))\n>> +\t\tdie(\"malformed expiration date '%s'\", arg);\n>> +\treturn 0;\n>>  }\n>\n> Should this error get translated?\n\nSure.  The new test to check this codepath even protects itself from\nsuch a translation by using test_i18ngrep, so this is safe to mark\nfor translation from day one.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH v2] parseopt: handle malformed --expire arguments more nicely\n\nA few commands that parse --expire=<time> command line option behave\nsillily when given nonsense input.  For example\n\n    $ git prune --no-expire\n    Segmentation falut\n    $ git prune --expire=npw; echo $?\n    129\n\nBoth come from parse_opt_expiry_date_cb().\n\nThe former is because the function is not prepared to see arg==NULL\n(for \"--no-expire\", it is a norm; \"--expire\" at the end of the\ncommand line could be made to pass NULL, if it is told that the\nargument is optional, but we don't so we do not have to worry about\nthat case).\n\nThe latter is because it does not check the value returned from the\nunderlying parse_expiry_date().\n\nThis seems to be a recent regression introduced while we attempted\nto avoid spewing the entire usage message when given a correct\noption but with an invalid value at 3bb0923f (\"parse-options: do not\nshow usage upon invalid option value\", 2018-03-22).  Before that, we\ndidn't fail silently but showed a full usage help (which arguably is\nnot all that better).\n\nAlso catch this error early when \"git gc --prune=<expiration>\" is\nmisspelled by doing a dummy parsing before the main body of \"gc\"\nthat is time consuming even begins.  Otherwise, we'd spend time to\npack objects and then later have \"git prune\" first notice the error.\nAborting \"gc\" in the middle that way is not harmful but is ugly and\ncan be avoided.\n\nHelped-by: Linus Torvalds <torvalds@linux-foundation.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * marking the new message in parse_opt_expiry_date_cb() function\n   for i18n is the only change from the previous round.\n\n builtin/gc.c       |  4 ++++\n parse-options-cb.c |  6 +++++-\n t/t5304-prune.sh   | 10 ++++++++++\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 3c5eae0edf..858aa444e1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -353,6 +353,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tconst char *name;\n \tpid_t pid;\n \tint daemonized = 0;\n+\ttimestamp_t dummy;\n \n \tstruct option builtin_gc_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress progress reporting\")),\n@@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tif (argc > 0)\n \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n \n+\tif (prune_expire && parse_expiry_date(prune_expire, &dummy))\n+\t\tdie(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n+\n \tif (aggressive) {\n \t\targv_array_push(&repack, \"-f\");\n \t\tif (aggressive_depth > 0)\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nindex c6679cb2cd..0f9f311a7a 100644\n--- a/parse-options-cb.c\n+++ b/parse-options-cb.c\n@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n {\n-\treturn parse_expiry_date(arg, (timestamp_t *)opt->value);\n+\tif (unset)\n+\t\targ = \"never\";\n+\tif (parse_expiry_date(arg, (timestamp_t *)opt->value))\n+\t\tdie(_(\"malformed expiration date '%s'\"), arg);\n+\treturn 0;\n }\n \n int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 6694c19a1e..af69cdc112 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -320,4 +320,14 @@ test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'prune: handle expire option correctly' '\n+\ttest_must_fail git prune --expire 2>error &&\n+\ttest_i18ngrep \"requires a value\" error &&\n+\n+\ttest_must_fail git prune --expire=nyah 2>error &&\n+\ttest_i18ngrep \"malformed expiration\" error &&\n+\n+\tgit prune --no-expire\n+'\n+\n test_done\n\n\n\n"},{"id":"345309","messageId":"CAP8UFD0_LvWkKSYd1MWrdoK-tqBcQqm32CUNrBqEfmjWdHkJgg@mail.gmail.com","threadId":"48323","inReplyTo":"xmqqlgdhb6ba.fsf@gitster-ct.c.googlers.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-04-21T06:07:01Z","receivedAt":"2018-04-21T06:07:05Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> @@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>         if (argc > 0)\n>                 usage_with_options(builtin_gc_usage, builtin_gc_options);\n>\n> +       if (prune_expire && parse_expiry_date(prune_expire, &dummy))\n> +               die(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n\nMicronit: I thought we prefer error messages to start with a lower\ncase letter, like:\n\n               die(_(\"failed to parse prune expiry value %s\"), prune_expire);\n"},{"id":"345461","messageId":"xmqqbmea9h6u.fsf@gitster-ct.c.googlers.com","threadId":"48323","inReplyTo":"CAP8UFD0_LvWkKSYd1MWrdoK-tqBcQqm32CUNrBqEfmjWdHkJgg@mail.gmail.com","subject":"Re: Silly \"git gc\" UI issue.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-23T13:38:01Z","receivedAt":"2018-04-23T13:38:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> @@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>>         if (argc > 0)\n>>                 usage_with_options(builtin_gc_usage, builtin_gc_options);\n>>\n>> +       if (prune_expire && parse_expiry_date(prune_expire, &dummy))\n>> +               die(_(\"Failed to parse prune expiry value %s\"), prune_expire);\n>\n> Micronit: I thought we prefer error messages to start with a lower\n> case letter, like:\n>\n>                die(_(\"failed to parse prune expiry value %s\"), prune_expire);\n\nThanks.\n\nThere is an existing \"Failed...\" already before the pre-context of\nthis hunk, which I'll fix with a preliminary clean-up patch.\n\n\n\n\n"}]}