Re: [PATCH v2] bugreport: reject positional arguments

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 25, 2023 at 11:52 PM Dragan Simic <dsimic@xxxxxxxxxxx> wrote:
> On 2023-10-26 05:43, Eric Sunshine wrote:
> > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@xxxxxxxxxx> 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 `'





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux