On 28 Nov 2023, at 07:46, Daniel Sahlberg <daniel.l.sahlb...@gmail.com> 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 :) +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. Regards, Graham —
