On Friday 24 June 2005 03:09, Alexander Thomas wrote:> test script only after complete fix>> [[[> Partial fix for issue 2269 - warn and skip for nonexistent file> on 'svn rm' and exit with failure
Need a period at he end of the sentence.
> * subversion/include/svn_error_codes.h> Added new error code SVN_ERR_WC_NOT_NONEXISTENT_PATH.>> * subversion/libsvn_wc/adm_ops.c> (erase_unversioned_from_wc): Modified.>> * subversion/libsvn_client/delete.c> (svn_client_delete): Modified.> ]]]>> -Alexander Thomas (AT)> Index: subversion/include/svn_error_codes.h> ===================================================================> --- subversion/include/svn_error_codes.h (revision 15130)> +++ subversion/include/svn_error_codes.h (working copy)> @@ -379,6 +379,9 @@> SVN_ERR_WC_CATEGORY_START + 23,> "Invalid schedule") > > + SVN_ERRDEF (SVN_ERR_WC_NOT_NONEXISTENT_PATH,> + SVN_ERR_WC_CATEGORY_START + 24,> + "Path does not exist")
Need to add a comment above this saying "@since New in 1.3.", and put a blank line below this new definition... if it's really going to stay. I don't believe there is a reason to introduce a new error though.
> /* fs errors */> > SVN_ERRDEF (SVN_ERR_FS_GENERAL,> Index: subversion/libsvn_wc/adm_ops.c> ===================================================================> --- subversion/libsvn_wc/adm_ops.c (revision 15130)> +++ subversion/libsvn_wc/adm_ops.c (working copy)> @@ -654,7 +654,8 @@> switch (kind)> {> case svn_node_none:> - /* Nothing to do. */> + case svn_node_unknown:> + return svn_error_create (SVN_ERR_WC_NOT_NONEXISTENT_PATH, 0, NULL);
Is this really necessary? I don't believe it is. Perhaps look at svn_client__can_delete() and the find_undeletables() callback to come up with a different technique? This feels hackish.
> break;> > default:> Index: subversion/libsvn_client/delete.c> ===================================================================> --- subversion/libsvn_client/delete.c (revision 15130)> +++ subversion/libsvn_client/delete.c (working copy)> @@ -30,6 +30,7 @@> #include "svn_error.h"> #include "svn_path.h"> #include "client.h"> +#include "svn_cmdline.h"> > #include "svn_private_config.h"> > @@ -239,6 +240,9 @@> svn_client_ctx_t *ctx,> apr_pool_t *pool)> {> + svn_error_t *err;> + svn_boolean_t success = FALSE;
Get rid of the success variable (more on that below).
> +> if (! paths->nelts)> return SVN_NO_ERROR;> > @@ -268,15 +272,30 @@> SVN_ERR (svn_wc_adm_open3 (&adm_access, NULL, parent_path, > TRUE, 0, ctx->cancel_func,> ctx->cancel_baton, subpool));> - SVN_ERR (svn_client__wc_delete (path, adm_access, force, > - FALSE,> - ctx->notify_func2,> - ctx->notify_baton2,> - ctx, subpool));> + err = svn_client__wc_delete (path, adm_access, force,> + FALSE,> + ctx->notify_func2,> + ctx->notify
_baton2,> + ctx, subpool);> + if (err)> + {> + if (err->apr_err == SVN_ERR_WC_NOT_NONEXISTENT_PATH)> + svn_cmdline_printf (> + pool, "Skipped, '%s' is not under version control\n",> + svn_path_local_style (path, pool));
We cannot assume that we're being called from the command line client here. You need use ctx->notify_func2(). You'll need to setup the svn_wc_notify_t structure with the action field set svn_wc_notify_skip to get what you want. Use svn_wc_create_notify() to allocate and initialize the structure. You also have a memory leak. You need to call svn_error_clear() on the error.
> + else> + SVN_ERR(err);
No need to use the SVN_ERR() macro here. Just put 'return err;'.
> + }> + else> + success = TRUE;> +
There's no need for the above else statement.
> SVN_ERR (svn_wc_adm_close (adm_access));> }> svn_pool_destroy (subpool);> }> > - return SVN_NO_ERROR;> + if (success)> + return SVN_NO_ERROR;> + else> + return err;
Get rid of this whole 'success' thing. Instead, after skipping the path, call svn_error_clear() to release any memory associated with the error. And keep the original 'return SVN_NO_ERROR;' statement. It's reasonable to return SVN_NO_ERROR even if we skipped a target (and looking at the email referenced in the issue, that was what Julian Foad thought as well).
Besides, you could potentially lose the error anyways. If you had another target in the array that did exist and was versioned, but it came after that nonexistant path, success would get clobbered with TRUE, and you'd fail to return the error.
> }
Unfortunately, I have to get to work, or I'd help find a way to get an error back, other than to create an entirely new one. I find it hard to believe that we don't have something to say a path is non-existant though.
HTH.
-John
Received on Fri Jul 1 12:18:37 2005