| Message ID | 20231026005542.872301-1-nasamuffin@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] bugreport: reject positional arguments | expand |
On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote: > git-bugreport already rejected unrecognized flag arguments, like > `--diaggnose`, but this doesn't help if the user's mistake was to forget > the `--` in front of the argument. This can result in a user's intended > argument not being parsed with no indication to the user that something > went wrong. Since git-bugreport presently doesn't take any positionals > at all, let's reject all positionals and give the user a usage hint. > > Signed-off-by: Emily Shaffer <nasamuffin@google.com> > --- > Per Eric's suggestion, added a citation of the first positional arg > found. I don't think it's necessary to unroll the entire argv array > here, though. Thanks. I had the same thought about the first positional argument being sufficient since it should provide enough context on its own. > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) > + if (argc) { > + if (argv[0]) > + error(_("unknown argument `%s'"), argv[0]); > + usage(bugreport_usage[0]); > + } Can it actually happen that argc is non-zero but argv[0] is NULL? (I don't have parse-options in front of me to check.) If not, then the extra `if (argv[0])` conditional may confuse future readers.
On 2023-10-26 05:43, Eric Sunshine wrote: > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote: >> diff --git a/builtin/bugreport.c b/builtin/bugreport.c >> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, >> const char *prefix) >> + if (argc) { >> + if (argv[0]) >> + error(_("unknown argument `%s'"), argv[0]); >> + usage(bugreport_usage[0]); >> + } > > Can it actually happen that argc is non-zero but argv[0] is NULL? (I > don't have parse-options in front of me to check.) If not, then the > extra `if (argv[0])` conditional may confuse future readers. According to https://stackoverflow.com/a/2794171/22330192 it can't, but argv[0] can be a zero-length string.
On Wed, Oct 25, 2023 at 11:52 PM Dragan Simic <dsimic@manjaro.org> wrote: > On 2023-10-26 05:43, Eric Sunshine wrote: > > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote: > >> diff --git a/builtin/bugreport.c b/builtin/bugreport.c > >> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, > >> const char *prefix) > >> + if (argc) { > >> + if (argv[0]) > >> + error(_("unknown argument `%s'"), argv[0]); > >> + usage(bugreport_usage[0]); > >> + } > > > > Can it actually happen that argc is non-zero but argv[0] is NULL? (I > > don't have parse-options in front of me to check.) If not, then the > > extra `if (argv[0])` conditional may confuse future readers. > > According to https://stackoverflow.com/a/2794171/22330192 it can't, but > argv[0] can be a zero-length string. This case is different, though, since, by this point, argv[] has been processed by Git's parse-options API. Here's the relevant comment from parse-options.h: * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and * skipped. * * Returns the number of arguments left in argv[]. So, I think the `if (argv[0])` conditional is unnecessary, thus potentially confusing. It's possible that Emily meant `if (*argv[0])`, but even that seems undesirable since even a zero-length argv[0] provides some useful context. % git bugreport "" error: unknown argument `'
On 2023-10-26 06:03, Eric Sunshine wrote: > On Wed, Oct 25, 2023 at 11:52 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2023-10-26 05:43, Eric Sunshine wrote: >> > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote: >> >> diff --git a/builtin/bugreport.c b/builtin/bugreport.c >> >> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, >> >> const char *prefix) >> >> + if (argc) { >> >> + if (argv[0]) >> >> + error(_("unknown argument `%s'"), argv[0]); >> >> + usage(bugreport_usage[0]); >> >> + } >> > >> > Can it actually happen that argc is non-zero but argv[0] is NULL? (I >> > don't have parse-options in front of me to check.) If not, then the >> > extra `if (argv[0])` conditional may confuse future readers. >> >> According to https://stackoverflow.com/a/2794171/22330192 it can't, >> but >> argv[0] can be a zero-length string. > > This case is different, though, since, by this point, argv[] has been > processed by Git's parse-options API. Here's the relevant comment from > parse-options.h: Ah, I see, thanks for the clarification. > * parse_options() will filter out the processed options and leave > the > * non-option arguments in argv[]. argv0 is assumed program name and > * skipped. > * > * Returns the number of arguments left in argv[]. > > So, I think the `if (argv[0])` conditional is unnecessary, thus > potentially confusing. > > It's possible that Emily meant `if (*argv[0])`, but even that seems > undesirable since even a zero-length argv[0] provides some useful > context. > > % git bugreport "" > error: unknown argument `'
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index d2ae5c305d..8a69a23397 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, bugreport_options, bugreport_usage, 0); + if (argc) { + if (argv[0]) + error(_("unknown argument `%s'"), argv[0]); + usage(bugreport_usage[0]); + } + /* Prepare the path to put the result */ prefixed_filename = prefix_filename(prefix, option_output ? option_output : ""); diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index f6998269be..5b1b3e8d07 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' ' test_path_is_missing git-bugreport-* ' +test_expect_success 'incorrect positional arguments abort with usage and hint' ' + test_must_fail git bugreport false 2>output && + test_i18ngrep usage output && + test_i18ngrep false output && + test_path_is_missing git-bugreport-* +' + test_expect_success 'runs outside of a git dir' ' test_when_finished rm non-repo/git-bugreport-* && nongit git bugreport