From: Dragan Simic <dsimic@manjaro.org>
Date: Thu, 26 Oct 2023 04:06:49 GMT
Subject: Re: [PATCH v2] bugreport: reject positional arguments
Message-ID: <aa11162c3220304b91ef25f30600c231@manjaro.org>
In-Reply-To: <CAPig+cTJFKp6RFdqJTpyL49V+M-zaTDbgpVd2OrgfWf4H-+K+g@mail.gmail.com>

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 `'