[PATCH] issue 1954 - v5

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

Hello,

Here's the latest patch, it should have changes for all of Peter's
(except the server-side checks) and Karl's review comments. The tab_test
has been fixed to use the correct error message.

Regards
Sameer

Re: [PATCH] issue 1954 - v5

[Philip Martin <ph...@codematters.co.uk>]

VK Sameer <sa...@collab.net> writes:

> --- subversion/libsvn_wc/copy.c	(revision 12313)
> +++ subversion/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));

Copy works on two paths, why are they checked in different functions?
Why check dst_path in copy_file_administratively and not in
copy_dir_administratively?  Why check dst_path at all if svn_wc_add is
going to do it?

>  
>    /* 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));

Why is src_path used before it is checked?  Why check an already
versioned path?

>    SVN_ERR (svn_io_check_path (src_path, &src_kind, pool));
>    
>    if (src_kind == svn_node_file)
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c	(revision 12313)
> +++ subversion/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: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c	(revision 12313)
> +++ subversion/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)
> @@ -1436,6 +1437,8 @@
>       a subpool for any temporary allocations. */
>    apr_pool_t *subpool = svn_pool_create (pool);
>  
> +  SVN_ERR (svn_path_check_valid (path, subpool));
> +

These checks pre-empt the svn_wc_add check, is that necessary?

>    /* ### kff todo: if file is marked as removed by user, then flag a
>       conflict in the entry and proceed.  Similarly if it has changed
>       kind.  see issuezilla task #398. */
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c	(revision 12313)
> +++ subversion/libsvn_client/add.c	(working copy)
> @@ -209,6 +209,8 @@
>    svn_node_kind_t kind;
>    svn_boolean_t is_special;
>  
> +  SVN_ERR (svn_path_check_valid (path, pool));
> +
>    /* add the file */
>    SVN_ERR (svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
>                         ctx->cancel_func, ctx->cancel_baton,
> @@ -276,6 +278,8 @@
>    svn_wc_adm_access_t *dir_access;
>    apr_array_header_t *ignores;
>  
> +  SVN_ERR (svn_path_check_valid (dirname, pool));
> +

Again, these pre-empt the svn_wc_add check.

>    /* Check cancellation; note that this catches recursive calls too. */
>    if (ctx->cancel_func)
>      SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c	(revision 12313)
> +++ subversion/libsvn_client/commit.c	(working copy)
> @@ -176,6 +176,8 @@
>    svn_node_kind_t kind;
>    svn_boolean_t is_special;
>  
> +  SVN_ERR (svn_path_check_valid (path, pool));
> +
>    SVN_ERR (svn_io_check_special_path (path, &kind, &is_special, pool));
>  
>    if (kind == svn_node_unknown)
> @@ -276,6 +278,8 @@
>    apr_hash_index_t *hi;
>    apr_array_header_t *ignores;
>  
> +  SVN_ERR (svn_path_check_valid (path, pool));
> +

These checks are probably required, but what about URI-escaped control
characters in the URL?

>    SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
>  
>    SVN_ERR (svn_io_get_dirents (&dirents, path, pool));

I'm not sure what criteria you used to choose where you check paths on
the client side.  Isn't svn_wc_add enough for anything going into a
working copy?  You appear to have additional checks on some commands
but merge appears to make do with the svn_wc_add check.

How are you going to handle things like "svn mkdir URL"?  Are you
going to use a libsvn_client check for (URI-escaped?) control
characters, or are you going to rely on checks within the RA layer?

-- 
Philip Martin

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