Julian Foad <julian.foad_at_wandisco.com> writes:
> On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <julian.foad_at_wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Julian Foad <julian.foad_at_wandisco.com> writes:
>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> >> > mix URL and local targets"?
>> > [...]
>> >> Make 'svn mkdir' verify that both working copy paths and URLs are
>> >> not passed.
>> >>
>> >> * subversion/svn/mkdir-cmd.c,
>> >> subversion/libsvn_client/add.c
>> >> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>> >> copy paths and URLs are passed.
>> >>
>> >> * subversion/tests/cmdline/input_validation_tests.py
>> >> (invalid_mkdir_targets, test_list): New test
>> > [...]
>> >> Index: subversion/svn/mkdir-cmd.c
>> >> ===================================================================
>> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>> >> +++ subversion/svn/mkdir-cmd.c (working copy)
>> >> @@ -48,6 +48,8 @@
>> >> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> >> + int i;
>> >> @@ -56,6 +58,22 @@
>> >> + /* Check to see if at least one of our paths is a working copy
>> >> + path or a repository url. */
>> >> + for (i = 0; i < targets->nelts; ++i)
>> >> + {
>> >> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> >> + if (! svn_path_is_url(target))
>> >> + wc_present = TRUE;
>> >> + else
>> >> + url_present = TRUE;
>> >> + }
>> >> +
>> >> + if (url_present && wc_present)
>> >> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> >> + _("Cannot mix repository and working copy "
>> >> + "targets"));
>> >
>> > This is fine.
>> >
>> > The same code already exists in three other files and equivalent but
>> > different code also exists in at least delete-cmd.c and probably other
>> > files. I think it is time to factor it out. We can do that in a
>> > subsequent patch.
>>
>> Do you mean we need to come up with new function that will do this check
>> and return the error message?
>
> Yes please.
Attached is the patch which has the new functions to replace the
existing blocks. All tests pass when tested with 'make check'. No need
for test cases as they are already available. Also I have not modified
libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
forward. I will go through them again and will come up with follow-up
patch.
Log
[[[
Two new functions one each for command line and client API which checks
whether the target types are homogeneous.
* subversion/svn/cl.h,
subversion/svn/util.c
(svn_cl__assert_homogeneous_target_type): New function
* subversion/libsvn_client/client.h
subversion/libsvn_client/util.c
(svn_client__assert_homogeneous_target_type): New function
* subversion/svn/mkdir-cmd.c,
subversion/svn/delete-cmd.c,
subversion/svn/lock-cmd.c,
subversion/svn/unlock-cmd.c
(svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
Replace existing logic with call to new function
svn_cl__assert_homogeneous_target_type
* subversion/libsvn_client/delete.c,
subversion/libsvn_client/locking_commands.c,
subversion/libsvn_client/add.c
(svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
Replace existing logic with call to new function
svn_client__assert_homogeneous_target_type
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1043482)
+++ subversion/svn/cl.h (working copy)
@@ -817,6 +817,11 @@
const char *path,
apr_pool_t *pool);
+/* This function checks to see if targets contain mixture of URLs
+ * and paths, returning an error if it finds a mixture of both. */
+svn_error_t *
+svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/svn/mkdir-cmd.c
===================================================================
--- subversion/svn/mkdir-cmd.c (revision 1043482)
+++ subversion/svn/mkdir-cmd.c (working copy)
@@ -48,8 +48,6 @@
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_array_header_t *targets;
svn_error_t *err;
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -58,22 +56,8 @@
if (! targets->nelts)
return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
- /* Check to see if at least one of our paths is a working copy
- path or a repository url. */
- for (i = 0; i < targets->nelts; ++i)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
- if (! svn_path_is_url(target))
- wc_present = TRUE;
- else
- url_present = TRUE;
- }
+ SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
- if (url_present && wc_present)
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Cannot mix repository and working copy "
- "targets"));
-
if (! svn_path_is_url(APR_ARRAY_IDX(targets, 0, const char *)))
{
ctx->log_msg_func3 = NULL;
Index: subversion/svn/util.c
===================================================================
--- subversion/svn/util.c (revision 1043482)
+++ subversion/svn/util.c (working copy)
@@ -1345,3 +1345,26 @@
return SVN_NO_ERROR;
}
+
+svn_error_t *
+svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets)
+{
+ svn_boolean_t wc_present = FALSE, url_present = FALSE;
+ int i;
+
+ for (i = 0; i < targets->nelts; ++i)
+ {
+ const char *target = APR_ARRAY_IDX(targets, i, const char *);
+ if (! svn_path_is_url(target))
+ wc_present = TRUE;
+ else
+ url_present = TRUE;
+ }
+
+ if (url_present && wc_present)
+ return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Cannot mix repository and working copy "
+ "targets"));
+
+ return SVN_NO_ERROR;
+}
Index: subversion/svn/delete-cmd.c
===================================================================
--- subversion/svn/delete-cmd.c (revision 1043482)
+++ subversion/svn/delete-cmd.c (working copy)
@@ -48,7 +48,6 @@
apr_array_header_t *targets;
svn_error_t *err;
svn_boolean_t is_url;
- int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -57,17 +56,8 @@
if (! targets->nelts)
return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
- /* Check that all targets are of the same type. */
+ SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
is_url = svn_path_is_url(APR_ARRAY_IDX(targets, 0, const char *));
- for (i = 1; i < targets->nelts; i++)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
- if (is_url != svn_path_is_url(target))
- return svn_error_return(
- svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Cannot mix repository and working copy "
- "targets")));
- }
if (! is_url)
{
Index: subversion/svn/lock-cmd.c
===================================================================
--- subversion/svn/lock-cmd.c (revision 1043482)
+++ subversion/svn/lock-cmd.c (working copy)
@@ -89,8 +89,6 @@
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_array_header_t *targets;
const char *comment;
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -100,22 +98,8 @@
if (! targets->nelts)
return svn_error_create(SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
- /* Check to see if at least one of our paths is a working copy
- * path or a repository url. */
- for (i = 0; i < targets->nelts; ++i)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
- if (! svn_path_is_url(target))
- wc_present = TRUE;
- else
- url_present = TRUE;
- }
+ SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
- if (url_present && wc_present)
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Cannot mix repository and working copy "
- "targets"));
-
/* Get comment. */
SVN_ERR(get_comment(&comment, ctx, opt_state, pool));
Index: subversion/svn/unlock-cmd.c
===================================================================
--- subversion/svn/unlock-cmd.c (revision 1043482)
+++ subversion/svn/unlock-cmd.c (working copy)
@@ -49,8 +49,6 @@
svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_array_header_t *targets;
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -62,23 +60,8 @@
SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
- /* Check to see if at least one of our paths is a working copy
- * path or a repository url. */
- for (i = 0; i < targets->nelts; ++i)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
+ SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
- if (! svn_path_is_url(target))
- wc_present = TRUE;
- else
- url_present = TRUE;
- }
-
- if (url_present && wc_present)
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Cannot mix repository and working copy "
- "targets"));
-
return svn_error_return(
svn_client_unlock(targets, opt_state->force, ctx, scratch_pool));
}
Index: subversion/libsvn_client/delete.c
===================================================================
--- subversion/libsvn_client/delete.c (revision 1043482)
+++ subversion/libsvn_client/delete.c (working copy)
@@ -328,17 +328,8 @@
if (! paths->nelts)
return SVN_NO_ERROR;
- /* Check that all targets are of the same type. */
+ SVN_ERR(svn_client__assert_homogeneous_target_type(paths));
is_url = svn_path_is_url(APR_ARRAY_IDX(paths, 0, const char *));
- for (i = 1; i < paths->nelts; i++)
- {
- const char *path = APR_ARRAY_IDX(paths, i, const char *);
- if (is_url != svn_path_is_url(path))
- return svn_error_return(
- svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
- _("Cannot mix repository and working copy "
- "targets")));
- }
if (is_url)
{
Index: subversion/libsvn_client/util.c
===================================================================
--- subversion/libsvn_client/util.c (revision 1043482)
+++ subversion/libsvn_client/util.c (working copy)
@@ -320,3 +320,26 @@
return peg_revision;
return revision;
}
+
+svn_error_t *
+svn_client__assert_homogeneous_target_type(const apr_array_header_t *targets)
+{
+ svn_boolean_t wc_present = FALSE, url_present = FALSE;
+ int i;
+
+ for (i = 0; i < targets->nelts; ++i)
+ {
+ const char *target = APR_ARRAY_IDX(targets, i, const char *);
+ if (! svn_path_is_url(target))
+ wc_present = TRUE;
+ else
+ url_present = TRUE;
+ }
+
+ if (url_present && wc_present)
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Cannot mix repository and working copy "
+ "targets"));
+
+ return SVN_NO_ERROR;
+}
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 1043482)
+++ subversion/libsvn_client/client.h (working copy)
@@ -1107,8 +1107,10 @@
svn_cl__rev_default_to_peg(const svn_opt_revision_t *revision,
const svn_opt_revision_t *peg_revision);
-
-
+/* This function checks to see if targets contain mixture of URLs
+ * and paths, returning an error if it finds mixture of both. */
+svn_error_t *
+svn_client__assert_homogeneous_target_type(const apr_array_header_t *targets);
#ifdef __cplusplus
}
Index: subversion/libsvn_client/locking_commands.c
===================================================================
--- subversion/libsvn_client/locking_commands.c (revision 1043482)
+++ subversion/libsvn_client/locking_commands.c (working copy)
@@ -182,26 +182,9 @@
apr_hash_t *rel_targets_ret = apr_hash_make(pool);
apr_pool_t *subpool = svn_pool_create(pool);
svn_boolean_t url_mode;
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- /* Check to see if at least one of our paths is a working copy
- * path or a repository url. */
- for (i = 0; i < targets->nelts; ++i)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
- if (! svn_path_is_url(target))
- wc_present = TRUE;
- else
- url_present = TRUE;
- }
+ SVN_ERR(svn_client__assert_homogeneous_target_type(targets));
- if (url_present && wc_present)
- return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
- _("Cannot mix repository and working copy "
- "targets"));
-
- /* All targets must be either urls or paths */
-
url_mode = ((targets->nelts >= 1) &&
svn_path_is_url(APR_ARRAY_IDX(targets, 0, const char *)));
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 1043482)
+++ subversion/libsvn_client/add.c (working copy)
@@ -865,28 +865,11 @@
svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
- svn_boolean_t wc_present = FALSE, url_present = FALSE;
- int i;
-
if (! paths->nelts)
return SVN_NO_ERROR;
- /* Check to see if at least one of our paths is a working copy
- path or a repository url. */
- for (i = 0; i < paths->nelts; ++i)
- {
- const char *path = APR_ARRAY_IDX(paths, i, const char *);
- if (! svn_path_is_url(path))
- wc_present = TRUE;
- else
- url_present = TRUE;
- }
+ SVN_ERR(svn_client__assert_homogeneous_target_type(paths));
- if (url_present && wc_present)
- return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
- _("Cannot mix repository and working copy "
- "targets"));
-
if (svn_path_is_url(APR_ARRAY_IDX(paths, 0, const char *)))
{
SVN_ERR(mkdir_urls(paths, make_parents, revprop_table, commit_callback,
@@ -896,6 +879,7 @@
{
/* This is a regular "mkdir" + "svn add" */
apr_pool_t *subpool = svn_pool_create(pool);
+ int i;
for (i = 0; i < paths->nelts; i++)
{
Received on 2010-12-08 19:09:36 CET