Julian Foad <julian.f...@wandisco.com> writes:
> On Tue, 2011-05-31 at 14:52 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <julian.f...@wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Noorul Islam K M <noo...@collab.net> writes:
>> >> > Julian Foad <julian.f...@wandisco.com> writes:
>> > [...]
>> >> >> * 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.
>> >
>> > Thanks. I confirm those fixes.
>> >
>> > [...]
>> >> * subversion/tests/cmdline/externals_tests.py
>> >> (export_wc_with_externals): Fix failing test by passing --force.
>> >
>> >> >> * Why does that externals test (number 10) need "--force"? Without it,
>> >> >> it fails like this, but I don't understand why:
>> >> >> 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.
>> >
>> > A/B/gamma is only an external: it does not appear in the WC until
>> > Subversion processes the external definitions.
>> >
>> > It looks to me like that failure was showing us a bug. If I run the
>> > test, without your patch, in verbose mode, I see:
>> >
>> > CMD: svn export svn-test-work/working_copies/externals_tests-10
>> > svn-test-work/working_copies/externals_tests-10.export [...]
>> > 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/gamma
>> > [...]
>> > CMD: svn export --ignore-externals
>> > svn-test-work/working_copies/externals_tests-10
>> > svn-test-work/working_copies/externals_tests-10.export [...]
>> > 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
>> > [...]
>> >
>> > There is a comment in the test about --ignore-externals not ignoring
>> > A/B/gamma. That's a bug. And the first export (without
>> > --ignore-externals) is also buggy. It shouldn't export A/B/gamma twice.
>> >
>> > We shouldn't just quietly tweak the test to hide the bug. We should
>> > write a new test specifically to check for that bug, or fix the bug, or
>> > file an issue, or write to the dev@ list about it. Something.
>> >
>>
>> Julian,
>>
>> I started a new thread http://svn.haxx.se/dev/archive-2011-05/1045.shtml
>> for this.
>
> Thanks.
>
>> Now is it ok to mark the failing test as XFail and proceed with this
>> patch?
>
> Yes, please do. With that change, I think the patch is finished and can
> be committed.
>
Please find updated log message and attached patch.
[[[
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_ILLEGAL_TARGET 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): Add XFail marker since it is failing
because of a bug in handling externals. See
http://svn.haxx.se/dev/archive-2011-05/1045.shtml
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1126953)
+++ subversion/tests/cmdline/externals_tests.py (working copy)
@@ -737,6 +737,7 @@
# Test for issue #2429
@Issue(2429)
+@XFail()
def export_wc_with_externals(sbox):
"test exports from working copies with externals"
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)
-@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. */
