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

On Fri, Dec 18, 2009 at 10:04:37AM +0000, Philip Martin wrote:
> 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.

I didn't know that. I've learned something new. A good thing for me! :-)
 
> 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.

I did that since I thought the locks weren't neccessary once the
operation had failed.
 
> 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.

Fixed.

I've used svn_wc_locked2() for checking if there already exists a
write_lock.

make check passed!

[[[
Use recursive locking when deleting an external. No need to make a
distinction between files and dirs as the previous code did.

* subversion/libsvn_client/externals.c
  (handle_external_item_change): Remove the adm_access functions and use
    svn_wc_{acqurire,release}write_lock instead and svn_wc_locked()
    instead.

Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
Review by: philip
]]]

/Daniel