You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Noorul Islam K M <no...@collab.net> on 2011/02/15 09:40:05 UTC
[PATCH] Fix for issue 3799 - exporting file should not overwrite
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.
* 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
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
Thanks. Committed in r1129621. I made some minor cosmetic tweaks to
the code and changed some wording in the log message.
- Julian
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Tue, 2011-05-31 at 14:52 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Noorul Islam K M <no...@collab.net> writes:
>> >> > Julian Foad <ju...@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>
]]]
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2011-05-31 at 14:52 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > Noorul Islam K M wrote:
> >
> >> Noorul Islam K M <no...@collab.net> writes:
> >> > Julian Foad <ju...@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.
- Julian
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Noorul Islam K M <no...@collab.net> writes:
>> > Julian Foad <ju...@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.
Now is it ok to mark the failing test as XFail and proceed with this
patch?
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Noorul Islam K M <no...@collab.net> writes:
> > Julian Foad <ju...@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
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@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. */
>
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@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
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
Just taking a quick look.
* Please always include the log message with the patch.
* 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?)
* 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
]]]
Thanks.
- Julian
On Tue, 2011-05-24 at 16:52 +0530, Noorul Islam K M wrote:
> 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,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
>
> @@ -899,6 +933,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 kind;
> +
> + 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_ILLEGAL_TARGET, 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_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_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_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. */
>
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Noorul Islam K M wrote on Tue, May 24, 2011 at 15:39:10 +0530:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>> > Noorul Islam K M <no...@collab.net> writes:
>> >
>> >> Index: subversion/libsvn_client/export.c
>> >> ===================================================================
>> >> --- subversion/libsvn_client/export.c (revision 1071880)
>> >> +++ subversion/libsvn_client/export.c (working copy)
>> >> @@ -524,7 +524,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) && ! 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));
>> >
>> > That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
>> > the repository.
>> >
>>
>> I could see usage of the same at several places in libsvn_client.
>>
>
> To indicate that a path doesn't exist in the repository, which isn't the
> case here.
As discussed on IRC updated patch to use SVN_ERR_ILLEGAL_TARGET.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Noorul Islam K M wrote on Tue, May 24, 2011 at 15:39:10 +0530:
> Philip Martin <ph...@wandisco.com> writes:
>
> > Noorul Islam K M <no...@collab.net> writes:
> >
> >> Index: subversion/libsvn_client/export.c
> >> ===================================================================
> >> --- subversion/libsvn_client/export.c (revision 1071880)
> >> +++ subversion/libsvn_client/export.c (working copy)
> >> @@ -524,7 +524,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) && ! 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));
> >
> > That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
> > the repository.
> >
>
> I could see usage of the same at several places in libsvn_client.
>
To indicate that a path doesn't exist in the repository, which isn't the
case here.
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Philip Martin <ph...@wandisco.com> writes:
> Noorul Islam K M <no...@collab.net> writes:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> Noorul Islam K M <no...@collab.net> writes:
>>>
>>>> Index: subversion/libsvn_client/export.c
>>>> ===================================================================
>>>> --- subversion/libsvn_client/export.c (revision 1071880)
>>>> +++ subversion/libsvn_client/export.c (working copy)
>>>> @@ -524,7 +524,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) && ! 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));
>>>
>>> That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
>>> the repository.
>>>
>>
>> I could see usage of the same at several places in libsvn_client.
>
> It's still the wrong error to use here. The client layer might use it
> to indicate that some path exists in the repository, but it's wrong to
> use it to indicate that something exists on the local disk.
I used SVN_ERR_ENTRY_EXISTS and looks like it is better.
Please find attached modified patch.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Philip Martin <ph...@wandisco.com>.
Noorul Islam K M <no...@collab.net> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Noorul Islam K M <no...@collab.net> writes:
>>
>>> Index: subversion/libsvn_client/export.c
>>> ===================================================================
>>> --- subversion/libsvn_client/export.c (revision 1071880)
>>> +++ subversion/libsvn_client/export.c (working copy)
>>> @@ -524,7 +524,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) && ! 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));
>>
>> That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
>> the repository.
>>
>
> I could see usage of the same at several places in libsvn_client.
It's still the wrong error to use here. The client layer might use it
to indicate that some path exists in the repository, but it's wrong to
use it to indicate that something exists on the local disk.
--
Philip
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Philip Martin <ph...@wandisco.com> writes:
> Noorul Islam K M <no...@collab.net> writes:
>
>> Index: subversion/libsvn_client/export.c
>> ===================================================================
>> --- subversion/libsvn_client/export.c (revision 1071880)
>> +++ subversion/libsvn_client/export.c (working copy)
>> @@ -524,7 +524,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) && ! 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));
>
> That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
> the repository.
>
I could see usage of the same at several places in libsvn_client.
>> + }
>> + 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));
>
> Why do we have these checks here as well as open_root_internal?
open_root_internal is not called in these situations.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Philip Martin <ph...@wandisco.com>.
Noorul Islam K M <no...@collab.net> writes:
> Index: subversion/libsvn_client/export.c
> ===================================================================
> --- subversion/libsvn_client/export.c (revision 1071880)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -524,7 +524,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) && ! 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));
That's the wrong error, SVN_ERR_FS_ is for the Subversion filesystem in
the repository.
> + }
> + 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));
Why do we have these checks here as well as open_root_internal?
--
Philip
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Stefan Sperling <st...@elego.de> writes:
> On Fri, May 20, 2011 at 01:04:18PM +0200, Stefan Sperling wrote:
>
>> On Fri, May 20, 2011 at 03:25:50PM +0530, Noorul Islam K M wrote:
>> > Pinging again.
>>
>> Looking into this...
>
> Noorul,
>
> unfortunately your patch does not apply cleanly to trunk anymore.
> Can you update it? Thanks!
Attached is the updated patch.
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
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 20, 2011 at 01:04:18PM +0200, Stefan Sperling wrote:
> On Fri, May 20, 2011 at 03:25:50PM +0530, Noorul Islam K M wrote:
> > Pinging again.
>
> Looking into this...
Noorul,
unfortunately your patch does not apply cleanly to trunk anymore.
Can you update it? Thanks!
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 20, 2011 at 03:25:50PM +0530, Noorul Islam K M wrote:
> Pinging again.
Looking into this...
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Noorul Islam K M <no...@collab.net> writes:
>
>> Gavin Baumanis <ga...@thespidernet.com> writes:
>>
>>> Hi Noorul,
>>>
>>> Just a quick message to see if you're still working on this patch or not?
>>> My threads stop at the comments below from Andrew?
>>>
>>
>> Someone has to review this and if everything is fine commit it. Julian
>> did review this earlier and I submitted updated patch. After that this
>> thread has got no updates.
>>
>
> Pinging again. This has been pending for long time now.
>
Pinging again.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Gavin Baumanis <ga...@thespidernet.com> writes:
>
>> Hi Noorul,
>>
>> Just a quick message to see if you're still working on this patch or not?
>> My threads stop at the comments below from Andrew?
>>
>
> Someone has to review this and if everything is fine commit it. Julian
> did review this earlier and I submitted updated patch. After that this
> thread has got no updates.
>
Pinging again. This has been pending for long time now.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Gavin Baumanis <ga...@thespidernet.com> writes:
> Hi Noorul,
>
> Just a quick message to see if you're still working on this patch or not?
> My threads stop at the comments below from Andrew?
>
Someone has to review this and if everything is fine commit it. Julian
did review this earlier and I submitted updated patch. After that this
thread has got no updates.
Thanks and Regards
Noorul
>
>
> On 01/03/2011, at 12:25 AM, Bolstridge, Andrew wrote:
>
>>
>>> -----Original Message-----
>>> From: Julian Foad [mailto:julian.foad@wandisco.com]
>>> Sent: 25 February 2011 15:33
>>> To: Noorul Islam K M
>>> Cc: Subversion
>>> Subject: Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
>>>
>>> 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.
>>>
>> My 2p:
>>
>> Why would we want dirs to be handled differently? Surely, we want the behaviour to be "as expected" as possible, and if file export behaves differently to directory export... that's not so good.
>>
>> Also, does this work as intended when using externals? I recall trying to export directories using externals to the same location on the filesystem and finding that each external wiped the target before copying the files (imagine you have several C modules and want to export all the headers from each of them to a single include directory, as an example of what I was trying to achieve).
>>
>>
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Gavin Baumanis <ga...@thespidernet.com>.
Hi Noorul,
Just a quick message to see if you're still working on this patch or not?
My threads stop at the comments below from Andrew?
On 01/03/2011, at 12:25 AM, Bolstridge, Andrew wrote:
>
>> -----Original Message-----
>> From: Julian Foad [mailto:julian.foad@wandisco.com]
>> Sent: 25 February 2011 15:33
>> To: Noorul Islam K M
>> Cc: Subversion
>> Subject: Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
>>
>> 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.
>>
> My 2p:
>
> Why would we want dirs to be handled differently? Surely, we want the behaviour to be "as expected" as possible, and if file export behaves differently to directory export... that's not so good.
>
> Also, does this work as intended when using externals? I recall trying to export directories using externals to the same location on the filesystem and finding that each external wiped the target before copying the files (imagine you have several C modules and want to export all the headers from each of them to a single include directory, as an example of what I was trying to achieve).
>
>
RE: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by "Bolstridge, Andrew" <an...@intergraph.com>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: 25 February 2011 15:33
> To: Noorul Islam K M
> Cc: Subversion
> Subject: Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
>
> 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.
>
My 2p:
Why would we want dirs to be handled differently? Surely, we want the behaviour to be "as expected" as possible, and if file export behaves differently to directory export... that's not so good.
Also, does this work as intended when using externals? I recall trying to export directories using externals to the same location on the filesystem and finding that each external wiped the target before copying the files (imagine you have several C modules and want to export all the headers from each of them to a single include directory, as an example of what I was trying to achieve).
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@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
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@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
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
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.
- Julian
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@wandisco.com> writes:
>
>> On Fri, 2011-02-18 at 15:54 +0530, Noorul Islam K M wrote:
>>
>>> Noorul Islam K M <no...@collab.net> writes:
>>>
>>> > Julian Foad <ju...@wandisco.com> writes:
>>> >
>>> >> On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
>>> >>
>>> >>> Julian Foad <ju...@wandisco.com> writes:
>>> >>>
>>> >>> > Noorul Islam K M wrote:
>>> >>> >
>>> >>> >> Julian Foad <ju...@wandisco.com> writes:
>>> >>> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
>>> >>> >> >> + 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.
>>> >>> >
>>> >>> > I'm asking about obstruction *by* a directory (or symlink or ...), in
>>> >>> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
>>> >>> >
>>> >>>
>>> >>> Are you talking about this scenario?
>>> >>>
>>> >>> noorul@laptop:/tmp/wc/testrepo$ touch iota
>>> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
>>> >>> A iota
>>> >>> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
>>> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
>>> >>> A /tmp/iota/iota
>>> >>> Export complete.
>>> >>> noorul@laptop:/tmp/wc/testrepo$
>>> >>
>>> >> Yes, this scenario and other scenarios similar to this where /tmp/iota
>>> >> could instead be a directory with children (and maybe one of the
>>> >> children is called 'iota'), or could be a symlink or a special file.
>>> >>
>>> >
>>> > I modified the patch so that if the target exists as a child directory
>>> > then throw the following error message.
>>> >
>>> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite
>>> > directory with non-directory
>>
>> Ah, a child of a directory - I see. I hadn't appreciated that if the
>> specified target is a directory ('/tmp/iota' here) then it will try to
>> export into a child of that directory.
>>
>> So now you have covered these cases:
>>
>> If specified target ('/tmp/iota' in the example above) is a ...
>> nothing -> fine
>> file -> if 'force' then overwrite else error
>> symlink -> ???
>> special -> ???
>> dir -> if sub-target ('/tmp/iota/iota') is a ...
>> nothing -> fine
>> file -> if 'force' then overwrite else error
>> symlink -> ???
>> special -> error
>> dir -> error
>>
>> Is that right?
>>
>> - Julian
>>
>>
>
> 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
>
> Thanks and Regards
> Noorul
>
>>> > With the latest patch I tested the following
>>> >
>>> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svnadmin create /tmp/testrepo
>>> >
>>> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svn co file:///tmp/testrepo
>>> > Checked out revision 0.
>>> >
>>> > noorul@noorul:/tmp/wc$ cd testrepo
>>> >
>>> > noorul@noorul:/tmp/wc/testrepo$ touch iota
>>> >
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn add iota
>>> > A iota
>>> >
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn ci -m "Adding file"
>>> > Adding iota
>>> > Transmitting file data .
>>> > Committed revision 1.
>>> >
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn up
>>> > Updating '.' ...
>>> > At revision 1.
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp
>>> > A /tmp/iota
>>> > Export complete.
>>> >
>>> > # Child directory
>>> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota
>>> > noorul@noorul:/tmp/wc/testrepo$ mkdir -p /tmp/iota/iota
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
>>> > ../subversion/svn/export-cmd.c:123: (apr_err=160020)
>>> > ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
>>> > ../subversion/libsvn_client/export.c:542: (apr_err=160020)
>>> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite directory with non-directory
>>> >
>>> > # Special file
>>> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota/iota
>>> > noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
>>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
>>> > A /tmp/iota/iota
>>> > Export complete.
>>> >
>>>
>>> Sorry the above should read
>>>
>>> # Special file
>>> noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
>>>
>>> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/
>>> ../subversion/svn/export-cmd.c:123: (apr_err=160020)
>>> ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
>>> ../subversion/libsvn_client/export.c:535: (apr_err=160020)
>>> svn: E160020: Destination file '/tmp/iota/iota' exists, and will not be overwritten unless forced
>>>
>>> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/ --force
>>> A /tmp/iota/iota
>>> Export complete.
>>>
>>> > For symlink it successfully overwrites.
>>> >
>>>
>>> I meant symlink type files behave the same as files. I forgot to attach
>>> the latest patch. Please find attached the latest one.
>>>
>>> > Update log message is here
>>> >
>>> > 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, child directory.
>>> >
>>> > * 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
>>>
>>> plain text document attachment (3799.txt)
>>> Index: subversion/tests/cmdline/externals_tests.py
>>> ===================================================================
>>> --- subversion/tests/cmdline/externals_tests.py (revision 1071880)
>>> +++ 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 1071880)
>>> +++ 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 1071880)
>>> +++ subversion/libsvn_client/export.c (working copy)
>>> @@ -524,7 +524,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));
Pinging to get some attention to this thread.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Fri, 2011-02-18 at 15:54 +0530, Noorul Islam K M wrote:
>
>> Noorul Islam K M <no...@collab.net> writes:
>>
>> > Julian Foad <ju...@wandisco.com> writes:
>> >
>> >> On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
>> >>
>> >>> Julian Foad <ju...@wandisco.com> writes:
>> >>>
>> >>> > Noorul Islam K M wrote:
>> >>> >
>> >>> >> Julian Foad <ju...@wandisco.com> writes:
>> >>> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
>> >>> >> >> + 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.
>> >>> >
>> >>> > I'm asking about obstruction *by* a directory (or symlink or ...), in
>> >>> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
>> >>> >
>> >>>
>> >>> Are you talking about this scenario?
>> >>>
>> >>> noorul@laptop:/tmp/wc/testrepo$ touch iota
>> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
>> >>> A iota
>> >>> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
>> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
>> >>> A /tmp/iota/iota
>> >>> Export complete.
>> >>> noorul@laptop:/tmp/wc/testrepo$
>> >>
>> >> Yes, this scenario and other scenarios similar to this where /tmp/iota
>> >> could instead be a directory with children (and maybe one of the
>> >> children is called 'iota'), or could be a symlink or a special file.
>> >>
>> >
>> > I modified the patch so that if the target exists as a child directory
>> > then throw the following error message.
>> >
>> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite
>> > directory with non-directory
>
> Ah, a child of a directory - I see. I hadn't appreciated that if the
> specified target is a directory ('/tmp/iota' here) then it will try to
> export into a child of that directory.
>
> So now you have covered these cases:
>
> If specified target ('/tmp/iota' in the example above) is a ...
> nothing -> fine
> file -> if 'force' then overwrite else error
> symlink -> ???
> special -> ???
> dir -> if sub-target ('/tmp/iota/iota') is a ...
> nothing -> fine
> file -> if 'force' then overwrite else error
> symlink -> ???
> special -> error
> dir -> error
>
> Is that right?
>
> - Julian
>
>
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
Thanks and Regards
Noorul
>> > With the latest patch I tested the following
>> >
>> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svnadmin create /tmp/testrepo
>> >
>> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svn co file:///tmp/testrepo
>> > Checked out revision 0.
>> >
>> > noorul@noorul:/tmp/wc$ cd testrepo
>> >
>> > noorul@noorul:/tmp/wc/testrepo$ touch iota
>> >
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn add iota
>> > A iota
>> >
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn ci -m "Adding file"
>> > Adding iota
>> > Transmitting file data .
>> > Committed revision 1.
>> >
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn up
>> > Updating '.' ...
>> > At revision 1.
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp
>> > A /tmp/iota
>> > Export complete.
>> >
>> > # Child directory
>> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota
>> > noorul@noorul:/tmp/wc/testrepo$ mkdir -p /tmp/iota/iota
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
>> > ../subversion/svn/export-cmd.c:123: (apr_err=160020)
>> > ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
>> > ../subversion/libsvn_client/export.c:542: (apr_err=160020)
>> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite directory with non-directory
>> >
>> > # Special file
>> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota/iota
>> > noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
>> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
>> > A /tmp/iota/iota
>> > Export complete.
>> >
>>
>> Sorry the above should read
>>
>> # Special file
>> noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
>>
>> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/
>> ../subversion/svn/export-cmd.c:123: (apr_err=160020)
>> ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
>> ../subversion/libsvn_client/export.c:535: (apr_err=160020)
>> svn: E160020: Destination file '/tmp/iota/iota' exists, and will not be overwritten unless forced
>>
>> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/ --force
>> A /tmp/iota/iota
>> Export complete.
>>
>> > For symlink it successfully overwrites.
>> >
>>
>> I meant symlink type files behave the same as files. I forgot to attach
>> the latest patch. Please find attached the latest one.
>>
>> > Update log message is here
>> >
>> > 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, child directory.
>> >
>> > * 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
>>
>> plain text document attachment (3799.txt)
>> Index: subversion/tests/cmdline/externals_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/externals_tests.py (revision 1071880)
>> +++ 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 1071880)
>> +++ 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 1071880)
>> +++ subversion/libsvn_client/export.c (working copy)
>> @@ -524,7 +524,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));
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2011-02-18 at 15:54 +0530, Noorul Islam K M wrote:
> Noorul Islam K M <no...@collab.net> writes:
>
> > Julian Foad <ju...@wandisco.com> writes:
> >
> >> On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
> >>
> >>> Julian Foad <ju...@wandisco.com> writes:
> >>>
> >>> > Noorul Islam K M wrote:
> >>> >
> >>> >> Julian Foad <ju...@wandisco.com> writes:
> >>> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
> >>> >> >> + 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.
> >>> >
> >>> > I'm asking about obstruction *by* a directory (or symlink or ...), in
> >>> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
> >>> >
> >>>
> >>> Are you talking about this scenario?
> >>>
> >>> noorul@laptop:/tmp/wc/testrepo$ touch iota
> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
> >>> A iota
> >>> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
> >>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
> >>> A /tmp/iota/iota
> >>> Export complete.
> >>> noorul@laptop:/tmp/wc/testrepo$
> >>
> >> Yes, this scenario and other scenarios similar to this where /tmp/iota
> >> could instead be a directory with children (and maybe one of the
> >> children is called 'iota'), or could be a symlink or a special file.
> >>
> >
> > I modified the patch so that if the target exists as a child directory
> > then throw the following error message.
> >
> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite
> > directory with non-directory
Ah, a child of a directory - I see. I hadn't appreciated that if the
specified target is a directory ('/tmp/iota' here) then it will try to
export into a child of that directory.
So now you have covered these cases:
If specified target ('/tmp/iota' in the example above) is a ...
nothing -> fine
file -> if 'force' then overwrite else error
symlink -> ???
special -> ???
dir -> if sub-target ('/tmp/iota/iota') is a ...
nothing -> fine
file -> if 'force' then overwrite else error
symlink -> ???
special -> error
dir -> error
Is that right?
- Julian
> > With the latest patch I tested the following
> >
> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svnadmin create /tmp/testrepo
> >
> > noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svn co file:///tmp/testrepo
> > Checked out revision 0.
> >
> > noorul@noorul:/tmp/wc$ cd testrepo
> >
> > noorul@noorul:/tmp/wc/testrepo$ touch iota
> >
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn add iota
> > A iota
> >
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn ci -m "Adding file"
> > Adding iota
> > Transmitting file data .
> > Committed revision 1.
> >
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn up
> > Updating '.' ...
> > At revision 1.
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp
> > A /tmp/iota
> > Export complete.
> >
> > # Child directory
> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota
> > noorul@noorul:/tmp/wc/testrepo$ mkdir -p /tmp/iota/iota
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
> > ../subversion/svn/export-cmd.c:123: (apr_err=160020)
> > ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
> > ../subversion/libsvn_client/export.c:542: (apr_err=160020)
> > svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite directory with non-directory
> >
> > # Special file
> > noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota/iota
> > noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
> > noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
> > A /tmp/iota/iota
> > Export complete.
> >
>
> Sorry the above should read
>
> # Special file
> noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
>
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/
> ../subversion/svn/export-cmd.c:123: (apr_err=160020)
> ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
> ../subversion/libsvn_client/export.c:535: (apr_err=160020)
> svn: E160020: Destination file '/tmp/iota/iota' exists, and will not be overwritten unless forced
>
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/ --force
> A /tmp/iota/iota
> Export complete.
>
> > For symlink it successfully overwrites.
> >
>
> I meant symlink type files behave the same as files. I forgot to attach
> the latest patch. Please find attached the latest one.
>
> > Update log message is here
> >
> > 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, child directory.
> >
> > * 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
>
> plain text document attachment (3799.txt)
> Index: subversion/tests/cmdline/externals_tests.py
> ===================================================================
> --- subversion/tests/cmdline/externals_tests.py (revision 1071880)
> +++ 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 1071880)
> +++ 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 1071880)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -524,7 +524,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));
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@wandisco.com> writes:
>
>> On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
>>
>>> Julian Foad <ju...@wandisco.com> writes:
>>>
>>> > Noorul Islam K M wrote:
>>> >
>>> >> Julian Foad <ju...@wandisco.com> writes:
>>> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
>>> >> >> + 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.
>>> >
>>> > I'm asking about obstruction *by* a directory (or symlink or ...), in
>>> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
>>> >
>>>
>>> Are you talking about this scenario?
>>>
>>> noorul@laptop:/tmp/wc/testrepo$ touch iota
>>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
>>> A iota
>>> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
>>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
>>> A /tmp/iota/iota
>>> Export complete.
>>> noorul@laptop:/tmp/wc/testrepo$
>>
>> Yes, this scenario and other scenarios similar to this where /tmp/iota
>> could instead be a directory with children (and maybe one of the
>> children is called 'iota'), or could be a symlink or a special file.
>>
>
> I modified the patch so that if the target exists as a child directory
> then throw the following error message.
>
> svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite
> directory with non-directory
>
> With the latest patch I tested the following
>
> noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svnadmin create /tmp/testrepo
>
> noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svn co file:///tmp/testrepo
> Checked out revision 0.
>
> noorul@noorul:/tmp/wc$ cd testrepo
>
> noorul@noorul:/tmp/wc/testrepo$ touch iota
>
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn add iota
> A iota
>
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn ci -m "Adding file"
> Adding iota
> Transmitting file data .
> Committed revision 1.
>
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn up
> Updating '.' ...
> At revision 1.
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp
> A /tmp/iota
> Export complete.
>
> # Child directory
> noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota
> noorul@noorul:/tmp/wc/testrepo$ mkdir -p /tmp/iota/iota
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
> ../subversion/svn/export-cmd.c:123: (apr_err=160020)
> ../subversion/libsvn_client/export.c:1188: (apr_err=160020)
> ../subversion/libsvn_client/export.c:542: (apr_err=160020)
> svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite directory with non-directory
>
> # Special file
> noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota/iota
> noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
> noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
> A /tmp/iota/iota
> Export complete.
>
Sorry the above should read
# Special file
noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/
../subversion/svn/export-cmd.c:123: (apr_err=160020)
../subversion/libsvn_client/export.c:1188: (apr_err=160020)
../subversion/libsvn_client/export.c:535: (apr_err=160020)
svn: E160020: Destination file '/tmp/iota/iota' exists, and will not be overwritten unless forced
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota/ --force
A /tmp/iota/iota
Export complete.
> For symlink it successfully overwrites.
>
I meant symlink type files behave the same as files. I forgot to attach
the latest patch. Please find attached the latest one.
> Update log message is here
>
> 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, child directory.
>
> * 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
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
>> >> >> + 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.
>> >
>> > I'm asking about obstruction *by* a directory (or symlink or ...), in
>> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
>> >
>>
>> Are you talking about this scenario?
>>
>> noorul@laptop:/tmp/wc/testrepo$ touch iota
>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
>> A iota
>> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
>> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
>> A /tmp/iota/iota
>> Export complete.
>> noorul@laptop:/tmp/wc/testrepo$
>
> Yes, this scenario and other scenarios similar to this where /tmp/iota
> could instead be a directory with children (and maybe one of the
> children is called 'iota'), or could be a symlink or a special file.
>
I modified the patch so that if the target exists as a child directory
then throw the following error message.
svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite
directory with non-directory
With the latest patch I tested the following
noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svnadmin create /tmp/testrepo
noorul@noorul:/tmp/wc$ ~/projects/subversion/builds/trunk/bin/svn co file:///tmp/testrepo
Checked out revision 0.
noorul@noorul:/tmp/wc$ cd testrepo
noorul@noorul:/tmp/wc/testrepo$ touch iota
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn add iota
A iota
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn ci -m "Adding file"
Adding iota
Transmitting file data .
Committed revision 1.
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn up
Updating '.' ...
At revision 1.
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp
A /tmp/iota
Export complete.
# Child directory
noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota
noorul@noorul:/tmp/wc/testrepo$ mkdir -p /tmp/iota/iota
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
../subversion/svn/export-cmd.c:123: (apr_err=160020)
../subversion/libsvn_client/export.c:1188: (apr_err=160020)
../subversion/libsvn_client/export.c:542: (apr_err=160020)
svn: E160020: Destination /tmp/iota/iota exists. Cannot overwrite directory with non-directory
# Special file
noorul@noorul:/tmp/wc/testrepo$ rm -r -f /tmp/iota/iota
noorul@noorul:/tmp/wc/testrepo$ mknod /tmp/iota/iota p
noorul@noorul:/tmp/wc/testrepo$ ~/projects/subversion/builds/trunk/bin/svn export iota /tmp/iota
A /tmp/iota/iota
Export complete.
For symlink it successfully overwrites.
Update log message is here
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, child directory.
* 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
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2011-02-17 at 21:04 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > Noorul Islam K M wrote:
> >
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > On Tue, 2011-02-15, Noorul Islam K M wrote:
> >> >> + 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.
> >
> > I'm asking about obstruction *by* a directory (or symlink or ...), in
> > otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
> >
>
> Are you talking about this scenario?
>
> noorul@laptop:/tmp/wc/testrepo$ touch iota
> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
> A iota
> noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
> noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
> A /tmp/iota/iota
> Export complete.
> noorul@laptop:/tmp/wc/testrepo$
Yes, this scenario and other scenarios similar to this where /tmp/iota
could instead be a directory with children (and maybe one of the
children is called 'iota'), or could be a symlink or a special file.
- Julian
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > On Tue, 2011-02-15, Noorul Islam K M wrote:
>> >> + 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.
>
> I'm asking about obstruction *by* a directory (or symlink or ...), in
> otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
>
Are you talking about this scenario?
noorul@laptop:/tmp/wc/testrepo$ touch iota
noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn add iota
A iota
noorul@laptop:/tmp/wc/testrepo$ mkdir /tmp/iota
noorul@laptop:/tmp/wc/testrepo$ ~/projects/subversion/src/trunk/vpath/subversion/svn/svn export iota /tmp/iota
A /tmp/iota/iota
Export complete.
noorul@laptop:/tmp/wc/testrepo$
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > On Tue, 2011-02-15, Noorul Islam K M wrote:
> >> + 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.
I'm asking about obstruction *by* a directory (or symlink or ...), in
otyher words when (kind == svn_node_dir) or (kind == svn_node_unknown).
- Julian
Re: [PATCH] Fix for issue 3799 - exporting file should not overwrite
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@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
Re: [PATCH] Fix for issue 3799 - exporting file should not
overwrite
Posted by Julian Foad <ju...@wandisco.com>.
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.
> * 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?
> + 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)?
> + 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.)
> SVN_ERR(copy_one_versioned_file(from_abspath, to_abspath, ctx->wc_ctx,
> revision, native_eol, ignore_keywords,
> pool));
- Julian