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 06:59:43 UTC
Re: svn commit: r40493 - in trunk/subversion: libsvn_fs_base
libsvn_fs_fs libsvn_ra_neon libsvn_ra_serf tests/cmdline
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?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417435
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
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:
> 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 Paul Burba <pt...@gmail.com>.
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. The doc
string for svn_fs_lock() states that SVN_ERR_FS_OUT_OF_DATE is
returned *only* when CURRENT_REV is valid:
...
* 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.
*
* @note At this time, only files can be locked.
*/
svn_error_t *
svn_fs_lock(svn_lock_t **lock,
svn_fs_t *fs,
const char *path,
const char *token,
const char *comment,
svn_boolean_t is_dav_comment,
apr_time_t expiration_date,
svn_revnum_t current_rev,
svn_boolean_t steal_lock,
apr_pool_t *pool);
This also fits with the API for svn_ra_lock's API, which is specific
about when out-of-dateness checks are done:
...
* For each path, if its base revision (in @a path_revs) is a valid
* revnum, then do an out-of-dateness check. If the revnum is less
* than the last-changed-revision of any path (or if a path doesn't
* exist in HEAD), call @a lock_func/@a lock_baton with an
* SVN_ERR_RA_OUT_OF_DATE error.
*
* After successfully locking a file, @a lock_func is called with the
* @a lock_baton.
*
* Use @a pool for temporary allocations.
*
* @since New in 1.2.
*/
svn_error_t *
svn_ra_lock(svn_ra_session_t *session,
apr_hash_t *path_revs,
const char *comment,
svn_boolean_t steal_lock,
svn_ra_lock_callback_t lock_func,
void *lock_baton,
apr_pool_t *pool);
One thing this means in practical terms is that if you call
svn_client_lock() on a WC path that no longer exists at HEAD then you
get a warning, but if you call it on a URL that no longer exists (or
never existed) at HEAD then you get an SVN_ERR_FS_NOT_FOUND error. If
we return SVN_ERR_FS_OUT_OF_DATE uniformly then we would only ever get
the warning. I'm uncertain as to which is the right behavior, I was
only trying to adhere to what the APIs promised. FWIW, I checked, and
the lock tests pass if we return only SVN_ERR_FS_OUT_OF_DATE.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417598