Re: [PATCH v2] bugreport: reject positional arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 26, 2023, 03:43 UTC
Message-ID: <CAPig+cT4G9vdu+se9Fbbs0TRCyPoAYFgVtkSwph_U=sWf-kQ9g@mail.gmail.com>
In-Reply-To: <20231026005542.872301-1-nasamuffin@google.com>

On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:

Show 12 quoted lines

> 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.

Show 7 quoted lines

> 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.