Noorul Islam K M <noo...@collab.net> writes:
> Julian Foad <julian.f...@wandisco.com> writes:
>
>> On Sat, 2011-02-19, Noorul Islam K M wrote:
>>
>>> I think it is this way
>>>
>>> If specified target ('/tmp/iota' in the example above) is a ...
>>> nothing -> fine
>>> file -> if 'force' then overwrite else error
>>> symlink -> if 'force' then overwrite else error
>>> special -> if 'force' then overwrite else error
>>> dir -> if sub-target ('/tmp/iota/iota') is a ...
>>> nothing -> fine
>>> file -> if 'force' then overwrite else error
>>> symlink -> if 'force' then overwrite else error
>>> special -> if 'force' then overwrite else error
>>> dir -> error cannot force
>>
>> (That's all when the source is a file; a dir is handled differently.)
>>
>> OK, that looks good. I agree that that would be a good behaviour to
>> implement.
>>
>> I see that svn_client_export5() handles the URL case and the WC case in
>> completely different ways. A URL source is handled with an "editor",
>> whereas a WC source is handled by calling copy_versioned_files() which
>> tries to do the everything including the source tree-walking by itself.
>>
>> I studied the implementation of copy_one_versioned_file() and
>> copy_versioned_files(), and I added doc strings to them in r1074526.
>> copy_versioned_files() is a mess: there are lots of things wrong with
>> it, so I can't believe it works properly.
>>
>> Does this "overwrite else error" behaviour work the same for exporting
>> from a URL as exporting from a local WC? Please could you check whether
>> that's already being tested, and extend your test if not.
>>
>
> Yes exporting from URL source also works similar to local WC. We need to
> fix that also and that should be another patch. But I extended the test
> to include this case and also left the XFail marker as such.
>
> Here is the updated patch and log message.
>
> Log
>
> [[[
>
> Fix for issue #3799. Make svn export display error, when exporting local
> WC file, tries to overwrite target. There will be a follow-up patch to
> fix when export source is URL.
>
> * subversion/libsvn_client/export.c
> (copy_versioned_files): 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.
> (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>
> ]]]
>
> Thanks and Regards
> Noorul
>
> Index: subversion/tests/cmdline/externals_tests.py
> ===================================================================
> --- subversion/tests/cmdline/externals_tests.py (revision 1074971)
> +++ 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 1074971)
> +++ subversion/tests/cmdline/export_tests.py (working copy)
> @@ -463,12 +463,13 @@
> 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)
> @@ -479,6 +480,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()
> @@ -703,6 +715,29 @@
>
> 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'))
> + 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
> + 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 the tests
>
> @@ -735,6 +770,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 1074971)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -609,7 +609,26 @@
> }
> else if (from_kind == svn_node_file)
> {
> + svn_node_kind_t kind;
> + svn_error_t *err;
> +
> SVN_ERR(append_basename_if_dir(&to_abspath, from_abspath, FALSE,
> pool));
> + svn_error_clear(svn_io_check_path(to_abspath, &kind, pool));
> +
> + if ((kind == svn_node_file || kind == svn_node_unknown) && ! force)
> + {
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> + _("Destination file '%s' exists, "
> + "and will not be overwritten unless "
> + "forced"),
> + svn_dirent_local_style(to_abspath, pool));
> + }
> + else if (kind == svn_node_dir)
> + return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, 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->wc_ctx,
> revision, native_eol, ignore_keywords,
> pool));
I incorporated the URL part also. Here is the updated patch and log. All
tests pass using "make check".
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>
]]]
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1074971)
+++ 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 1074971)
+++ subversion/tests/cmdline/export_tests.py (working copy)
@@ -456,19 +456,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)
@@ -479,6 +479,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()
@@ -703,6 +714,29 @@
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'))
+ 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
+ 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 the tests
@@ -735,6 +769,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 1074971)
+++ subversion/libsvn_client/export.c (working copy)
@@ -609,7 +609,26 @@
}
else if (from_kind == svn_node_file)
{
+ svn_node_kind_t kind;
+ svn_error_t *err;
+
SVN_ERR(append_basename_if_dir(&to_abspath, from_abspath, FALSE, pool));
+ svn_error_clear(svn_io_check_path(to_abspath, &kind, pool));
+
+ if ((kind == svn_node_file || kind == svn_node_unknown) && ! force)
+ {
+ return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
+ _("Destination file '%s' exists, "
+ "and will not be overwritten unless "
+ "forced"),
+ svn_dirent_local_style(to_abspath, pool));
+ }
+ else if (kind == svn_node_dir)
+ return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, 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->wc_ctx,
revision, native_eol, ignore_keywords,
pool));
@@ -1111,6 +1130,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))
{
@@ -1127,7 +1147,23 @@
eb->root_path = to_path;
}
+ svn_error_clear(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_FS_ALREADY_EXISTS, 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_FS_ALREADY_EXISTS, 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. */
