Hi Karl,
Thanks for your feedback.
Please find the attached patch and inline log.
With regards
Kamesh Jayachandran
[[[
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
moving the testcase to subversion/tests/svn/schedule_tests.py.
* subversion/tests/svn/schedule_tests.py
(unschedule_missing_added): Deleting set of working copy files,
fails on first unversioned file and subsequent files are not deleted
at all,
so seperating the deletion to 2 delete operations.
(delete_non_existent): Regression test for #2440
]]]
kfogel@collab.net wrote:
> Kamesh Jayachandran <kamesh@collab.net> writes:
>
>> Thanks Julian for your valuable feedback.
>> Please find the attached patch.
>>
>
> When posting a patch, please include the log message with it. Don't
> rely on a log message being quoted from previous mails -- the reader
> won't know if that one still applies. People generally look for all
> the important stuff to be at same top (unquoted) level of the mail.
>
> Review on the patch:
>
>
>>> 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.
>>>>
>>>> ]]]
>>>>
>
> I'm assuming this log message is still the one for the patch.
>
> Asterisk missing before "subversion/tests/svn/schedule_tests.py". No
> big deal, anyone could fix that when applying the patch.
>
> Your description of what happened in basic_delete isn't accurate now.
> We're not expecting an error anymore; instead, you've removed that
> line from the test entirely, on Julian's advice. I think Julian was
> too eager, though: we should test that we get an error when we expect
> to! So changing to SVNAnyOutput (as your original change did) was the
> right thing to do. However, it should probably be a whole new test in
> schedule_tests.py, not part of basic_tests.py. Can you do it that way?
>
> Your log message for unschedule_missing_added() includes a long code
> excerpt; I think it might be clearer to just say it with prose.
>
>
>> 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;
>>
>
> The error SVN_ERR_WC_NOT_FILE is for a file that is not versioned in
> this working copy; the error is not about existence. I recommend
> SVN_ERR_BAD_FILENAME instead.
>
> The error string claims that the file does not exist in the working
> copy and repository. That's not true -- how can the client know
> anything about the repository? :-) All we know is that %s does not
> exist, so just say that:
>
> return svn_error_createf
> (SVN_ERR_WC_NOT_FILE, NULL,
> _("'%s' does not exist"), svn_path_local_style (path));
>
> Note two other things there:
>
> 1. I adjusted the formatting so as not to exceed 80 columns. Please
> see www/hacking.html about that.
>
> 2. For inclusion in a user-destined error, we need to set path (which
> is utf8) back the local style, using svn_path_local_style().
>
>
>> 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'
>>
>
> Again, I liked your earlier code (using SVNAnyOutput instead of "[]"),
> but it should be a separate, independent test, in schedule_tests.py
> not in basic_tests.py.
>
>
>> 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"
>>
>
> The new language is confusing. How will "'svn rm'(with errors)" make
> anything useful happen? You might want to explain to the reader that
> even though it errors, it does do the right thing in the .svn/entries
> file first (i.e., unschedules the addition).
>
> Looking forward to the next patch,
> -Karl
>
>
Index: libsvn_wc/adm_ops.c
===================================================================
--- libsvn_wc/adm_ops.c (revision 17664)
+++ 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_BAD_FILENAME, NULL,
+ _("'%s' does not exist"), svn_path_local_style(path, pool));
break;
default:
Index: tests/svn/basic_tests.py
===================================================================
--- tests/svn/basic_tests.py (revision 17664)
+++ 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: tests/svn/schedule_tests.py
===================================================================
--- tests/svn/schedule_tests.py (revision 17664)
+++ tests/svn/schedule_tests.py (working copy)
@@ -446,8 +446,9 @@
# 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' or 'svn rm' will make that happen by removing the entry from
+# .svn/entries file. While 'svn revert' does with no error,
+# 'svn rm' does it with error.
def unschedule_missing_added(sbox):
"unschedule addition on missing items"
@@ -484,7 +485,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.
@@ -656,6 +658,21 @@
os.chdir(saved_wd)
+#----------------------------------------------------------------------
+# Regression test for #2440
+# Ideally this test should test for the exit status of the
+# 'svn rm non-existent' invocation.
+# As the corresponding change to get the exit code of svn binary invoked needs
+# a change in many testcase, for now this testcase checks the stderr.
+def delete_non_existent(sbox):
+ "'svn rm non-existent' should exit with an error"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ svntest.actions.run_and_verify_svn(None, None, SVNAnyOutput,
+ 'rm', '--force', 'non-existent')
+
+
########################################################################
# Run the tests
@@ -686,6 +703,7 @@
status_add_deleted_directory,
add_recursive_already_versioned,
fail_add_directory,
+ delete_non_existent,
]
if __name__ == '__main__':
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Dec 25 03:23:13 2005