You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2009/11/13 16:43:18 UTC
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 <cm...@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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417634
Re: svn commit: r40493 - in trunk/subversion: libsvn_fs_base
libsvn_fs_fs libsvn_ra_neon libsvn_ra_serf tests/cmdline
Posted by "C. Michael Pilato" <cm...@collab.net>.
Paul Burba wrote:
>> 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.
Yes, right. Mentally interpreted the SVN_IS_VALID_REVNUM in reverse.
>> 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
I didn't read this part of your previous mail well enough -- my bad:
* If @a current_rev is a valid revnum, then do an out-of-dateness
* check. If the revnum is less than the last-changed-revision of @a
* path (or if @a path doesn't exist in HEAD), return
* #SVN_ERR_FS_OUT_OF_DATE.
So I'm guessing that args->current_rev is the working copy revision if you
do 'svn lock /path/to/working_copy', and SVN_INVALID_REVNUM if you do 'svn
lock URL'. But regardless, the path check is versus HEAD, so the error
message should report it as such. I get it now. Carry on.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417721
Re: svn commit: r40493 - in trunk/subversion: libsvn_fs_base
libsvn_fs_fs libsvn_ra_neon libsvn_ra_serf tests/cmdline
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Nov 13, 2009 at 11:43 AM, C. Michael Pilato <cm...@collab.net> wrote:
> Paul Burba wrote:
>> On Fri, Nov 13, 2009 at 1:59 AM, C. Michael Pilato <cm...@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@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