On Fri, Dec 18, 2009 at 10:04:37AM +0000, Philip Martin wrote:
> Daniel Näslund <[email protected]> writes:
>
> > On Thu, Dec 17, 2009 at 06:10:18PM +0000, Philip Martin wrote:
> >> Daniel Näslund <[email protected]> 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
Index: subversion/libsvn_client/externals.c
===================================================================
--- subversion/libsvn_client/externals.c (revision 892468)
+++ subversion/libsvn_client/externals.c (arbetskopia)
@@ -894,51 +894,22 @@
just be renamed to a new one. */
svn_error_t *err;
- svn_wc_adm_access_t *adm_access;
- svn_boolean_t close_access_baton_when_done;
-
const char *remove_target_abspath;
+ svn_boolean_t lock_existed;
- /* Determine if a directory or file external is being removed.
- Try to handle the case when the user deletes the external by
- hand or it is missing. First try to open the directory for a
- directory external. If that doesn't work, get the access
- baton for the parent directory and remove the entry for the
- external. */
- err = svn_wc__adm_open_in_context(&adm_access, ib->ctx->wc_ctx, path,
- TRUE, -1, ib->ctx->cancel_func,
- ib->ctx->cancel_baton, ib->iter_pool);
- if (err)
- {
- const char *anchor;
- const char *target;
- svn_error_t *err2;
+ SVN_ERR(svn_dirent_get_absolute(&remove_target_abspath,
+ path,
+ ib->iter_pool));
- SVN_ERR(svn_wc_get_actual_target2(&anchor, &target, ib->ctx->wc_ctx,
- path, ib->iter_pool,
- ib->iter_pool));
+ SVN_ERR(svn_wc_locked2(&lock_existed, NULL, ib->ctx->wc_ctx,
+ remove_target_abspath, ib->iter_pool));
- err2 = svn_wc_adm_retrieve(&adm_access, ib->adm_access, anchor,
- ib->iter_pool);
- if (err2)
- {
- svn_error_clear(err2);
- return svn_error_return(err);
- }
- else
- {
- svn_error_clear(err);
- }
- close_access_baton_when_done = FALSE;
- SVN_ERR(svn_dirent_get_absolute(&remove_target_abspath, target,
- ib->iter_pool));
- }
- else
+ if (! lock_existed)
{
- close_access_baton_when_done = TRUE;
- SVN_ERR(svn_dirent_get_absolute(&remove_target_abspath,
- svn_wc_adm_access_path(adm_access),
- ib->iter_pool));
+ SVN_ERR(svn_wc__acquire_write_lock(NULL, ib->ctx->wc_ctx,
+ remove_target_abspath,
+ ib->iter_pool,
+ ib->iter_pool));
}
/* We don't use relegate_dir_external() here, because we know that
@@ -965,13 +936,15 @@
}
/* ### Ugly. Unlock only if not going to return an error. Revisit */
- if (close_access_baton_when_done &&
- (!err || err->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD))
+ if (! lock_existed
+ && (! err || err->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD))
{
- svn_error_t *err2 = svn_wc_adm_close2(adm_access, ib->iter_pool);
+ svn_error_t *err2 = svn_wc__release_write_lock(ib->ctx->wc_ctx,
+ remove_target_abspath,
+ ib->iter_pool);
if (err2)
{
- if (!err)
+ if (! err)
err = err2;
else
svn_error_clear(err2);
