Julian Foad <julian.f...@wandisco.com> writes:
> On Tue, 2011-02-15, Noorul Islam K M wrote:
>
>> More details about the issue can be found
>> here. http://subversion.tigris.org/issues/show_bug.cgi?id=3799
>>
>> Using trunk when exporting a file, if the file already exists at target
>> location then operation overwrites the file without giving any
>> warning. But in the case of directory it throws an error saying
>>
>> "Destination directory exists, and will not be overwritten unless
>> forced"
>>
>> This patch makes export throw an error in the case of file
>> overwriting and ask to use --force option.
>>
>> Log
>> [[[
>>
>> Make svn export display error when exporting file tries to overwrite
>> target.
>
> Please include the issue number in the log message.
>
Please see the modified log.
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): Return SVN_ERR_FS_ALREADY_EXISTS if export
tries to overwrite existing file.
* subversion/tests/cmdline/export_tests.py
(export_file_overwrite_fails): 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>
]]]
>> * subversion/svn/export-cmd.c
>> (svn_cl__export): Generalise error message to include file.
>>
>> * subversion/libsvn_client/export.c
>> (copy_versioned_files): Return SVN_ERR_WC_OBSTRUCTED_UPDATE if export
>> tries to overwrite existing file.
>>
>> * subversion/tests/cmdline/export_tests.py
>> (export_file_overwrite_fails): 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/export_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/export_tests.py (revision 1070515)
>> +++ subversion/tests/cmdline/export_tests.py (working copy)
>> @@ -456,7 +456,6 @@
>> '.', expected_output,
>> expected_disk)
>>
>> -@XFail()
>> @Issue(3799)
>> def export_file_overwrite_fails(sbox):
>> "exporting a file refuses to silently overwrite"
>> @@ -703,6 +702,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)
>
> Should we test a non-file obstruction (a directory and/or a symlink?) as
> well?
>
I will submit a follow-up patch.
>> + 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 +757,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/svn/export-cmd.c
>> ===================================================================
>> --- subversion/svn/export-cmd.c (revision 1070515)
>> +++ subversion/svn/export-cmd.c (working copy)
>> @@ -112,8 +112,8 @@
>> opt_state->native_eol, ctx, pool);
>> if (err && err->apr_err == SVN_ERR_WC_OBSTRUCTED_UPDATE &&
>> !opt_state->force)
>> SVN_ERR_W(err,
>> - _("Destination directory exists; please remove "
>> - "the directory or use --force to overwrite"));
>> + _("Destination directory/file already exists; please remove "
>> + "it or use --force to overwrite"));
>>
>> if (nwb.had_externals_error)
>> return svn_error_create(SVN_ERR_CL_ERROR_PROCESSING_EXTERNALS, NULL,
>> Index: subversion/libsvn_client/export.c
>> ===================================================================
>> --- subversion/libsvn_client/export.c (revision 1070515)
>> +++ subversion/libsvn_client/export.c (working copy)
>> @@ -524,7 +524,16 @@
>> }
>> else if (from_kind == svn_node_file)
>> {
>> + svn_node_kind_t kind;
>> +
>> 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) && ! force)
>
> What if the obstruction is not a file but something else (dir or
> unknown)?
>
Obstruction of directory is already taken care of.
>> + return svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
>> + _("'%s' already exists"),
>> + svn_dirent_local_style(to_abspath, pool));
>
> Why not make the wording of this error message similar to the one that
> is given when exporting a directory hits an obstruction?
>
> SVN_ERR_W(err, _("Destination directory exists, and will not be "
> "overwritten unless forced"));
>
> (Including the path in the error message is good; I think we should
> tweak the directory-hits-an-obstruction error message to do so as well.)
>
Incorporated for file. For directory I will come up another patch.
Please find attached modified patch.
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1071585)
+++ 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 1071585)
+++ subversion/tests/cmdline/export_tests.py (working copy)
@@ -456,7 +456,6 @@
'.', expected_output,
expected_disk)
-@XFail()
@Issue(3799)
def export_file_overwrite_fails(sbox):
"exporting a file refuses to silently overwrite"
@@ -703,6 +702,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 +757,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 1071585)
+++ subversion/libsvn_client/export.c (working copy)
@@ -524,7 +524,21 @@
}
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) && ! 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));
+ }
+
SVN_ERR(copy_one_versioned_file(from_abspath, to_abspath, ctx->wc_ctx,
revision, native_eol, ignore_keywords,
pool));
