Hi,

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

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).

Kind regards,
Daniel Sahlberg

Reply via email to