[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 13 Nov 2009 10:53:54 -0500

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

This is an archived mail posted to the Subversion Dev mailing list.