On Mar 10, 2005, at 6:03 PM, Philip Martin wrote:
> fitz@tigris.org writes:
>
>> Author: fitz
>> Date: Thu Mar 10 15:52:46 2005
>> New Revision: 13359
>
>> --- branches/locking/subversion/include/svn_error.h (original)
>> +++ branches/locking/subversion/include/svn_error.h Thu Mar 10
>> 15:52:46 2005
>> @@ -218,6 +218,21 @@
>>
>> /** @} */
>>
>> +/*
>> + * @since New in 1.2.
>> + *
>> + * Return TRUE if @a err is an error specifically related to locking
>> a
>> + * path, FALSE otherwise. */
>> +svn_boolean_t svn_error_is_lock_error (svn_error_t *err);
>
> The implementation allows err to be NULL, but the documentation
> doesn't mention it which means callers cannot rely on it.
Will fix.
>> +
>> +/*
>> + * @since New in 1.2.
>> + *
>> + * Return TRUE if @a err is an error specifically related to
>> unlocking
>> + * a path, FALSE otherwise. */
>> +svn_boolean_t svn_error_is_unlock_error (svn_error_t *err);
>
> ditto
ditto.
>> +
>> +
>> #ifdef __cplusplus
>> }
>> #endif /* __cplusplus */
>>
>
>> ---
>> branches/locking/subversion/libsvn_client/locking_commands.c
>> (original)
>> +++ branches/locking/subversion/libsvn_client/locking_commands.c Thu
>> Mar 10 15:52:46 2005
>> @@ -104,18 +104,24 @@
>> struct lock_baton *lb = baton;
>> svn_wc_adm_access_t *adm_access;
>> const char *abs_path;
>> -
>> +
>> abs_path = svn_path_join (svn_wc_adm_access_path (lb->adm_access),
>> path, lb->pool);
>> -
>> +
>> SVN_ERR (svn_wc_adm_probe_retrieve (&adm_access, lb->adm_access,
>> abs_path, lb->pool));
>> -
>> +
>> if (do_lock)
>> - SVN_ERR (svn_wc_add_lock (abs_path, lock, adm_access, lb->pool));
>> - else
>> - SVN_ERR (svn_wc_remove_lock (abs_path, adm_access, lb->pool));
>> + if (!ra_err)
>> + SVN_ERR (svn_wc_add_lock (abs_path, lock, adm_access,
>> lb->pool));
>>
>> + if (!do_lock)
>> + /* Remove our wc lock token either a) if we got no error, or b)
>> if
>> + we got any locking-related error except for owner mismatch. */
>> + if (!ra_err ||
>> + (ra_err && (ra_err->apr_err !=
>> SVN_ERR_FS_LOCK_OWNER_MISMATCH)))
>> + SVN_ERR (svn_wc_remove_lock (abs_path, adm_access, lb->pool));
>
> The comment reads "any locking-related error except for owner mismatch"
> but the code appears to implement "any error except for owner
> mismatch".
> Now I suppose you might have meant that all the ra_err errors are
> "locking-related", but perhaps you missed out some code?
>
> I'd prefer it if the comment and code were a more obvious match,
> that's assuming that the comment really is adding value; merely
> duplicating the code doesn't add much.
I just need to better document that the callback will only ever get
passed locking/unlocking related errors (ruled as such by
svn_error_is_(un)lock_error).
>> --- branches/locking/subversion/libsvn_ra_dav/session.c (original)
>> +++ branches/locking/subversion/libsvn_ra_dav/session.c Thu Mar 10
>> 15:52:46 2005
>> @@ -1104,17 +1104,28 @@
>> apr_ssize_t keylen;
>> void *val;
>> svn_revnum_t *revnum;
>> + svn_error_t *err, *callback_err;
>>
>> apr_hash_this(hi, &key, &keylen, &val);
>> path = key;
>> revnum = val;
>>
>> - SVN_ERR (shim_svn_ra_dav__lock(session, &lock, path, comment,
>> - force, *revnum, pool));
>> + err = shim_svn_ra_dav__lock(session, &lock, path, comment,
>> + force, *revnum, pool);
>> +
>> + if (err && !svn_error_is_lock_error (err))
>
> The implementation checks for a NULL err (although you can't rely on
> that unless the documentation changes).
Will fix.
>> + return err;
>>
>> /* Run the lock callback if we have one. */
>> if (lock_func)
>> - SVN_ERR(lock_func(lock_baton, path, TRUE, lock, NULL));
>> + callback_err = lock_func(lock_baton, path, TRUE, lock, err);
>> +
>> + /* clear the error if there was one. */
>
> That comment is pointless, it adds nothing.
>
>> + if (err)
>> + svn_error_clear (err);
>
> svn_error_clear checks for NULL so the caller doesn't have to.
Will fix.
>> --- branches/locking/subversion/libsvn_subr/error.c (original)
>> +++ branches/locking/subversion/libsvn_subr/error.c Thu Mar 10
>> 15:52:46 2005
>> @@ -393,3 +393,27 @@
>>
>> return apr_strerror (statcode, buf, bufsize);
>> }
>> +
>> +svn_boolean_t
>> +svn_error_is_lock_error (svn_error_t *err)
>> +{
>> + if (err &&
>
> There is no point handling NULL inputs if you don't document it.
Will fix.
>> + (err->apr_err == SVN_ERR_FS_PATH_LOCKED ||
>> + err->apr_err == SVN_ERR_FS_OUT_OF_DATE))
>> + return TRUE;
>> + return FALSE;
>> +}
>> +
>> +svn_boolean_t
>> +svn_error_is_unlock_error (svn_error_t *err)
>> +{
>> + if (err &&
>
> ditto
ditto.
>> + (err->apr_err == SVN_ERR_FS_PATH_NOT_LOCKED ||
>> + err->apr_err == SVN_ERR_FS_BAD_LOCK_TOKEN ||
>> + err->apr_err == SVN_ERR_FS_LOCK_OWNER_MISMATCH ||
>> + err->apr_err == SVN_ERR_FS_NO_SUCH_LOCK ||
>> + err->apr_err == SVN_ERR_RA_NOT_LOCKED ||
>> + err->apr_err == SVN_ERR_FS_LOCK_EXPIRED))
>> + return TRUE;
>> + return FALSE;
>> +}
Thanks for the review... I'll get these Friday morning (CST).
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 11 08:00:19 2005