Re: Silly "git gc" UI issue.
Christian Couder writes:
> On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano 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.
On Sat, Apr 21, 2018 at 5:13 AM, Junio C Hamano 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.
Simon Ruderich 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= 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=" 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
Signed-off-by: Junio C Hamano
---
* 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.
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.
Simon Ruderich 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
Re: Silly "git gc" UI issue.
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.
Linus Torvalds 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
>
> real0m0.004s
> user0m0.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= 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=" 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
Signed-off-by: Junio C Hamano
---
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 &&
+
Re: Silly "git gc" UI issue.
Linus Torvalds 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.
On Wed, Apr 18, 2018 at 7:16 PM, Junio C Hamano wrote:
> A few commands that parse --expire= 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
real1m4.376s
user0m59.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
real0m0.004s
user0m0.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)
Re: Silly "git gc" UI issue.
On Wed, Apr 18, 2018 at 6:52 PM, Junio C Hamano 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.
A few commands that parse --expire= 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
---
* 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.
Junio C Hamano 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.
Linus Torvalds 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.
Silly "git gc" UI issue.
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
