On Jul 7, 2009, at 10:35 AM, Greg Stein wrote:
> On Tue, Jun 2, 2009 at 19:17, Hyrum K. Wright<hyrum_at_hyrumwright.org>
> wrote:
>> ...
>> +++ trunk/subversion/libsvn_client/locking_commands.c Tue Jun 2
>> 10:17:27 2009 (r37912)
>> @@ -38,9 +38,10 @@
>> /* For use with store_locks_callback, below. */
>> struct lock_baton
>> {
>> - svn_wc_adm_access_t *adm_access;
>> + const char *base_path;
>> apr_hash_t *urls_to_paths;
>> svn_client_ctx_t *ctx;
>> + svn_wc_context_t *wc_ctx;
>> apr_pool_t *pool;
>> };
>>
>> @@ -49,7 +50,7 @@ struct lock_baton
>> * BATON is a 'struct lock_baton *', PATH is the path being locked,
>> * and LOCK is the lock itself.
>> *
>> - * If BATON->adm_access is not null, then this function either
>> stores
>> + * If BATON->base_path is not null, then this function either stores
>> * the LOCK on REL_URL or removes any lock tokens from REL_URL
>> * (depending on whether DO_LOCK is true or false respectively), but
>> * only if RA_ERR is null, or (in the unlock case) is something other
>> @@ -63,8 +64,6 @@ store_locks_callback(void *baton,
>> svn_error_t *ra_err, apr_pool_t *pool)
>> {
>> struct lock_baton *lb = baton;
>> - svn_wc_adm_access_t *adm_access;
>> - const char *abs_path;
>> svn_wc_notify_t *notify;
>>
>> /* Create the notify struct first, so we can tweak it below. */
>> @@ -80,25 +79,21 @@ store_locks_callback(void *baton,
>> notify->lock = lock;
>> notify->err = ra_err;
>>
>> - if (lb->adm_access)
>> + if (lb->base_path)
>> {
>> - const char *base_path = svn_wc_adm_access_path(lb-
>> >adm_access);
>> char *path = apr_hash_get(lb->urls_to_paths, rel_url,
>> APR_HASH_KEY_STRING);
>> - abs_path = svn_path_join(base_path, path, lb->pool);
>> -
>> - SVN_ERR(svn_wc_adm_probe_retrieve(&adm_access, lb->adm_access,
>> - abs_path, lb->pool));
>> + const char *abs_path = svn_path_join(lb->base_path, path, lb-
>> >pool);
>>
>> /* Notify a valid working copy path */
>> notify->path = abs_path;
>> - notify->path_prefix = base_path;
>> + notify->path_prefix = lb->base_path;
>>
>> if (do_lock)
>> {
>> if (!ra_err)
>> {
>> - SVN_ERR(svn_wc_add_lock(abs_path, lock, adm_access,
>> lb->pool));
>> + SVN_ERR(svn_wc_add_lock2(lb->wc_ctx, abs_path, lock,
>> lb->pool));
>
> This is incorrect. abs_path is NOT an absolute path. Total misnomer.
Rejiggered to make sure we get a true absolute path in r38754.
>> notify->lock_state = svn_wc_notify_lock_state_locked;
>> }
>> else
>> @@ -114,7 +109,7 @@ store_locks_callback(void *baton,
>> 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));
>> + SVN_ERR(svn_wc_remove_lock2(lb->wc_ctx, abs_path, lb-
>> >pool));
>
> Same problem.
>
>> notify->lock_state = svn_wc_notify_lock_state_unlocked;
>> }
>> else
>> @@ -425,14 +420,20 @@ svn_client_lock(const apr_array_header_t
>> adm_access, NULL, FALSE, FALSE, ctx, pool));
>>
>> cb.pool = pool;
>> - cb.adm_access = adm_access;
>> + if (adm_access)
>> + cb.base_path = svn_wc_adm_access_path(adm_access);
>
> You could use svn_wc__adm_access_abspath() here.
Nope, that's a libsvn_wc private API.
>> + else
>> + cb.base_path = NULL;
>> cb.urls_to_paths = urls_to_paths;
>> cb.ctx = ctx;
>> + SVN_ERR(svn_wc_context_create(&cb.wc_ctx, NULL /* config */,
>> pool, pool));
>>
>> /* Lock the paths. */
>> SVN_ERR(svn_ra_lock(ra_session, path_revs, comment,
>> steal_lock, store_locks_callback, &cb, pool));
>>
>> + SVN_ERR(svn_wc_context_destroy(cb.wc_ctx));
>> +
>> /* Unlock the wc. */
>> if (adm_access)
>> return svn_wc_adm_close2(adm_access, pool);
>> @@ -473,14 +474,20 @@ svn_client_unlock(const apr_array_header
>> SVN_ERR(fetch_tokens(ra_session, path_tokens, pool));
>>
>> cb.pool = pool;
>> - cb.adm_access = adm_access;
>> + if (adm_access)
>> + cb.base_path = svn_wc_adm_access_path(adm_access);
>
> And here.
Nope, not an exposed API.
>> ...
>> +++ trunk/subversion/libsvn_wc/adm_ops.c Tue Jun 2 10:17:27
>> 2009 (r37912)
>> @@ -3082,21 +3082,21 @@ svn_wc_resolved_conflict4(const char *pa
>> cancel_func, cancel_baton,
>> pool);
>> }
>>
>> -svn_error_t *svn_wc_add_lock(const char *path, const svn_lock_t
>> *lock,
>> - svn_wc_adm_access_t *adm_access,
>> apr_pool_t *pool)
>> +svn_error_t *
>> +svn_wc_add_lock2(svn_wc_context_t *wc_ctx,
>> + const char *local_abspath,
>> + const svn_lock_t *lock,
>> + apr_pool_t *scratch_pool)
>> {
>> - svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>> - const char *local_abspath;
>> svn_wc__db_lock_t db_lock;
>> svn_error_t *err;
>> const svn_string_t *needs_lock;
>>
>> - SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>
> For safety, and to catch bugs (like above, where abs_path is anything
> but), you could/should add an assertion here.
Done, r38754.
>> db_lock.token = lock->token;
>> db_lock.owner = lock->owner;
>> db_lock.comment = lock->comment;
>> db_lock.date = lock->creation_date;
>> - err = svn_wc__db_lock_add(db, local_abspath, &db_lock, pool);
>> + err = svn_wc__db_lock_add(wc_ctx->db, local_abspath, &db_lock,
>> scratch_pool);
>> if (err)
>> {
>> if (err->apr_err != SVN_ERR_WC_PATH_NOT_FOUND)
>> @@ -3106,29 +3106,29 @@ svn_error_t *svn_wc_add_lock(const char
>> svn_error_clear(err);
>> return svn_error_createf(SVN_ERR_ENTRY_NOT_FOUND, NULL,
>> _("'%s' is not under version
>> control"),
>> - svn_path_local_style(path, pool));
>> + svn_path_local_style(local_abspath,
>> + scratch_pool));
>> }
>
> You've rev'd the API, so this error remapping should move to
> deprecated.c.
I agree that we should move this, but I'm not quite ready to assess
the impact that it would have on callers to change the API in that
manner. So I'm going to file this in the post-API change sweep.
(Others are free to do it, of course.)
>> ...
>> {
>> - svn_wc__db_t *db = svn_wc__adm_get_db(adm_access);
>> - const char *local_abspath;
>> svn_error_t *err;
>> const svn_string_t *needs_lock;
>>
>> - SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>
> Another assertion.
>
>> - err = svn_wc__db_lock_remove(db, local_abspath, pool);
>> + err = svn_wc__db_lock_remove(wc_ctx->db, local_abspath,
>> scratch_pool);
>> if (err)
>> {
>> if (err->apr_err != SVN_ERR_WC_PATH_NOT_FOUND)
>
> Another remap to move.
>
>> ...
>> +++ trunk/subversion/libsvn_wc/deprecated.c Tue Jun 2 10:17:27
>> 2009 (r37912)
>> @@ -528,6 +528,39 @@ svn_wc_resolved_conflict3(const char *pa
>> cancel_baton, pool);
>> }
>>
>> +svn_error_t *
>> +svn_wc_add_lock(const char *path,
>> + const svn_lock_t *lock,
>> + svn_wc_adm_access_t *adm_access,
>> + apr_pool_t *pool)
>> +{
>> + const char *local_abspath;
>> + svn_wc_context_t *wc_ctx;
>> +
>> + SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>> + SVN_ERR(svn_wc__context_create_with_db(&wc_ctx, NULL /* config */,
>> +
>> svn_wc__adm_get_db(adm_access),
>> + pool));
>> +
>> + return svn_error_return(svn_wc_add_lock2(wc_ctx, local_abspath,
>> lock, pool));
>
> The context is not destroyed, which could mean dangling open-handles
> on SDBs.
Fixed in r38754.
>> +}
>> +
>> +svn_error_t *
>> +svn_wc_remove_lock(const char *path,
>> + svn_wc_adm_access_t *adm_access,
>> + apr_pool_t *pool)
>> +{
>> + const char *local_abspath;
>> + svn_wc_context_t *wc_ctx;
>> +
>> + SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>> + SVN_ERR(svn_wc__context_create_with_db(&wc_ctx, NULL /* config */,
>> +
>> svn_wc__adm_get_db(adm_access),
>> + pool));
>> +
>> + return svn_error_return(svn_wc_remove_lock2(wc_ctx,
>> local_abspath, pool));
>
> Likewise.
Also fixed.
Thanks for the review,
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383681
Received on 2009-08-14 19:25:21 CEST