On Fri, Nov 13, 2009 at 11:43 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> 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*.
Sorry Mike, I misunderstood your concern :-\
> Should the error in the 'else' clause
I assume you mean the first error? The error in the else clause if
for when *no* valid current rev is specified.
> 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.
We shouldn't say that in either case. If we are trying to lock a file
in a WC then current_rev is that path's base revision, which does
exist, the problem is that the path doesn't exist at HEAD -- here is
the context you need to see this (In particular note that we call
svn_fs_base__get_path_kind, which give us the kind of
args->path_at_HEAD):
static svn_error_t *
txn_body_lock(void *baton, trail_t *trail)
{
struct lock_args *args = baton;
svn_node_kind_t kind = svn_node_file;
svn_lock_t *existing_lock;
const char *fs_username;
svn_lock_t *lock;
SVN_ERR(svn_fs_base__get_path_kind(&kind, args->path, trail, trail->pool));
/* Until we implement directory locks someday, we only allow locks
on files or non-existent paths. */
if (kind == svn_node_dir)
return SVN_FS__ERR_NOT_FILE(trail->fs, args->path);
/* While our locking implementation easily supports the locking of
nonexistent paths, we deliberately choose not to allow such madness. */
if (kind == svn_node_none)
{
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, args->current_rev);
else
return svn_error_createf(
SVN_ERR_FS_NOT_FOUND, NULL,
_("Path '%s' doesn't exist in HEAD revision"),
args->path);
}
Perhaps the first error should say something to the effect of 'the
path in your working copy no longer exists at head', e.g.:
[[[
Index: subversion/libsvn_fs_base/lock.c
===================================================================
--- subversion/libsvn_fs_base/lock.c (revision 40495)
+++ subversion/libsvn_fs_base/lock.c (working copy)
@@ -102,8 +102,8 @@
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);
+ _("Path '%s@%d' doesn't exist in HEAD revision"),
+ args->path, args->current_rev);
else
return svn_error_createf(
SVN_ERR_FS_NOT_FOUND, NULL,
]]]
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417673
Received on 2009-11-13 18:59:16 CET