Noorul Islam K M <noo...@collab.net> writes:
> Julian Foad <julian.f...@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>
> ]]]
>
>
The above log message is not correct. Please find below the correct
one.
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_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): Fix failing test by passing --force.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
>> * 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)
>
> -@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. */
>
