Re: getopt error message with a bare "-"

Den lör 2 dec. 2023 kl 11:08 skrev Graham Leggett <[email protected]>:

> On 28 Nov 2023, at 07:46, Daniel Sahlberg <[email protected]>
> wrote:
>
> Subversion is using APR's apr_getopt_long for command line processing.
>
> A user tried to supply "-" for the filename with the intention of using
> stdin (Subversion has support for this in our code). However the user got
> the following error message (with the first line coming from
> apr_getopt_long and the second from Subversion's code):
>
> $ ./svnmucc put - file:///url/to/repository
> svnmucc: invalid option:
> Type 'svnmucc --help' for usage.
>
> We found the solution, to use "--" to disable option processing, so no
> discussion around this is needed.
>
> In the thread, someone mentioned that it would be useful if the error
> message could be improved so I looked into the APR code and have a
> suggestion.
>
> The following code is responsible for the error message. In other places,
> the error message contains the offending (long) option (third argument).
> However since *p is always \0, checked just above, the error message will
> just say "invalid option". I'm suggesting to change this to the fixed
> string "-" instead.
> [[[
> --- getopt.orig 2023-11-28 08:22:16.690508444 +0100
> +++ getopt.c    2023-11-28 08:22:50.270297468 +0100
> @@ -272,7 +272,7 @@
>              }
>              else
>                  if (*p == '\0')                    /* Bare "-" is illegal
> */
> -                    return serr(os, "invalid option", p, APR_BADCH);
> +                    return serr(os, "invalid option", "-", APR_BADCH);
>          }
>      }
> ]]]
>
> With this change, the error message will instead be:
> $ ./svnmucc put - file:///url/to/repository
> svnmucc: invalid option: -
> Type 'svnmucc --help' for usage.
>
> with the "-" indicating which part of the command line is invalid.
>
> Original e-mail from our users@ list:
> https://lists.apache.org/thread/mbsx7rxpx07sh6sfs6xhbnvgo2wbfl0k
>
> Follow-up e-mail in our dev@ list:
> https://lists.apache.org/thread/l7x22yp6kb71qlv3rn8tfmcrc5hk73r4
>
>
> Waves hi :)
>

Oh, hi! :)   (Graham is the user I mentioned above, didn't know you were
here ... )


>
> +1 on the above message fix at the very least. I’ll commit it if there are
> no objections.
>
> Another solution would be to add a switch to getopt.c that allows for - to
> be counted not as a "start of short option string" but as a separate
> argument, possibly hidden behind a switch in the apr_getopt_t struct. For
> our use, that would be even better, but I assume it would require more code
> (also on our side to remain backwards compatible with older APR versions).
>
>
> Looks like apr_getopt_t is the right place for a flag. It seems a lot more
> intuitive to have “-“ recognised as an argument rather than an invalid
> option.
>

That's exactly how I saw it as well. Not sure if we (Subversion) would even
make use of that option for a long time, since we need to be reasonably
sure an uptodate version of APR is available before we release.


>
> Regards,
> Graham
> —
>

Kind regards
Daniel


>