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

Julian Foad <julianfoad@btopenworld.com> writes:
> 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.

True, though in general we've taken the attitude that it's okay to
break scripts that depend on a buggy behavior (unless it would cause
dataloss in some obvious way, of course).

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

(Note: whatever we consense on will go into hacking.html, of course.)

Here's a policy proposal:

   "A Subversion command should, by default, error when asked to do
    something that it cannot do (e.g., remove a non-existent file).
    Here, "error" means both display a message on stderr and return
    non-success. However, it is acceptable for some commands to not
    error in this circumstance, iff passed the '--force' flag. This
    is a convenience for scripts that don't need to bother with
    sophisticated error handling."

Thoughts?

I picked "--force" not because it's a completely natural semantic fit
(it's not quite), but because other Unix commands seem to treat it
this way, so some users may be familiar with the convention. Also, we
already have it, and I was reluctant to add Yet Another Flag just for
this.

-Karl

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