Re: getopt error message with a bare "-"

2023-12-02 Thread Daniel Sahlberg
Den lör 2 dec. 2023 kl 11:08 skrev Graham Leggett :

> On 28 Nov 2023, at 07:46, Daniel Sahlberg 
> 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.c2023-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


>


Re: getopt error message with a bare "-"

2023-12-02 Thread Graham Leggett via dev
On 28 Nov 2023, at 07:46, Daniel Sahlberg  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.c2023-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
—