Thanks Julian for your valuable feedback.
Please find the attached patch.
With regards
Kamesh Jayachandran
Julian Foad wrote:
> Thanks, Kamesh.
>
> Questions for the other Subversion developers:
>
> 1. Do we really want this to throw an error? People may have scripts
> that rely on it succeeding.
>
> 2. Where are we going to write down our policy on whether a command
> like this should throw an error or a warning? It's obvious that we
> need to, since the question crops up over and over again.
>
>
>
> Kamesh Jayachandran wrote:
>>
>> [[[
>> Fix issue #2440, svn rm nonexistent returns success.
>>
>> * subversion/libsvn_wc/adm_ops.c
>> (erase_unversioned_from_wc): Return an error in the case where the
>> file is not found.
>>
>>
>> * Fix tests
>> subversion/tests/svn/basic_tests.py
>> (basic_delete): Deleting non-existant unversioned item returns the
>> error so expecting the error.
>> subversion/tests/svn/schedule_tests.py
>> (unschedule_missing_added):svntest.main.run_svn(None, 'rm',
>> file1_path, dir1_path) fails while deleting file1_path and stops
>> immedeately not trying to delete the dir1_path because of the fix, so
>> seperating the deletion to 2 operations.
>>
>> ]]]
>
>
>> Index: subversion/libsvn_wc/adm_ops.c
>> ===================================================================
>> --- subversion/libsvn_wc/adm_ops.c (revision 17664)
>> +++ subversion/libsvn_wc/adm_ops.c (working copy)
>> @@ -667,7 +667,8 @@
>> switch (kind)
>> {
>> case svn_node_none:
>> - /* Nothing to do. */
>> + return svn_error_createf (SVN_ERR_WC_NOT_FILE, NULL,
>> + _("'%s' not existing in the working copy and repository"), path);
>> break;
>>
>> default:
>> Index: subversion/tests/svn/basic_tests.py
>> ===================================================================
>> --- subversion/tests/svn/basic_tests.py (revision 17664)
>> +++ subversion/tests/svn/basic_tests.py (working copy)
>> @@ -1093,7 +1093,7 @@
>> verify_file_deleted("Failed to remove unversioned file foo", foo_path)
>>
>> # Deleting non-existant unversioned item
>> - svntest.actions.run_and_verify_svn(None, None, [],
>> + svntest.actions.run_and_verify_svn(None, None, SVNAnyOutput,
>> 'rm', '--force', foo_path)
>
> What's the point of that action? It no longer tests anything, really.
> Might as well remove it.
>
>>
>> # At one stage deleting an URL dumped core
>> Index: subversion/tests/svn/schedule_tests.py
>> ===================================================================
>> --- subversion/tests/svn/schedule_tests.py (revision 17664)
>> +++ subversion/tests/svn/schedule_tests.py (working copy)
>> @@ -484,7 +484,8 @@
>> svntest.main.safe_rmtree(dir2_path)
>>
>> # Unschedule the additions, using 'svn rm' and 'svn revert'.
>> - svntest.main.run_svn(None, 'rm', file1_path, dir1_path)
>> + svntest.main.run_svn(SVNAnyOutput, 'rm', file1_path)
>> + svntest.main.run_svn(SVNAnyOutput, 'rm', dir1_path)
>> svntest.main.run_svn(None, 'revert', file2_path, dir2_path)
>
> That seems to contradict the comment at the top of the test that says
> we should be able to do this with no errors. Maybe the comment is now
> deemed wrong ... or something is.
>
> - Julian
>
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 17664)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -667,7 +667,8 @@
switch (kind)
{
case svn_node_none:
- /* Nothing to do. */
+ return svn_error_createf (SVN_ERR_WC_NOT_FILE, NULL,
+ _("'%s' not existing in the working copy and repository"), path);
break;
default:
Index: subversion/tests/svn/basic_tests.py
===================================================================
--- subversion/tests/svn/basic_tests.py (revision 17664)
+++ subversion/tests/svn/basic_tests.py (working copy)
@@ -1092,10 +1092,6 @@
'rm', '--force', foo_path)
verify_file_deleted("Failed to remove unversioned file foo", foo_path)
- # Deleting non-existant unversioned item
- svntest.actions.run_and_verify_svn(None, None, [],
- 'rm', '--force', foo_path)
-
# At one stage deleting an URL dumped core
iota_URL = svntest.main.current_repo_url + '/iota'
Index: subversion/tests/svn/schedule_tests.py
===================================================================
--- subversion/tests/svn/schedule_tests.py (revision 17664)
+++ subversion/tests/svn/schedule_tests.py (working copy)
@@ -446,8 +446,8 @@
# Suppose here is a scheduled-add file or directory which is
# also missing. If I want to make the working copy forget all
# knowledge of the item ("unschedule" the addition), then either 'svn
-# revert' or 'svn rm' will make that happen, with no errors. The
-# entry is simply removed from the entries file.
+# revert'(with no errors) or 'svn rm'(with errors) will make that happen.
+# The entry is simply removed from the entries file.
def unschedule_missing_added(sbox):
"unschedule addition on missing items"
@@ -484,7 +484,8 @@
svntest.main.safe_rmtree(dir2_path)
# Unschedule the additions, using 'svn rm' and 'svn revert'.
- svntest.main.run_svn(None, 'rm', file1_path, dir1_path)
+ svntest.main.run_svn(SVNAnyOutput, 'rm', file1_path)
+ svntest.main.run_svn(SVNAnyOutput, 'rm', dir1_path)
svntest.main.run_svn(None, 'revert', file2_path, dir2_path)
# 'svn st' should now show absolutely zero local mods.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 13 13:55:48 2005