Philip Martin <philip.mar...@wandisco.com> writes:
> Philip Martin <philip.mar...@wandisco.com> writes:
>
>> hwri...@apache.org writes:
>>
>>> Author: hwright
>>> Date: Thu Nov 19 18:09:08 2009
>>> New Revision: 882232
>>
>>> --- subversion/trunk/subversion/libsvn_client/revert.c (original)
>>> +++ subversion/trunk/subversion/libsvn_client/revert.c Thu Nov 19 18:09:08
>>> 2009
>>> @@ -68,19 +68,13 @@
>>> svn_client_ctx_t *ctx,
>>> apr_pool_t *pool)
>>> {
>>> - svn_wc_adm_access_t *adm_access, *target_access;
>>> - const char *target, *local_abspath;
>>> + const char *local_abspath;
>>> svn_error_t *err;
>>> - int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
>>> -
>>> - SVN_ERR(svn_wc__adm_open_anchor_in_context(
>>> - &adm_access, &target_access, &target,
>>> - ctx->wc_ctx, path, TRUE, adm_lock_level,
>>> - ctx->cancel_func, ctx->cancel_baton,
>>> - pool));
>>>
>>> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>>>
>>> + SVN_ERR(svn_wc__acquire_write_lock(ctx->wc_ctx, local_abspath, pool));
>>> +
>>> err = svn_wc_revert4(ctx->wc_ctx,
>>> local_abspath,
>>> depth,
>>> @@ -108,7 +102,8 @@
>>> return svn_error_return(err);
>>> }
>>>
>>> - return svn_wc_adm_close2(adm_access, pool);
>>> + return svn_error_return(
>>> + svn_wc__release_write_lock(ctx->wc_ctx, local_abspath, pool));
>>> }
>>
>> Your change conflicts with my change shown below. The problem is
>> shown by depth_tests 33 which reverts a schedule add directory. Your
>> code above assumes after calling svn_wc_revert4 on such a directory it
>> is still possible to call svn_wc__release_write_lock. Is that valid
>> given that revert makes the whole directory vanish?
>
> So there might be a problem in my code, but I still don't understand
> your code. The problem is when local_abspath is a schedule add
> directory. Your code acquires a write lock on local_abspath and then
> runs revert, which removes local_abspath from revision control. Then
> you release the lock on local_abspath which is no longer a versioned
> directory.
I'm still stuck on this bit above. Your code only works because
r881265 means that we don't remove .svn directories when reverting
added directories. That allows you to release the lock because the
directory still exists; that's not going to work when the directory is
correctly removed.
How do we fix it? I suppose we could simply make it not an error to
release locks from directories that don't exist. That would have the
drawback that some errors such as releasing the wrong lock wouldn't
get caught. We could attempt to second-guess which directories get
removed and so restrict which lock releases we allow to fail. That
doesn't sound very good either, one bit of code is second-guessing
another. Also what happens if something creates a new directory with
a name matching the removed one in the gap between the directory
removal and the attempted lock release? We need to be very careful
not to release a lock we didn't acquire.
Another solution would be to track the directory removal and record
the "missing" lock until it gets released. We could have
svn_wc__db_temp_forget_directory leave an in-memory pdh structure that
exists until the lock is released. I think this is a better solution,
it also seems closer to the single DB per WC case where it will be
possible to hold onto the locks until released.
Do you agree there is a problem? Do any of these solutions look like
the right thing to do?
> Inside svn_wc__release_write_lock the first check on kind
> gets svn_wc__db_kind_unknown so the code then gets the parent and
> releases locks from there. The recently reverted directory is no
> longer a child of the parent, isn't locked and so doesn't get a lock
> released. In effect the release call is releasing a completely
> different set of locks to those acquired earlier in the function.
> That's not the right thing to do is it?
That bit was my mistake, I had a patch applied.
--
Philip
