Re: getopt error message with a bare "-"

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

+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
—