I want to commit this. Do I have any supporters or objectors? It goes
hand-in-hand with my patch
<http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=108158> to stop
printing help as well as or instead of an error message. The present patch
goes some way towards that same goal.
Hyrum K. Wright wrote:
> In many places in the command line client application, we return a
> SVN_ERR_CL_ARG_PARSING_ERR when we try to run a command that needs
> arguments. One notable except is the merge command, which returns a
> SVN_ERR_CL_INSUFFICIENT_ARGS error instead. This more closely matches
> the nature of the error, we haven't errored because we can't parse the
> arguments, we've errored because there are an insufficient number of them.
Excellent. Thanks for this.
> Because this error occurs when the user hasn't fed us enough arguments,
> the generic error message has also been changed to suggest they run help
> for more information.
I wasn't sure at first that I liked that change. There are a lot of errors
where we could suggest reading the help. What's special about this one? Well,
it is the one that results if no arguments are given, and that is likely to be
a common action of new people who need help, so OK, it does make some sense to
add the suggestion just to this particular error message. I've tweaked the
message slightly.
> Also, the method of checking the number of arguments was a little
> inconsistent between commands, so I've fixed that as well.
Well, partly; you changed "nelts < 1" to "! nelts" in one place, but not where
it is accompanied by a "nelts > 2". But that's OK: it's a bit more consistent
than it was.
> This patch complements Julian Foad's patch[1], because it causes a
> semi-useful error message to be displayed, instead of running the
> command help when no arguments are passed. These error messages should
> probably be touched up to reflect the individual nature of the commands,
> but that can be done in another patch.
OK. Actually, I can't see any need to provide messages more specific than "Not
enough arguments".
> [[[
> Replace some occurrences of SVN_ERR_CL_ARG_PARSING_ERR in favor of
> SVN_ERR_CL_INSUFFICIENT_ARGS. INSUFFICIENT_ARGS better matches the
> meaning of what we were using ARG_PARSING_ERR for. Also, update the
> generic error message to suggest running help.
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_CL_INSUFFICIENT_ARGS): Add suggestion to run svn help.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_num_args): Replace SVN_ERR_CL_ARG_PARSING_ERR in favor
> of SVN_ERR_CL_INSUFFICIENT_ARGS.
>
> [in subversion/clients/cmdline]
>
> * checkout-cmd.c
> (svn_cl__checkout): Replace SVN_ERR_CL_ARG_PARSING_ERR in favor
> of SVN_ERR_CL_INSUFFICIENT_ARGS, and update a comment.
"merge" had exactly the same, so I've changed it too. (Actually I deleted the
comments because the new versions of them don't say anything more than the code
says.)
> * import-cmd.c
> (svn_cl__import): Split the conditional checking for arguments and
> replace SVN_ERR_CL_ARG_PARSING_ERR in favor of
> SVN_ERR_CL_INSUFFICIENT_ARGS. This is consistent with svn_cl__export.
Actually it was "export" where you made this change and "import" with which it
was consistent.
Also, the same applies to "copy", "move" and "svn_cl__switch".
> * mkdir-cmd.c (svn_cl__mkdir),
> cat-cmd.c (svn_cl__cat),
> revert-cmd.c (svn_cl__revert),
> blame-cmd.c (svn_cl__blame),
> resolved-cmd.c (svn_cl__resolved),
> add-cmd.c (svn_cl__add),
> switch-cmd.c (svn_cl__switch),
Actually the function you changed in switch-cmd.c was "rewrite_urls".
> lock-cmd.c (svn_cl__lock),
> unlock-cmd.c (svn_cl__unlock): Replace SVN_ERR_CL_ARG_PARSING_ERR in
> favor of SVN_ERR_CL_INSUFFICIENT_ARGS.
Some files were missing from that list.
> ]]]
I've fixed up the log message and added everything I mentioned in the attached
patch.
- Julian
[[[
Replace some occurrences of SVN_ERR_CL_ARG_PARSING_ERR in favor of
SVN_ERR_CL_INSUFFICIENT_ARGS. INSUFFICIENT_ARGS better matches the
meaning of what we were using ARG_PARSING_ERR for. Also, update the
generic error message to suggest running help.
Patch by: Hyrum K. Wright <hyrum_wright@byu.edu>
me
* subversion/include/svn_error_codes.h
(SVN_ERR_CL_INSUFFICIENT_ARGS): Add suggestion to run 'svn help'.
* subversion/libsvn_subr/opt.c
(svn_opt_parse_num_args): Replace SVN_ERR_CL_ARG_PARSING_ERR in favor
of SVN_ERR_CL_INSUFFICIENT_ARGS.
[in subversion/clients/cmdline]
* cleanup-cmd.c
(svn_cl__cleanup): Don't bother checking for no arguments just after
we've ensured there is one.
* copy-cmd.c (svn_cl__copy):
* export-cmd.c (svn_cl__export):
* move-cmd.c (svn_cl__move):
Split the conditional checking for arguments and replace
SVN_ERR_CL_ARG_PARSING_ERR in favor of SVN_ERR_CL_INSUFFICIENT_ARGS.
This is consistent with svn_cl__import.
* add-cmd.c (svn_cl__add),
blame-cmd.c (svn_cl__blame),
cat-cmd.c (svn_cl__cat),
checkout-cmd.c (svn_cl__checkout),
delete-cmd.c (svn_cl__delete),
import-cmd.c (svn_cl__import),
lock-cmd.c (svn_cl__lock),
merge-cmd.c (svn_cl__merge),
mkdir-cmd.c (svn_cl__mkdir),
resolved-cmd.c (svn_cl__resolved),
revert-cmd.c (svn_cl__revert),
switch-cmd.c (rewrite_urls, svn_cl__switch),
unlock-cmd.c (svn_cl__unlock): Replace SVN_ERR_CL_ARG_PARSING_ERR in
favor of SVN_ERR_CL_INSUFFICIENT_ARGS.
]]]
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 17287)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -995,7 +995,7 @@ SVN_ERROR_START
SVN_ERRDEF (SVN_ERR_CL_INSUFFICIENT_ARGS,
SVN_ERR_CL_CATEGORY_START + 1,
- "Not enough args provided")
+ "Not enough arguments provided; try 'svn help' for more info")
SVN_ERRDEF (SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS,
SVN_ERR_CL_CATEGORY_START + 2,
Index: subversion/libsvn_subr/opt.c
===================================================================
--- subversion/libsvn_subr/opt.c (revision 17287)
+++ subversion/libsvn_subr/opt.c (working copy)
@@ -445,8 +445,7 @@ svn_opt_parse_num_args (apr_array_header
{
if (os->ind >= os->argc)
{
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR,
- 0, _("Too few arguments"));
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
}
array_push_str (args, os->argv[os->ind++], pool);
}
Index: subversion/clients/cmdline/add-cmd.c
===================================================================
--- subversion/clients/cmdline/add-cmd.c (revision 17287)
+++ subversion/clients/cmdline/add-cmd.c (working copy)
@@ -49,7 +49,7 @@ svn_cl__add (apr_getopt_t *os,
opt_state->targets, pool));
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (! opt_state->quiet)
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
Index: subversion/clients/cmdline/blame-cmd.c
===================================================================
--- subversion/clients/cmdline/blame-cmd.c (revision 17287)
+++ subversion/clients/cmdline/blame-cmd.c (working copy)
@@ -182,7 +182,7 @@ svn_cl__blame (apr_getopt_t *os,
/* Blame needs a file on which to operate. */
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
{
Index: subversion/clients/cmdline/cat-cmd.c
===================================================================
--- subversion/clients/cmdline/cat-cmd.c (revision 17287)
+++ subversion/clients/cmdline/cat-cmd.c (working copy)
@@ -48,7 +48,7 @@ svn_cl__cat (apr_getopt_t *os,
/* Cat cannot operate on an implicit '.' so a filename is required */
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
SVN_ERR (svn_stream_for_stdout (&out, pool));
Index: subversion/clients/cmdline/checkout-cmd.c
===================================================================
--- subversion/clients/cmdline/checkout-cmd.c (revision 17287)
+++ subversion/clients/cmdline/checkout-cmd.c (working copy)
@@ -73,10 +73,8 @@ svn_cl__checkout (apr_getopt_t *os,
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 silently exiting. */
- if (targets->nelts < 1)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ if (! targets->nelts)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
/* Add a path if the user only specified URLs */
local_dir = ((const char **) (targets->elts))[targets->nelts - 1];
Index: subversion/clients/cmdline/cleanup-cmd.c
===================================================================
--- subversion/clients/cmdline/cleanup-cmd.c (revision 17287)
+++ subversion/clients/cmdline/cleanup-cmd.c (working copy)
@@ -49,11 +49,6 @@ svn_cl__cleanup (apr_getopt_t *os,
/* Add "." if user passed 0 arguments */
svn_opt_push_implicit_dot_target (targets, pool);
- /* At this point, we should never have an empty TARGETS array, but
- check it just in case. */
- if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
-
subpool = svn_pool_create (pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/clients/cmdline/copy-cmd.c
===================================================================
--- subversion/clients/cmdline/copy-cmd.c (revision 17287)
+++ subversion/clients/cmdline/copy-cmd.c (working copy)
@@ -48,7 +48,9 @@ svn_cl__copy (apr_getopt_t *os,
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
opt_state->targets, pool));
- if (targets->nelts != 2)
+ if (targets->nelts < 2)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
+ if (targets->nelts > 2)
return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
src_path = ((const char **) (targets->elts))[0];
Index: subversion/clients/cmdline/delete-cmd.c
===================================================================
--- subversion/clients/cmdline/delete-cmd.c (revision 17287)
+++ subversion/clients/cmdline/delete-cmd.c (working copy)
@@ -48,7 +48,7 @@ svn_cl__delete (apr_getopt_t *os,
opt_state->targets, pool));
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (! opt_state->quiet)
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
Index: subversion/clients/cmdline/export-cmd.c
===================================================================
--- subversion/clients/cmdline/export-cmd.c (revision 17287)
+++ subversion/clients/cmdline/export-cmd.c (working copy)
@@ -50,7 +50,9 @@ svn_cl__export (apr_getopt_t *os,
opt_state->targets, pool));
/* We want exactly 1 or 2 targets for this subcommand. */
- if ((targets->nelts < 1) || (targets->nelts > 2))
+ if (targets->nelts < 1)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
+ if (targets->nelts > 2)
return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
/* The first target is the `from' path. */
Index: subversion/clients/cmdline/import-cmd.c
===================================================================
--- subversion/clients/cmdline/import-cmd.c (revision 17287)
+++ subversion/clients/cmdline/import-cmd.c (working copy)
@@ -78,7 +78,7 @@ svn_cl__import (apr_getopt_t *os,
if (targets->nelts < 1)
return svn_error_create
- (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ (SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
_("Repository URL required when importing"));
else if (targets->nelts > 2)
return svn_error_create
Index: subversion/clients/cmdline/lock-cmd.c
===================================================================
--- subversion/clients/cmdline/lock-cmd.c (revision 17287)
+++ subversion/clients/cmdline/lock-cmd.c (working copy)
@@ -89,7 +89,7 @@ svn_cl__lock (apr_getopt_t *os,
/* We only support locking files, so '.' is not valid. */
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
/* Get comment. */
SVN_ERR (get_comment (&comment, ctx, opt_state, pool));
Index: subversion/clients/cmdline/merge-cmd.c
===================================================================
--- subversion/clients/cmdline/merge-cmd.c (revision 17287)
+++ subversion/clients/cmdline/merge-cmd.c (working copy)
@@ -66,10 +66,8 @@ svn_cl__merge (apr_getopt_t *os,
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 (! targets->nelts)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (using_alternate_syntax)
{
Index: subversion/clients/cmdline/mkdir-cmd.c
===================================================================
--- subversion/clients/cmdline/mkdir-cmd.c (revision 17287)
+++ subversion/clients/cmdline/mkdir-cmd.c (working copy)
@@ -50,7 +50,7 @@ svn_cl__mkdir (apr_getopt_t *os,
opt_state->targets, pool));
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (! opt_state->quiet)
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
Index: subversion/clients/cmdline/move-cmd.c
===================================================================
--- subversion/clients/cmdline/move-cmd.c (revision 17287)
+++ subversion/clients/cmdline/move-cmd.c (working copy)
@@ -48,7 +48,9 @@ svn_cl__move (apr_getopt_t *os,
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
opt_state->targets, pool));
- if (targets->nelts != 2)
+ if (targets->nelts < 2)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
+ if (targets->nelts > 2)
return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
src_path = ((const char **) (targets->elts))[0];
Index: subversion/clients/cmdline/resolved-cmd.c
===================================================================
--- subversion/clients/cmdline/resolved-cmd.c (revision 17287)
+++ subversion/clients/cmdline/resolved-cmd.c (working copy)
@@ -49,7 +49,7 @@ svn_cl__resolved (apr_getopt_t *os,
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
opt_state->targets, pool));
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
subpool = svn_pool_create (pool);
if (! opt_state->quiet)
Index: subversion/clients/cmdline/revert-cmd.c
===================================================================
--- subversion/clients/cmdline/revert-cmd.c (revision 17287)
+++ subversion/clients/cmdline/revert-cmd.c (working copy)
@@ -47,7 +47,7 @@ svn_cl__revert (apr_getopt_t *os,
/* Revert has no implicit dot-target `.', so don't you put that code here! */
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
if (! opt_state->quiet)
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
Index: subversion/clients/cmdline/switch-cmd.c
===================================================================
--- subversion/clients/cmdline/switch-cmd.c (revision 17287)
+++ subversion/clients/cmdline/switch-cmd.c (working copy)
@@ -45,8 +45,8 @@ rewrite_urls(apr_array_header_t *targets
int i;
if (targets->nelts < 2)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
-
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
+
from = ((const char **) (targets->elts))[0];
to = ((const char **) (targets->elts))[1];
@@ -103,7 +103,9 @@ svn_cl__switch (apr_getopt_t *os,
if (opt_state->relocate)
return rewrite_urls (targets, !opt_state->nonrecursive, ctx, pool);
- if ((targets->nelts < 1) || (targets->nelts > 2))
+ if (targets->nelts < 1)
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
+ if (targets->nelts > 2)
return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
/* Get the required SWITCH_URL and the optional TARGET arguments. */
Index: subversion/clients/cmdline/unlock-cmd.c
===================================================================
--- subversion/clients/cmdline/unlock-cmd.c (revision 17287)
+++ subversion/clients/cmdline/unlock-cmd.c (working copy)
@@ -48,7 +48,7 @@ svn_cl__unlock (apr_getopt_t *os,
/* We don't support unlock on directories, so "." is not relevant. */
if (! targets->nelts)
- return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
+ return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
FALSE, FALSE, pool);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 10 16:28:13 2005