On 29 September 2015 at 15:16,  <rhuij...@apache.org> wrote:
> Author: rhuijben
> Date: Tue Sep 29 12:16:04 2015
> New Revision: 1705843
>
> URL: http://svn.apache.org/viewvc?rev=1705843&view=rev
> Log:
> Remove registration of external in the EXTERNALS table when we find a
> versioned node that takes its place during update.
>

> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/externals.c (original)
> +++ subversion/trunk/subversion/libsvn_client/externals.c Tue Sep 29 12:16:04 
> 2015
> @@ -231,6 +231,40 @@ switch_dir_external(const char *local_ab
>
>        if (node_url)
>          {
> +          svn_boolean_t is_wcroot;
> +
> +          SVN_ERR(svn_wc__is_wcroot(&is_wcroot, ctx->wc_ctx, local_abspath,
> +                                    pool));
> +
> +          if (! is_wcroot)
> +          {
> +            /* This can't be a directory external! */
> +
> +            err = svn_wc__external_remove(ctx->wc_ctx, defining_abspath,
> +                                          local_abspath,
> +                                          TRUE /* declaration_only */,
> +                                          ctx->cancel_func, 
> ctx->cancel_baton,
> +                                          pool);
> +
> +            if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> +              {
> +                /* New external... No problem that we can't remove it */
> +                svn_error_clear(err);
> +                err = NULL;
> +              }
> +
> +            return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, err,
Hi Bert,

I'm not sure that passing error from svn_wc__external_remove() to
error chain is the good idea. Why do not return it in the same way
like we handle error from svn_wc__is_wcroot()?

I mean code like this:
[[
if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
  {
    /* New external... No problem that we can't remove it */
    svn_error_clear(err);
  }
else if (err)
  return svn_error_trace(err);

return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, NULL,
...
]]]
See attached patch.

Otherwise error message could be confusing, in some cases. For example
if svn_wc__external_remove() return SVN_ERR_CANCELED error message
will be something like:
[[[
The external '%s' defined in %s at '%s' cannot be checked out because
'%s' is already a versioned path.
The operation was interrupted
]]]

-- 
Ivan Zhakov
Index: subversion/libsvn_client/externals.c
===================================================================
--- subversion/libsvn_client/externals.c        (revision 1710064)
+++ subversion/libsvn_client/externals.c        (working copy)
@@ -250,10 +250,11 @@
               {
                 /* New external... No problem that we can't remove it */
                 svn_error_clear(err);
-                err = NULL;
               }
+            else if (err)
+              return svn_error_trace(err);
 
-            return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, err,
+            return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, NULL,
                                      _("The external '%s' defined in %s at 
'%s' "
                                        "cannot be checked out because '%s' is "
                                        "already a versioned path."),

Reply via email to