Julian Foad <julian.foad_at_wandisco.com> writes:
> Just taking a quick look.
>
> * Please always include the log message with the patch.
>
I did include the log message in one of the previous mails. Since there
was no change to the log message I did not include in the subsequent
replies.
> * Use SVN_ERR instead of svn_error_clear. There 'kind' variable is not
> guaranteed to be set to a valid value if you the function throws an
> error.
>
> * Name the variable the same way ('to_kind') in both code paths.
>
> * Should export_file_overwrite_with_force() test exporting from a URL as
> well as from a local source? (If not, why not?)
>
Incorporated you review comments. Please find attached updated
patch. Here is the log message.
Log
[[[
Fix for issue #3799. Make svn export display error, when exporting file,
tries to overwrite target.
* subversion/libsvn_client/export.c
(copy_versioned_files, svn_client_export5): Return
SVN_ERR_FS_ALREADY_EXISTS if export tries to overwrite existing
file, child directory.
* subversion/tests/cmdline/export_tests.py
(export_file_overwrite_fails): Extend test for URL source. Remove
XFail marker.
(export_file_overwrite_with_force): New test
(test_list): Add reference to new test
* subversion/tests/cmdline/externals_tests.py
(export_wc_with_externals): Fix failing test by passing --force.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
> * Why does that externals test (number 10) need "--force"? Without it,
> it fails like this, but I don't understand why:
>
> [[[
> CMD: /home/julianfoad/src/subversion-b/bin/svn export svn-test-work/working_copies/externals_tests-10 svn-test-work/working_copies/externals_tests-10.export --config-dir /home/julianfoad/build/subversion-b/subversion/tests/cmdline/svn-test-work/local_tmp/config --password rayjandom --no-auth-cache --username jrandom exited with 1
> <TIME = 0.039988>
> A svn-test-work/working_copies/externals_tests-10.export/A
> A svn-test-work/working_copies/externals_tests-10.export/A/B
> A svn-test-work/working_copies/externals_tests-10.export/A/B/lambda
> A svn-test-work/working_copies/externals_tests-10.export/A/B/gamma
> A svn-test-work/working_copies/externals_tests-10.export/A/B/E
> A svn-test-work/working_copies/externals_tests-10.export/A/B/E/alpha
> A svn-test-work/working_copies/externals_tests-10.export/A/B/E/beta
> A svn-test-work/working_copies/externals_tests-10.export/A/B/F
> /home/julianfoad/src/subversion-b/subversion/svn/export-cmd.c:123: (apr_err=200009)
> /home/julianfoad/src/subversion-b/subversion/libsvn_client/export.c:1291: (apr_err=200009)
> /home/julianfoad/src/subversion-b/subversion/libsvn_client/export.c:499: (apr_err=200009)
> /home/julianfoad/src/subversion-b/subversion/libsvn_client/export.c:499: (apr_err=200009)
> /home/julianfoad/src/subversion-b/subversion/libsvn_client/export.c:563: (apr_err=200009)
> /home/julianfoad/src/subversion-b/subversion/libsvn_client/export.c:578: (apr_err=200009)
> svn: E200009: Destination file '/home/julianfoad/build/subversion-b/subversion/tests/cmdline/svn-test-work/working_copies/externals_tests-10.export/A/B/gamma' exists, and will not be overwritten unless forced
> ]]]
>
A/B/gamma is part of working copy and also part of the externals. This
makes this path to be exported twice. During the second time it is
failing with the above message.
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1126953)
+++ subversion/tests/cmdline/externals_tests.py (working copy)
@@ -752,7 +752,8 @@
repo_url, wc_dir)
# Export the working copy.
svntest.actions.run_and_verify_svn(None, None, [],
- 'export', wc_dir, export_target)
+ 'export', '--force',
+ wc_dir, export_target)
### We should be able to check exactly the paths that externals_test_setup()
### set up; however, --ignore-externals fails to ignore 'A/B/gamma' so this
Index: subversion/tests/cmdline/export_tests.py
===================================================================
--- subversion/tests/cmdline/export_tests.py (revision 1126953)
+++ subversion/tests/cmdline/export_tests.py (working copy)
@@ -589,19 +589,19 @@
'.', expected_output,
expected_disk)
-_at_XFail()
@Issue(3799)
def export_file_overwrite_fails(sbox):
"exporting a file refuses to silently overwrite"
sbox.build(create_wc = True, read_only = True)
iota_path = os.path.abspath(os.path.join(sbox.wc_dir, 'iota'))
+ iota_url = sbox.repo_url + '/iota'
not_iota_contents = "This obstructs 'iota'.\n"
tmpdir = sbox.get_tempname('file-overwrites')
os.mkdir(tmpdir)
- # Run it
+ # Run it for source local
open(os.path.join(tmpdir, 'iota'), 'w').write(not_iota_contents)
svntest.actions.run_and_verify_svn(None, [], '.*exist.*',
'export', iota_path, tmpdir)
@@ -612,6 +612,17 @@
})
svntest.actions.verify_disk(tmpdir, expected_disk)
+ # Run it for source URL
+ open(os.path.join(tmpdir, 'iota'), 'w').write(not_iota_contents)
+ svntest.actions.run_and_verify_svn(None, [], '.*exist.*',
+ 'export', iota_url, tmpdir)
+
+ # Verify it failed
+ expected_disk = svntest.wc.State('', {
+ 'iota': Item(contents=not_iota_contents),
+ })
+ svntest.actions.verify_disk(tmpdir, expected_disk)
+
def export_ignoring_keyword_translation(sbox):
"export ignoring keyword translation"
sbox.build()
@@ -867,6 +878,36 @@
os.chdir(orig_dir)
+def export_file_overwrite_with_force(sbox):
+ "exporting a file with force option"
+ sbox.build(create_wc = True, read_only = True)
+
+ iota_path = os.path.abspath(os.path.join(sbox.wc_dir, 'iota'))
+ iota_url = sbox.repo_url + '/iota'
+ not_iota_contents = "This obstructs 'iota'.\n"
+ iota_contents = "This is the file 'iota'.\n"
+
+ tmpdir = sbox.get_tempname('file-overwrites')
+ os.mkdir(tmpdir)
+
+ expected_disk = svntest.wc.State('', {
+ 'iota': Item(contents=iota_contents),
+ })
+
+ # Run it for WC export
+ open(os.path.join(tmpdir, 'iota'), 'w').write(not_iota_contents)
+ svntest.actions.run_and_verify_svn(None, svntest.verify.AnyOutput,
+ [], 'export', '--force',
+ iota_path, tmpdir)
+ svntest.actions.verify_disk(tmpdir, expected_disk)
+
+ # Run it for URL export
+ open(os.path.join(tmpdir, 'iota'), 'w').write(not_iota_contents)
+ svntest.actions.run_and_verify_svn(None, svntest.verify.AnyOutput,
+ [], 'export', '--force',
+ iota_url, tmpdir)
+ svntest.actions.verify_disk(tmpdir, expected_disk)
+
########################################################################
# Run the tests
@@ -899,6 +940,7 @@
export_working_copy_with_depths,
export_externals_with_native_eol,
export_to_current_dir,
+ export_file_overwrite_with_force,
]
if __name__ == '__main__':
Index: subversion/libsvn_client/export.c
===================================================================
--- subversion/libsvn_client/export.c (revision 1126953)
+++ subversion/libsvn_client/export.c (working copy)
@@ -569,6 +569,25 @@
}
else if (from_kind == svn_node_file)
{
+ svn_node_kind_t to_kind;
+
+ SVN_ERR(svn_io_check_path(to_abspath, &to_kind, pool));
+
+ if ((to_kind == svn_node_file || to_kind == svn_node_unknown) && ! force)
+ {
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Destination file '%s' exists, "
+ "and will not be overwritten unless "
+ "forced"),
+ svn_dirent_local_style(to_abspath, pool));
+ }
+ else if (to_kind == svn_node_dir)
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Destination %s exists. Cannot overwrite "
+ "directory with non-directory"),
+ svn_dirent_local_style(to_abspath, pool));
+
+
SVN_ERR(copy_one_versioned_file(from_abspath, to_abspath, ctx,
revision, native_eol, ignore_keywords,
pool));
@@ -1064,6 +1083,7 @@
apr_hash_t *props;
apr_hash_index_t *hi;
struct file_baton *fb = apr_pcalloc(pool, sizeof(*fb));
+ svn_node_kind_t to_kind;
if (svn_path_is_empty(to_path))
{
@@ -1080,7 +1100,23 @@
eb->root_path = to_path;
}
+ SVN_ERR(svn_io_check_path(to_path, &to_kind, pool));
+ if ((to_kind == svn_node_file || to_kind == svn_node_unknown) &&
+ ! overwrite)
+ {
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Destination file '%s' exists, "
+ "and will not be overwritten unless "
+ "forced"),
+ svn_dirent_local_style(to_path, pool));
+ }
+ else if (to_kind == svn_node_dir)
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Destination %s exists. Cannot overwrite "
+ "directory with non-directory"),
+ svn_dirent_local_style(to_path, pool));
+
/* Since you cannot actually root an editor at a file, we
* manually drive a few functions of our editor. */
Received on 2011-05-25 11:51:28 CEST