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

On 12/9/05, Kamesh Jayachandran <kamesh@collab.net> wrote:
> Hi All,
> [[[
> returning the error in case file does not exist in the working copy.
> ]]]

Kamesh,

This patch seems pretty reasonable, but the log message is not
sufficient. Please look at
http://subversion.tigris.org/hacking.html#log-messages for a
description of the format the Subversion project uses for it's log
messages. While your message does describe what the change does, it
doesn't mention what files and functions were changed, and it doesn't
mention the issue you're fixing, both of which are important
information.

A log message for this kind of change should look something like this:

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

Also, I'm always kind of skeptical about changing low level stuff in
libsvn_wc because there are often unexpected side effects. Did you
run the test suite after making this change? Even if you can't add a
test for this specific problem we should still verify that it doesn't
break any existing tests.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 9 20:05:06 2005