(Re-sent, this time with the patch.)
Hyrum K. Wright wrote:
> Julian Foad wrote:
>
>>Incorrect usage should result in an error message, consistently for all
>>commands. It's been on my "to do" list for years. Perhaps now I'll do it.
>
> Sounds reasonable. Here's a patch to print an error instead of the
> online help for the move command.
>
> -Hyrum
>
> [[[
> * subversion/clients/cmdline/move-cmd.c
> (svn_cl__move): Change the error from NULL to something useful, thus
> preventing the display of the online help when there aren't enough
> arguments.
[...]
> - return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
> + return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0,
> + _("No source and/or destination path given"));
Well, that's one way to do it. I was thinking, instead, of removing all the
code that prints extra help, as in the attached patch. (My favourite type of
programming: removing needless complexity. :-)
The only concerns I have with my patch are (1) some people are likely to be
uncomfortable with the whole idea, so we need a little time for consultation;
and (2) some of the error messages that will now appear may be not as good as
we would wish, but I think we can improve them separately.
- Julian
On argument-parsing errors, don't print help, just an error message.
The errors in question are mainly for too few or too many arguments.
Printing help may be more helpful to a novice in the short term, but in the
long term it gives a false sense of security, causing users to develop a bad
habit. It's bad because not all commands print help when invoked without
arguments. Also, when invoking the command accidentally without arguments
(perhaps giving as an argument an environment variable which was accidentally
undefined), printing a long help message is annoying. Also it makes a
compatibility problem if we want to extend a command to have an argument-free
form in the future.
* subversion/clients/cmdline/main.c (main):
Don't replace a generic argument-parsing error with help.
* subversion/clients/cmdline/merge-cmd.c (svn_cl__merge):
* subversion/svnadmin/main.c (main):
* subversion/svndumpfilter/main.c (main):
* subversion/svnlook/main.c (main):
Don't augment argument-parsing errors with help.
Index: subversion/clients/cmdline/main.c
===================================================================
--- subversion/clients/cmdline/main.c (revision 17245)
+++ subversion/clients/cmdline/main.c (working copy)
@@ -1425,16 +1425,7 @@ main (int argc, const char * const *argv
{
svn_error_t *tmp_err;
- /* If we got an SVN_ERR_CL_ARG_PARSING_ERROR with no useful content
- (i.e. a NULL or empty message), display help, otherwise just display
- the errors as usual, since the error output will probably be just as
- much help to them as our help output, if not more. */
- if (err->apr_err == SVN_ERR_CL_ARG_PARSING_ERROR
- && (err->message == NULL || err->message[0] == '\0'))
- svn_opt_subcommand_help (subcommand->name, svn_cl__cmd_table,
- svn_cl__options, pool);
- else
- svn_handle_error2 (err, stderr, FALSE, "svn: ");
+ svn_handle_error2 (err, stderr, FALSE, "svn: ");
/* Tell the user about 'svn cleanup' if any error on the stack
was about locked working copies. */
Index: subversion/clients/cmdline/merge-cmd.c
===================================================================
--- subversion/clients/cmdline/merge-cmd.c (revision 17245)
+++ subversion/clients/cmdline/merge-cmd.c (working copy)
@@ -54,32 +54,19 @@ svn_cl__merge (apr_getopt_t *os,
{
/* sanity check: they better have given supplied a *range*. */
if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
- {
- svn_opt_subcommand_help ("merge", svn_cl__cmd_table,
- svn_cl__options, pool);
- return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
- _("Second revision required"));
- }
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
+ _("Second revision required"));
using_alternate_syntax = TRUE;
}
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
opt_state->targets, pool));
- /* If there are no targets at all, then let's just give the user a
- friendly help message, rather than spewing an error. */
- if (targets->nelts == 0)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
-
if (using_alternate_syntax)
{
if ((targets->nelts < 1) || (targets->nelts > 2))
- {
- svn_opt_subcommand_help ("merge", svn_cl__cmd_table,
- svn_cl__options, pool);
- return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
- _("Wrong number of paths given"));
- }
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
+ _("Wrong number of paths given"));
SVN_ERR (svn_opt_parse_path (&peg_revision, &sourcepath1,
((const char **)(targets->elts))[0], pool));
@@ -99,12 +86,8 @@ svn_cl__merge (apr_getopt_t *os,
else /* using @rev syntax */
{
if ((targets->nelts < 2) || (targets->nelts > 3))
- {
- svn_opt_subcommand_help ("merge", svn_cl__cmd_table,
- svn_cl__options, pool);
- return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
- _("Wrong number of paths given"));
- }
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0,
+ _("Wrong number of paths given"));
/* the first two paths become the 'sources' */
SVN_ERR (svn_opt_parse_path (&opt_state->start_revision, &sourcepath1,
Index: subversion/svnadmin/main.c
===================================================================
--- subversion/svnadmin/main.c (revision 17245)
+++ subversion/svnadmin/main.c (working copy)
@@ -1356,8 +1356,6 @@ main (int argc, const char * const *argv
if(err)
{
svn_handle_error2 (err, stderr, FALSE, "svnadmin: ");
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
svn_error_clear (err);
svn_pool_destroy (pool);
return EXIT_FAILURE;
@@ -1376,8 +1374,6 @@ main (int argc, const char * const *argv
if(err)
{
svn_handle_error2 (err, stderr, FALSE, "svnadmin: ");
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
svn_error_clear (err);
svn_pool_destroy (pool);
return EXIT_FAILURE;
@@ -1424,14 +1420,7 @@ main (int argc, const char * const *argv
err = (*subcommand->cmd_func) (os, &opt_state, pool);
if (err)
{
- if (err->apr_err == SVN_ERR_CL_ARG_PARSING_ERROR)
- {
- svn_handle_error2 (err, stderr, FALSE, "svnadmin: ");
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
- }
- else
- svn_handle_error2 (err, stderr, FALSE, "svnadmin: ");
+ svn_handle_error2 (err, stderr, FALSE, "svnadmin: ");
svn_error_clear (err);
svn_pool_destroy (pool);
return EXIT_FAILURE;
Index: subversion/svndumpfilter/main.c
===================================================================
--- subversion/svndumpfilter/main.c (revision 17245)
+++ subversion/svndumpfilter/main.c (working copy)
@@ -1227,8 +1227,6 @@ main (int argc, const char * const *argv
svn_error_clear (svn_cmdline_fprintf
(stderr, pool,
_("\nError: no prefixes supplied.\n")));
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
svn_pool_destroy (pool);
return EXIT_FAILURE;
}
@@ -1281,16 +1279,7 @@ main (int argc, const char * const *argv
err = (*subcommand->cmd_func) (os, &opt_state, pool);
if (err)
{
- if (err->apr_err == SVN_ERR_CL_ARG_PARSING_ERROR)
- {
- svn_handle_error2 (err, stderr, FALSE, "svndumpfilter: ");
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
- }
- else
- {
- svn_handle_error2 (err, stderr, FALSE, "svndumpfilter: ");
- }
+ svn_handle_error2 (err, stderr, FALSE, "svndumpfilter: ");
svn_pool_destroy (pool);
return EXIT_FAILURE;
}
Index: subversion/svnlook/main.c
===================================================================
--- subversion/svnlook/main.c (revision 17245)
+++ subversion/svnlook/main.c (working copy)
@@ -2281,14 +2281,7 @@ main (int argc, const char * const *argv
err = (*subcommand->cmd_func) (os, &opt_state, pool);
if (err)
{
- if (err->apr_err == SVN_ERR_CL_ARG_PARSING_ERROR)
- {
- svn_handle_error2 (err, stderr, FALSE, "svnlook: ");
- svn_opt_subcommand_help (subcommand->name, cmd_table,
- options_table, pool);
- }
- else
- svn_handle_error2 (err, stderr, FALSE, "svnlook: ");
+ svn_handle_error2 (err, stderr, FALSE, "svnlook: ");
svn_error_clear (err);
svn_pool_destroy (pool);
return EXIT_FAILURE;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 8 18:52:01 2005