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

Daniel Näslund <daniel_at_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? 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. 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.

>
> - 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?

> 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);

-- 
Philip