Re: svn commit: r40493 - in trunk/subversion: libsvn_fs_base libsvn_fs_fs libsvn_ra_neon libsvn_ra_serf tests/cmdline

Paul Burba wrote:
> On Fri, Nov 13, 2009 at 1:59 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>> Paul T. Burba wrote:
>>> Author: pburba
>>> Date: Thu Nov 12 18:37:57 2009
>>> New Revision: 40493
>> [...]
>>
>>> Modified: trunk/subversion/libsvn_fs_base/lock.c
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_base/lock.c?pathrev=40493&r1=40492&r2=40493
>>> ==============================================================================
>>> --- trunk/subversion/libsvn_fs_base/lock.c Thu Nov 12 15:37:51 2009 (r40492)
>>> +++ trunk/subversion/libsvn_fs_base/lock.c Thu Nov 12 18:37:57 2009 (r40493)
>>> @@ -98,9 +98,18 @@ txn_body_lock(void *baton, trail_t *trai
>>> /* While our locking implementation easily supports the locking of
>>> nonexistent paths, we deliberately choose not to allow such madness. */
>>> if (kind == svn_node_none)
>>> - return svn_error_createf(SVN_ERR_FS_NOT_FOUND, NULL,
>>> - "Path '%s' doesn't exist in HEAD revision",
>>> - args->path);
>>> + {
>>> + if (SVN_IS_VALID_REVNUM(args->current_rev))
>>> + return svn_error_createf(
>>> + SVN_ERR_FS_OUT_OF_DATE, NULL,
>>> + _("Path '%s' doesn't exist in HEAD revision"),
>>> + args->path);
>>> + else
>>> + return svn_error_createf(
>>> + SVN_ERR_FS_NOT_FOUND, NULL,
>>> + _("Path '%s' doesn't exist in HEAD revision"),
>>> + args->path);
>>> + }
>> Did you intend to leave both error messages the same even though the error
>> *codes* differ? That seems non-normal, but perhaps has good reasoning in
>> this case that I can't detect from the provided context?
>
> Mike,
>
> I kept the old error in place if ((LOCK_ARGS *)BATON)->CURRENT_REV was
> invalid because I wasn't sure if any callers depend on that.

It's not the continued use of the old error code that concerns me. It's the
weirdness that this function can return two different error codes that both
use the same *error message*.

Should the error in the 'else' clause there say something like, "Path 'foo'
doesn't exist in revision REV?" instead of mentioning "HEAD"? Like I said,
maybe not -- I can't see enough of the function in your diff to know.

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417634