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. 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
Received on 2009-11-13 16:54:12 CET