Re: [PATCH]#2440 svn rm nonexistent exits with success

Kamesh Jayachandran <kamesh@collab.net> writes:
> [[[
> 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
>
> ]]]

Looks good to me. Watch out for slightly inconsistent formatting, and
remember to mention the change to 'test_list' in schedule_tests.py. I
took care of these issues when committing, just mentioning for next
time.

> Index: libsvn_wc/adm_ops.c
> ===================================================================
> --- libsvn_wc/adm_ops.c (revision 17664)
> +++ libsvn_wc/adm_ops.c (working copy)

Ooops, the patch should apply from top of trunk, that is, that should
be 'subversion/libsvn_wc/adm_ops.c' above. Same for the other files.

No big deal, I adjusted it. Just letting you know for next time.

> @@ -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;

The code looks fine. The indentation is off, and there should be a
space before the paren in the svn_path_local_style call. I fixed
those.

> 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'

Heh, r17860 changed it to "non-existent", making the patch not apply.
I didn't think of that when I applied all those spelling changes. Not
your fault, of course.

Rest looks great -- applied in r17993.

Thanks,
-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org