VK Sameer <sameer@collab.net> writes:
> Sorry for not replying to the review comments. I think all of them have
> been accepted in this patch.
The patch is the reply, no problem!
> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
>
> ####################################################
> ### ###
> ### Note: This is a draft patch for review only, ###
> ### please do not commit it. ###
> ### ###
> ####################################################
I guess the warning doesn't need to be so strict anymore, since the
patch is getting closer and closer :-).
> * subversion/include/svn_path.h
> (svn_path_check_valid): New function.
>
> * subversion/libsvn_subr/path.c
> (svn_path_check_valid): New function.
>
> * 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_ra_local/ra_plugin.c
> (svn_ra_local__do_check_path): 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): Removed tab test
For the last line, say "tab test portion moved to new test above" or
something, to make it clear what's going on here.
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 12257)
> +++ subversion/include/svn_path.h (working copy)
> @@ -356,6 +356,26 @@
> const char *path2,
> apr_pool_t *pool);
>
> +svn_boolean_t svn_path_is_valid (const char *path,
> + apr_pool_t *pool,
> + svn_boolean_t check_utf8);
> +
Oops, I think that just slipped in there :-).
> +/**
> + * @since New in 1.2.
> + *
> + * Check whether @a path is a valid Subversion path.
> + * @a pool is used for temporary allocation.
> + *
> + * 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.
> + *
> + * returns SVN_NO_ERROR if valid
> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
> + */
> +svn_error_t *svn_path_check_valid (const char *path, apr_pool_t *pool);
> +
Looks great!
Although normally we assume UTF8 strings as input, here it might be
good to explicitly state that the string must be UTF8. Because this
is a validation function, and people might otherwise assume that it
will validate that the string is valid UTF8, among other things.
>
> /** URI/URL stuff
> *
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 12257)
> +++ 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));
>
> /* 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: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 12257)
> +++ 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 12257)
> +++ 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)
> @@ -1431,10 +1432,12 @@
> const svn_wc_entry_t *entry;
> svn_node_kind_t kind;
> svn_wc_adm_access_t *adm_access;
> + apr_pool_t *subpool;
>
> /* 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);
> + SVN_ERR (svn_path_check_valid (path, subpool));
The subpool initialization changes are extraneous to this patch, and
anyway not mentioned in the log message. I'd say leave it as is, the
code was okay there anyway.
> [...]
Everything else looks good. If any of the call sites look like they
should be doing UTF8 testing as well, then this would be a good time
to add that.
> +def tab_test(sbox):
> + "tab testing"
> +
> + # ripped out of commit_uri_unsafe - currently must be used with an XFail
> + # add test for directory
Why must it be used with XFail?
You don't need to mention the source of the code, btw (unless the
source has some special pertinence here?). Everything is
cut-and-pasted from somewhere else.
> + 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)
So, is there any way we can test that a forbidden path gets the right
error? Or would that make the test suite too unportable?
> +#----------------------------------------------------------------------
> +
> def hook_test(sbox):
> "hook testing"
>
> @@ -1069,17 +1107,17 @@
> if svntest.main.windows or sys.platform == 'cygwin':
> angle_name = '_angle_'
> nasty_name = '#![]{}()__%'
> - tab_name = 'tab-path'
> + #tab_name = 'tab-path'
> else:
> angle_name = '<angle>'
> nasty_name = '#![]{}()<>%'
> - tab_name = "tab\tpath"
> + #tab_name = "tab\tpath"
>
> # Make some convenient paths.
> hash_dir = os.path.join(wc_dir, '#hash#')
> nasty_dir = os.path.join(wc_dir, nasty_name)
> space_path = os.path.join(wc_dir, 'A', 'D', 'space path')
> - tab_path = os.path.join(wc_dir, 'A', 'D', 'G', tab_name)
> + #tab_path = os.path.join(wc_dir, 'A', 'D', 'G', tab_name)
> bang_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bang!')
> bracket_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra[ket')
> brace_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra{e')
> @@ -1091,7 +1129,7 @@
> os.mkdir(hash_dir)
> os.mkdir(nasty_dir)
> svntest.main.file_append(space_path, "This path has a space in it.")
> - svntest.main.file_append(tab_path, "This path has a tab in it.")
> + #svntest.main.file_append(tab_path, "This path has a tab in it.")
> svntest.main.file_append(bang_path, "This path has a bang in it.")
> svntest.main.file_append(bracket_path, "This path has a bracket in it.")
> svntest.main.file_append(brace_path, "This path has a brace in it.")
> @@ -1103,7 +1141,7 @@
> add_list = [hash_dir,
> nasty_dir, # not xml-safe
> space_path,
> - tab_path,
> + #tab_path,
> bang_path,
> bracket_path,
> brace_path,
> @@ -1119,7 +1157,7 @@
> '#hash#' : Item(verb='Adding'),
> nasty_name : Item(verb='Adding'),
> 'A/D/space path' : Item(verb='Adding'),
> - 'A/D/G/' + tab_name : Item(verb='Adding'),
> + #'A/D/G/' + tab_name : Item(verb='Adding'),
> 'A/D/H/bang!' : Item(verb='Adding'),
> 'A/D/H/bra[ket' : Item(verb='Adding'),
> 'A/D/H/bra{e' : Item(verb='Adding'),
If you moved the code, why leave commented-out bits? Just remove them
entirely. It's all under version control anyway :-).
> @@ -1891,6 +1929,8 @@
> hudson_part_1_variation_2,
> hudson_part_2,
> hudson_part_2_1,
> + XFail(tab_test),
> + #tab_test,
> XFail(hook_test),
> merge_mixed_revisions,
> commit_uri_unsafe,
Why the commented-out "#tab_test" there?
(And again not sure why the XFail.)
Rest of the patch looks good.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 14 07:41:51 2004