[Patch] issue 1954 v4

[VK Sameer <sa...@collab.net>]

Hello,

Here is the updated patch after review. I still have to fix the tests
and check the call sites.

Thanks
Sameer

Re: [Patch] issue 1954 v4

[kf...@collab.net]

VK Sameer <sa...@collab.net> writes:
> Here is the updated patch after review. I still have to fix the tests
> and check the call sites.

Thanks so much for your patience, Sameer.  We're getting close to
applying this; most of my comments below are very minor nits (and in
fact I'd just fix them when applying, except I know you'd want to see
the review anyway).

> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
> 
> * subversion/include/svn_path.h
>   (svn_path_check_valid): New function.
> 
> * subversion/libsvn_subr/path.c
>   (svn_path_check_valid): New function.

I'll often compress the header and source entry like this:

   * subversion/include/svn_path.h, subversion/libsvn_subr/path.c
     (svn_path_check_valid): New function.

when possible, though it's a matter of taste.

> * subversion/libsvn_wc/copy.c
>   (copy_file_administratively): Call svn_path_check_valid().
>   (svn_wc_copy): Same.
> 
> * subversion/libsvn_wc/adm_ops.c
>   (svn_wc_add): Same.
> 
> * subversion/libsvn_wc/update_editor.c
>   (add_directory, add_or_open_file): Same.
> 
> * subversion/libsvn_client/copy.c
>   (wc_to_wc_copy): Same.
> 
> * subversion/libsvn_client/add.c
>   (add_dir_recursive, add_file): Same.
> 
> * subversion/libsvn_client/commit.c
>   (import_file, import_dir): Same.
> 
> * subversion/tests/clients/cmdline/commit_tests.py
>   (tab_test): New test.
>   (test_list): Run it XFail.
>   (commit_uri_unsafe): Moved tab test parts to (tab_test)

You don't need parens around "(tab_test)" at the end there.  Those are
only for the symbol's own entry in the change list.

> Index: include/svn_path.h
> ===================================================================
> --- include/svn_path.h	(revision 12305)
> +++ include/svn_path.h	(working copy)
> @@ -356,6 +356,22 @@
>                                 const char *path2,
>                                 apr_pool_t *pool);
>  
> +/** @since New in 1.2.
> + * Check whether @a path is a valid Subversion path.
> + *
> + * A valid Subversion pathname is a UTF-8 string without control
> + * characters. "Valid" means Subversion can store the pathname in
> + * a repository. There may be other, OS-specific, limitations on what
> + * paths can be represented in a working copy.
> + *
> + * ASSUMPTION: @a path has already been validated as a UTF-8 string
> + * and no further check is necessary.
> + *
> + * Returns @c SVN_NO_ERROR if valid and @c SVN_ERR_FS_PATH_SYNTAX if
> + * invalid.
> + */
> +svn_error_t *svn_path_check_valid (const char *path, apr_pool_t *pool);
> +

Some minor language comments about the docstring.

Please use the same location/formatting for the "@since" line, and the
line following, as the rest of the file does.

Say "ASSUMPTION: @a path is a valid UTF-8 string.  This function does
not check UTF-8 validity."  That's tighter and less confusing, IMHO.

Some may point out that the last sentence isn't *strictly* true, since
we return not the error code but an error object (or null, which is
SVN_NO_ERROR).  But we write it your way a lot in Subversion, so I
think it's fine :-).

