On Thu, Dec 17, 2009 at 06:10:18PM +0000, Philip Martin wrote:
> Daniel Näslund <dan...@longitudo.com> writes:
>
> > Index: subversion/libsvn_client/externals.c
> > ===================================================================
> > --- subversion/libsvn_client/externals.c (revision 891778)
> > +++ subversion/libsvn_client/externals.c (arbetskopia)
> > @@ -894,53 +894,17 @@
> > 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;
> >
> > - /* 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__acquire_write_lock(NULL, ib->ctx->wc_ctx,
> > + remove_target_abspath,
> > + ib->iter_pool,
> > + ib->iter_pool));
>
> Does this lock an external directory?
Is externals handled differently? svn_wc__db_kind_t has no field for
externals. I assumed that those files and dirs were treated like any
other file or dir.
> I think the 1.6 code would lock such a directory before deleting it
> and that the code would fail if it was not locked.
svn_wc_acquire_write_lock() calls svn_wc__db_wclock_set() which returns
SVN_ERR_WC_LOCKED if the wc is locked.
> The current wc-ng code doesn't check for locks in quite the same way
> and this allows code to make changes without locks, but I'm not sure
> that's the correct thing to do.
Is code allowed to make changes without locks? That sounds very strange to
me. A transition phase before the API ha seattled? Do you have any
examples? I think I remember a thread with you and hwright talking about
that. Will look it up later.
>
> >
> > - 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
> > - {
> > - 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));
> > - }
> > -
> > /* We don't use relegate_dir_external() here, because we know that
> > nothing else in this externals description (at least) is
> > going to need this directory, and therefore it's better to
> > @@ -950,6 +914,10 @@
> > ib->ctx->cancel_func, ib->ctx->cancel_baton,
> > ib->iter_pool);
> >
> > + 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?
>
> > if (ib->ctx->notify_func2)
> > {
> > svn_wc_notify_t *notify =
> > @@ -964,20 +932,6 @@
> > notify, ib->iter_pool);
> > }
> >
> > - /* ### 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))
> > - {
> > - svn_error_t *err2 = svn_wc_adm_close2(adm_access, ib->iter_pool);
> > - if (err2)
> > - {
> > - if (!err)
> > - err = err2;
> > - else
> > - svn_error_clear(err2);
> > - }
> > - }
> > -
> > if (err && (err->apr_err != SVN_ERR_WC_LEFT_LOCAL_MOD))
> > return svn_error_return(err);
> > svn_error_clear(err);
Thanks for your feedback!
Daniel
