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

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

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