Julian Foad wrote:
[...]
> 1. It was never intended that a working copy item should be specified as
> the target for operating on a revision property. It is accepted, but
> the documentation doesn't mention it: the help says "[URL]". Therefore
> there is no reason why a revision should be able to be specified
> relative to a WC target. However, the error message for such an attempt
> was inappropriate:
>
> $ svn proplist --revprop -r BASE
> svn: A path under version control is needed for this operation
>
> The new error message with this patch is:
>
> $ svn proplist --revprop -r BASE
> svn: Must specify revision explicitly, and not relative to a WC item,
> when operating on a revision property
>
[...]
>
> The attached patch fixes that error message and does some more besides.
> I'll leave point 2 for now. Reviews or thoughts, please.
... and, this time, the patch is attached.
- Julian
Improve the error checking for commands operating on revision properties.
- Give a more appropriate error when a WC-relative revision like "BASE" is
specified. (Issue #2364.)
- Give an error if multiple targets are specified, rather than ignoring them.
- Ensure that error messages are marked for localisation (they weren't for
"propget").
- Factor out duplicated code.
* subversion/clients/cmdline/cl.h,
subversion/clients/cmdline/props.c
(svn_cl__revprop_no_rev_error): Remove.
(svn_cl__revprop_prepare): New function.
* subversion/clients/cmdline/propdel-cmd.c (svn_cl__propdel),
subversion/clients/cmdline/propedit-cmd.c (svn_cl__propedit),
subversion/clients/cmdline/propget-cmd.c (svn_cl__propget),
subversion/clients/cmdline/proplist-cmd.c (svn_cl__proplist),
subversion/clients/cmdline/propset-cmd.c (svn_cl__propset):
Replace duplicated code with a call to svn_cl__revprop_prepare().
Index: subversion/clients/cmdline/cl.h
===================================================================
--- subversion/clients/cmdline/cl.h (revision 15510)
+++ subversion/clients/cmdline/cl.h (working copy)
@@ -270,10 +270,17 @@ svn_cl__print_prop_hash (apr_hash_t *pro
svn_boolean_t names_only,
apr_pool_t *pool);
-/* Return a SVN_ERR_CL_ARG_PARSING_ERROR error, with a message stating
- that one must give an explicit revision when operating on a
- revision property. */
-svn_error_t *svn_cl__revprop_no_rev_error (apr_pool_t *pool);
+/* Do the following things that are commonly required before accessing revision
+ properties. Ensure that REVISION is specified explicitly and is not
+ relative to a working-copy item. Ensure that exactly one target is
+ specified in TARGETS. Set *URL to the URL of the target. Return an
+ appropriate error if any of those checks or operations fail.
+ */
+svn_error_t *
+svn_cl__revprop_prepare (const svn_opt_revision_t *revision,
+ apr_array_header_t *targets,
+ const char **URL,
+ apr_pool_t *pool);
/* Search for a text editor command in standard environment variables,
and invoke it to edit CONTENTS (using a temporary file created in
Index: subversion/clients/cmdline/propdel-cmd.c
===================================================================
--- subversion/clients/cmdline/propdel-cmd.c (revision 15510)
+++ subversion/clients/cmdline/propdel-cmd.c (working copy)
@@ -62,26 +62,10 @@ svn_cl__propdel (apr_getopt_t *os,
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
- const char *URL, *target;
+ const char *URL;
- /* All property commands insist on a specific revision when
- operating on a revprop. */
- if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- return svn_cl__revprop_no_rev_error (pool);
-
- /* Else some revision was specified, so proceed. */
-
- /* Either we have a URL target, or an implicit wc-path ('.')
- which needs to be converted to a URL. */
- if (targets->nelts <= 0)
- return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
- _("No URL target available"));
- target = ((const char **) (targets->elts))[0];
- SVN_ERR (svn_client_url_from_path (&URL, target, pool));
- if (URL == NULL)
- return svn_error_create
- (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
- _("Either a URL or versioned item is required"));
+ SVN_ERR (svn_cl__revprop_prepare (&opt_state->start_revision, targets,
+ &URL, pool));
/* Let libsvn_client do the real work. */
SVN_ERR (svn_client_revprop_set (pname_utf8, NULL,
Index: subversion/clients/cmdline/propedit-cmd.c
===================================================================
--- subversion/clients/cmdline/propedit-cmd.c (revision 15510)
+++ subversion/clients/cmdline/propedit-cmd.c (working copy)
@@ -63,32 +63,16 @@ svn_cl__propedit (apr_getopt_t *os,
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
- const char *URL, *target;
+ const char *URL;
svn_string_t *propval;
const char *temp_dir;
- /* All property commands insist on a specific revision when
- operating on a revprop. */
- if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- return svn_cl__revprop_no_rev_error (pool);
-
- /* Else some revision was specified, so proceed. */
-
/* Implicit "." is okay for revision properties; it just helps
us find the right repository. */
svn_opt_push_implicit_dot_target (targets, pool);
- /* Either we have a URL target, or an implicit wc-path ('.')
- which needs to be converted to a URL. */
- if (targets->nelts <= 0)
- return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
- _("No URL target available"));
- target = ((const char **) (targets->elts))[0];
- SVN_ERR (svn_client_url_from_path (&URL, target, pool));
- if (URL == NULL)
- return svn_error_create
- (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
- _("Either a URL or versioned item is required"));
+ SVN_ERR (svn_cl__revprop_prepare (&opt_state->start_revision, targets,
+ &URL, pool));
/* Fetch the current property. */
SVN_ERR (svn_client_revprop_get (pname_utf8, &propval,
Index: subversion/clients/cmdline/propget-cmd.c
===================================================================
--- subversion/clients/cmdline/propget-cmd.c (revision 15510)
+++ subversion/clients/cmdline/propget-cmd.c (working copy)
@@ -86,26 +86,11 @@ svn_cl__propget (apr_getopt_t *os,
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
- const char *URL, *target;
+ const char *URL;
svn_string_t *propval;
- /* All property commands insist on a specific revision when
- operating on a revprop. */
- if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- return svn_cl__revprop_no_rev_error (pool);
-
- /* Else some revision was specified, so proceed. */
-
- /* Either we have a URL target, or an implicit wc-path ('.')
- which needs to be converted to a URL. */
- if (targets->nelts <= 0)
- return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
- "No URL target available");
- target = ((const char **) (targets->elts))[0];
- SVN_ERR (svn_client_url_from_path (&URL, target, pool));
- if (URL == NULL)
- return svn_error_create(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
- "Either a URL or versioned item is required");
+ SVN_ERR (svn_cl__revprop_prepare (&opt_state->start_revision, targets,
+ &URL, pool));
/* Let libsvn_client do the real work. */
SVN_ERR (svn_client_revprop_get (pname_utf8, &propval,
Index: subversion/clients/cmdline/proplist-cmd.c
===================================================================
--- subversion/clients/cmdline/proplist-cmd.c (revision 15510)
+++ subversion/clients/cmdline/proplist-cmd.c (working copy)
@@ -55,27 +55,11 @@ svn_cl__proplist (apr_getopt_t *os,
if (opt_state->revprop) /* operate on revprops */
{
svn_revnum_t rev;
- const char *URL, *target;
+ const char *URL;
apr_hash_t *proplist;
- /* All property commands insist on a specific revision when
- operating on revprops. */
- if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- return svn_cl__revprop_no_rev_error (pool);
-
- /* Else some revision was specified, so proceed. */
-
- /* Either we have a URL target, or an implicit wc-path ('.')
- which needs to be converted to a URL. */
- if (targets->nelts <= 0)
- return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
- _("No URL target available"));
- target = ((const char **) (targets->elts))[0];
- SVN_ERR (svn_client_url_from_path (&URL, target, pool));
- if (URL == NULL)
- return svn_error_create
- (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
- _("Either a URL or versioned item is required"));
+ SVN_ERR (svn_cl__revprop_prepare (&opt_state->start_revision, targets,
+ &URL, pool));
/* Let libsvn_client do the real work. */
SVN_ERR (svn_client_revprop_list (&proplist,
Index: subversion/clients/cmdline/props.c
===================================================================
--- subversion/clients/cmdline/props.c (revision 15510)
+++ subversion/clients/cmdline/props.c (working copy)
@@ -28,6 +28,7 @@
#include "svn_error.h"
#include "svn_subst.h"
#include "svn_props.h"
+#include "svn_opt.h"
#include "cl.h"
#include "svn_private_config.h"
@@ -35,12 +36,37 @@
svn_error_t *
-svn_cl__revprop_no_rev_error (apr_pool_t *pool)
+svn_cl__revprop_prepare (const svn_opt_revision_t *revision,
+ apr_array_header_t *targets,
+ const char **URL,
+ apr_pool_t *pool)
{
- return svn_error_create
- (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Must specify revision explicitly when operating on a "
- "revision property"));
+ const char *target;
+
+ if (revision->kind != svn_opt_revision_number
+ && revision->kind != svn_opt_revision_date
+ && revision->kind != svn_opt_revision_head)
+ return svn_error_create
+ (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Must specify revision explicitly, and not relative to a WC item, "
+ "when operating on a revision property"));
+
+ /* There must be exactly one target at this point. If it was optional and
+ unspecified by the user, the caller has already added the implicit '.'. */
+ if (targets->nelts != 1)
+ return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Wrong number of targets specified"));
+
+ /* (The docs say the target must be either a URL or implicit '.', but
+ explicit WC targets are also accepted.) */
+ target = ((const char **) (targets->elts))[0];
+ SVN_ERR (svn_client_url_from_path (URL, target, pool));
+ if (*URL == NULL)
+ return svn_error_create
+ (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
+ _("Either a URL or versioned item is required"));
+
+ return SVN_NO_ERROR;
}
Index: subversion/clients/cmdline/propset-cmd.c
===================================================================
--- subversion/clients/cmdline/propset-cmd.c (revision 15510)
+++ subversion/clients/cmdline/propset-cmd.c (working copy)
@@ -91,30 +91,14 @@ svn_cl__propset (apr_getopt_t *os,
if (opt_state->revprop) /* operate on a revprop */
{
svn_revnum_t rev;
- const char *URL, *target;
-
- /* All property commands insist on a specific revision when
- operating on a revprop. */
- if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
- return svn_cl__revprop_no_rev_error (pool);
-
- /* Else some revision was specified, so proceed. */
+ const char *URL;
/* Implicit "." is okay for revision properties; it just helps
us find the right repository. */
svn_opt_push_implicit_dot_target (targets, pool);
- /* Either we have a URL target, or an implicit wc-path ('.')
- which needs to be converted to a URL. */
- if (targets->nelts <= 0)
- return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, NULL,
- _("No URL target available"));
- target = ((const char **) (targets->elts))[0];
- SVN_ERR (svn_client_url_from_path (&URL, target, pool));
- if (URL == NULL)
- return svn_error_create (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
- _("Either a URL or versioned item is"
- " required"));
+ SVN_ERR (svn_cl__revprop_prepare (&opt_state->start_revision, targets,
+ &URL, pool));
/* Let libsvn_client do the real work. */
SVN_ERR (svn_client_revprop_set (pname_utf8, propval,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 31 18:33:56 2005