VK Sameer <sameer@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
Received on Wed Dec 15 07:54:22 2004