[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Input validation observations

From: Noorul Islam K M <noorul_at_collab.net>
Date: Fri, 03 Dec 2010 19:15:39 +0530

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.
>
>> Index: subversion/libsvn_client/add.c
>> ===================================================================
>> --- subversion/libsvn_client/add.c (revision 1041293)
>> +++ subversion/libsvn_client/add.c (working copy)
>> @@ -875,9 +875,28 @@
>> 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;
>> + }
>> +
>> + if (url_present && wc_present)
>> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
>> + _("Cannot mix repository and working copy "
>> + "targets"));
>
> Does this validation make the check at the beginning of mkdir_urls()
> redundant? If so, we should get rid of it.
>

Attached is the modified patch.

Log

[[[
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.
  (mkdir_urls): Remove redundant code.

* subversion/tests/cmdline/input_validation_tests.py
  (invalid_mkdir_targets, test_list): New test

Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]

Thanks and Regards
Noorul

Index: subversion/tests/cmdline/input_validation_tests.py
===================================================================
--- subversion/tests/cmdline/input_validation_tests.py (revision 1040511)
+++ subversion/tests/cmdline/input_validation_tests.py (working copy)
@@ -234,6 +234,12 @@
   run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'relocate',
                            "^/", "^/", "^/")
 
+def invalid_mkdir_targets(sbox):
+ "invalid targets for 'mkdir'"
+ sbox.build(read_only=True)
+ run_and_verify_svn_in_wc(sbox, "svn: Cannot mix repository and working "
+ "copy targets", 'mkdir', "folder", "^/folder")
+
 ########################################################################
 # Run the tests
 
@@ -261,6 +267,7 @@
               invalid_patch_targets,
               invalid_switch_targets,
               invalid_relocate_targets,
+ invalid_mkdir_targets,
              ]
 
 if __name__ == '__main__':
Index: subversion/svn/mkdir-cmd.c
===================================================================
--- subversion/svn/mkdir-cmd.c (revision 1040511)
+++ subversion/svn/mkdir-cmd.c (working copy)
@@ -48,6 +48,8 @@
   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,
@@ -56,6 +58,22 @@
   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;
+ }
+
+ 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/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 1040511)
+++ subversion/libsvn_client/add.c (working copy)
@@ -673,16 +673,6 @@
   const char *common;
   int i;
 
- /* Early exit when there is a mix of URLs and local paths. */
- for (i = 0; i < urls->nelts; i++)
- {
- const char *url = APR_ARRAY_IDX(urls, i, const char *);
- if (! svn_path_is_url(url))
- return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
- _("Illegal repository URL '%s'"),
- url);
- }
-
   /* Find any non-existent parent directories */
   if (make_parents)
     {
@@ -875,9 +865,28 @@
                   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;
+ }
+
+ 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,
@@ -887,7 +896,6 @@
     {
       /* 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-03 14:46:26 CET

This is an archived mail posted to the Subversion Dev mailing list.