>  
>  /** URI/URL stuff
>   *
> Index: libsvn_wc/copy.c
> ===================================================================
> --- libsvn_wc/copy.c	(revision 12305)
> +++ libsvn_wc/copy.c	(working copy)
> @@ -141,6 +141,7 @@
>    /* The 'dst_path' is simply dst_parent/dst_basename */
>    const char *dst_path
>      = svn_path_join (svn_wc_adm_access_path (dst_parent), dst_basename, pool);
> +  SVN_ERR (svn_path_check_valid (dst_path, pool));
>  
>    /* Sanity check:  if dst file exists already, don't allow overwrite. */
>    SVN_ERR (svn_io_check_path (dst_path, &dst_kind, pool));
> @@ -470,6 +471,7 @@
>    SVN_ERR (svn_wc_adm_probe_open2 (&adm_access, NULL, src_path, FALSE, -1,
>                                     pool));
>  
> +  SVN_ERR (svn_path_check_valid (src_path, pool));
>    SVN_ERR (svn_io_check_path (src_path, &src_kind, pool));
>    
>    if (src_kind == svn_node_file)
> Index: libsvn_wc/adm_ops.c
> ===================================================================
> --- libsvn_wc/adm_ops.c	(revision 12305)
> +++ libsvn_wc/adm_ops.c	(working copy)
> @@ -879,6 +879,8 @@
>    svn_node_kind_t kind;
>    apr_uint32_t modify_flags = 0;
>    svn_wc_adm_access_t *adm_access;
> +
> +  SVN_ERR (svn_path_check_valid (path, pool));
>    
>    /* Make sure something's there. */
>    SVN_ERR (svn_io_check_path (path, &kind, pool));
> Index: libsvn_wc/update_editor.c
> ===================================================================
> --- libsvn_wc/update_editor.c	(revision 12305)
> +++ libsvn_wc/update_editor.c	(working copy)
> @@ -1014,6 +1014,7 @@
>        || ((! copyfrom_path) && (SVN_IS_VALID_REVNUM (copyfrom_revision))))
>      abort();
>  
> +  SVN_ERR (svn_path_check_valid (db->path, db->pool));
>    /* There should be nothing with this name. */
>    SVN_ERR (svn_io_check_path (db->path, &kind, db->pool));
>    if (kind != svn_node_none)
> @@ -1431,10 +1432,13 @@
>    const svn_wc_entry_t *entry;
>    svn_node_kind_t kind;
>    svn_wc_adm_access_t *adm_access;
> +  apr_pool_t *subpool;
>  
> +  SVN_ERR (svn_path_check_valid (path, pool));
> +
>    /* the file_pool can stick around for a *long* time, so we want to use
>       a subpool for any temporary allocations. */
> -  apr_pool_t *subpool = svn_pool_create (pool);
> +  subpool = svn_pool_create (pool);

I agree with Julian's comment about using the subpool here instead,
though it's probably not a big deal either way.

>  #----------------------------------------------------------------------
>  
> +def tab_test(sbox):
> +  "tab testing"
> +
> +  # ripped out of commit_uri_unsafe - currently must be used with an XFail
> +  # add test for directory

Don't forget to remove that XFail :-).

I don't think you need to say where you got the code from.  No one
will ever need to know that this was once part of commit_uri_unsafe.
The presence of that fact is distracting, because people might assume
it has some relevance, and so they'll pause and wonder why they might
need to know it.

> +  sbox.build()
> +  wc_dir = sbox.wc_dir
> +
> +  if svntest.main.windows or sys.platform == 'cygwin':
> +    tab_name   = 'tab-path'
> +  else:
> +    tab_name   = "tab\tpath"
> +  
> +  tab_path = os.path.join(wc_dir, 'A', tab_name)
> +  svntest.main.file_append(tab_path, "This path has a tab in it.")
> +  svntest.main.run_svn(None, 'add', '--non-recursive', tab_path)
> +
> +  expected_output = svntest.wc.State(wc_dir, {
> +    'A/' + tab_name : Item(verb='Adding'),
> +    })
> +
> +  expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
> +  # Items in the status list are all at rev 1
> +  expected_status.tweak(wc_rev=1)
> +
> +  # Items in our add list will be at rev 2
> +  for item in expected_output.desc.keys():
> +    expected_status.add({ item : Item(wc_rev=2, repos_rev=2, status='  ') })
> +
> +  svntest.actions.run_and_verify_commit (wc_dir,
> +                                         expected_output,
> +                                         expected_status,
> +                                         None, None, None, None, None,
> +                                         wc_dir)
> +
> +#----------------------------------------------------------------------

If you're expecting a particular error message, maybe use
svntest.actions.run_svn() instead?  (This is from memory, I might have
the name wrong.)  I know we have examples in the test suite of testing
for particular errors, from commit I believe.  Look for one of those.

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org