Re: [PATCH] issue 1954 - v5

VK Sameer <sameer@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