Re: [PATCH] Replace some adm_access batons in externals.c

Daniel Näslund <daniel_at_longitudo.com> writes:

> On Thu, Dec 17, 2009 at 06:10:18PM +0000, Philip Martin wrote:
>> Daniel Näslund <daniel_at_longitudo.com> writes:
>> >
>> > + SVN_ERR(svn_wc__release_write_lock(ib->ctx->wc_ctx,
>> > + remove_target_abspath,
>> > + ib->iter_pool));
>> > +
>>
>> Why the change in behaviour? The old code would leave the locks if
>> the remove call failed. Is this because you are no longer locking the
>> external?
>
> A comment in svn_wc_acquire_write_locks():
>
> [[[
> /* The current lock paradigm is that each directory holds a lock for itself,
> and there are no inherited locks. In the eventual wc-ng paradigm, a
> lock on a directory, would imply a infinite-depth lock on the children.
> But since we aren't quite there yet, we do the infinite locking
> manually (and be sure to release them in svn_wc__release_write_lock(). */
> ]]]
>
> Why leave locks we have created if the operation we attempted to do
> failed?

That's how locks work.

 - take the lock
 - make changes possibly leaving the wc inconsistent
 - complete changes making the wc consistent
 - remove lock

If an error occurs that aborts the operation while the wc is
inconsistent then the lock should not be removed.

The original code was ignoring LEFT_LOCAL_MOD errors, treating them as
not-an-error, so it removed locks in that case. For other errors it
left the locks.

The other problem with your new code is that if the
svn_wc__release_write_lock call returns an error any error from the
svn_wc_remove_from_revision_control2 call will leak.

-- 
Philip