* Silly "git gc" UI issue. @ 2018-04-19 1:45 Linus Torvalds 2018-04-19 1:52 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Linus Torvalds @ 2018-04-19 1:45 UTC (permalink / raw) To: Git Mailing List 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 1:45 Silly "git gc" UI issue Linus Torvalds @ 2018-04-19 1:52 ` Junio C Hamano 2018-04-19 1:55 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Junio C Hamano @ 2018-04-19 1:52 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 1:52 ` Junio C Hamano @ 2018-04-19 1:55 ` Junio C Hamano 2018-04-19 2:16 ` Junio C Hamano 2018-04-19 2:19 ` Linus Torvalds 2018-04-19 10:34 ` Simon Ruderich 2 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2018-04-19 1:55 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 1:55 ` Junio C Hamano @ 2018-04-19 2:16 ` Junio C Hamano 2018-04-19 2:29 ` Linus Torvalds 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2018-04-19 2:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Linus Torvalds, Paul-Sebastian Ungureanu 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, ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 2:16 ` Junio C Hamano @ 2018-04-19 2:29 ` Linus Torvalds 2018-04-19 3:22 ` Junio C Hamano 2018-04-19 5:10 ` Junio C Hamano 0 siblings, 2 replies; 14+ messages in thread From: Linus Torvalds @ 2018-04-19 2:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Paul-Sebastian Ungureanu [-- 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) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 2:29 ` Linus Torvalds @ 2018-04-19 3:22 ` Junio C Hamano 2018-04-19 5:10 ` Junio C Hamano 1 sibling, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2018-04-19 3:22 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List, Paul-Sebastian Ungureanu 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 2:29 ` Linus Torvalds 2018-04-19 3:22 ` Junio C Hamano @ 2018-04-19 5:10 ` Junio C Hamano 2018-04-20 7:27 ` Simon Ruderich 1 sibling, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2018-04-19 5:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: Git Mailing List, Paul-Sebastian Ungureanu 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 5:10 ` Junio C Hamano @ 2018-04-20 7:27 ` Simon Ruderich 2018-04-21 3:13 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Simon Ruderich @ 2018-04-20 7:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List, Paul-Sebastian Ungureanu 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-20 7:27 ` Simon Ruderich @ 2018-04-21 3:13 ` Junio C Hamano 2018-04-21 6:07 ` Christian Couder 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2018-04-21 3:13 UTC (permalink / raw) To: Simon Ruderich; +Cc: Linus Torvalds, Git Mailing List, Paul-Sebastian Ungureanu 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-21 3:13 ` Junio C Hamano @ 2018-04-21 6:07 ` Christian Couder 2018-04-23 13:38 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Christian Couder @ 2018-04-21 6:07 UTC (permalink / raw) To: Junio C Hamano Cc: Simon Ruderich, Linus Torvalds, Git Mailing List, Paul-Sebastian Ungureanu 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-21 6:07 ` Christian Couder @ 2018-04-23 13:38 ` Junio C Hamano 0 siblings, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2018-04-23 13:38 UTC (permalink / raw) To: Christian Couder Cc: Simon Ruderich, Linus Torvalds, Git Mailing List, Paul-Sebastian Ungureanu 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 1:52 ` Junio C Hamano 2018-04-19 1:55 ` Junio C Hamano @ 2018-04-19 2:19 ` Linus Torvalds 2018-04-19 10:34 ` Simon Ruderich 2 siblings, 0 replies; 14+ messages in thread From: Linus Torvalds @ 2018-04-19 2:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 1:52 ` Junio C Hamano 2018-04-19 1:55 ` Junio C Hamano 2018-04-19 2:19 ` Linus Torvalds @ 2018-04-19 10:34 ` Simon Ruderich 2018-04-20 0:14 ` Junio C Hamano 2 siblings, 1 reply; 14+ messages in thread From: Simon Ruderich @ 2018-04-19 10:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: Silly "git gc" UI issue. 2018-04-19 10:34 ` Simon Ruderich @ 2018-04-20 0:14 ` Junio C Hamano 0 siblings, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2018-04-20 0:14 UTC (permalink / raw) To: Simon Ruderich; +Cc: Linus Torvalds, Git Mailing List 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-04-23 13:38 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-19 1:45 Silly "git gc" UI issue Linus Torvalds 2018-04-19 1:52 ` Junio C Hamano 2018-04-19 1:55 ` Junio C Hamano 2018-04-19 2:16 ` Junio C Hamano 2018-04-19 2:29 ` Linus Torvalds 2018-04-19 3:22 ` Junio C Hamano 2018-04-19 5:10 ` Junio C Hamano 2018-04-20 7:27 ` Simon Ruderich 2018-04-21 3:13 ` Junio C Hamano 2018-04-21 6:07 ` Christian Couder 2018-04-23 13:38 ` Junio C Hamano 2018-04-19 2:19 ` Linus Torvalds 2018-04-19 10:34 ` Simon Ruderich 2018-04-20 0:14 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.