Den lör 2 dec. 2023 kl 11:08 skrev Graham Leggett <minf...@sharp.fm>:
> 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 :) > 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 >
