[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Remove redundant svn_error_return() wrapper

From: Stefan Sperling <stsp_at_elego.de>
Date: Thu, 18 Nov 2010 13:00:46 +0100

Hi Noorul,

two small remarks below:

On Thu, Nov 18, 2010 at 04:51:50PM +0530, Noorul Islam K M wrote:
> Index: subversion/svn/log-cmd.c
> ===================================================================
> --- subversion/svn/log-cmd.c (revision 1036324)
> +++ subversion/svn/log-cmd.c (working copy)
> @@ -647,12 +647,12 @@
> target = APR_ARRAY_IDX(targets, i, const char *);
>
> if (svn_path_is_url(target) || target[0] == '/')
> - return svn_error_return(svn_error_createf(
> - SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("Only relative paths can be specified"
> - " after a URL for 'svn log', "
> - "but '%s' is not a relative path"),
> - target));
> + return svn_error_createf(
> + SVN_ERR_CL_ARG_PARSING_ERROR, NULL,

The above two lines could be one line, like this:

 + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,

> + _("Only relative paths can be specified"
> + " after a URL for 'svn log', "
> + "but '%s' is not a relative path"),
> + target);
> }
> }
>

> Index: subversion/svn/relocate-cmd.c
> ===================================================================
> --- subversion/svn/relocate-cmd.c (revision 1036324)
> +++ subversion/svn/relocate-cmd.c (working copy)
> @@ -97,8 +97,20 @@
> apr_pool_t *subpool = svn_pool_create(scratch_pool);
> int i;

This part of the patch doesn't seem to be related to the rest.
It's not eliminating redundant svn_error_return() calls:

>
> + /* Target working copy root dir must be local. */
> for (i = 2; i < targets->nelts; i++)
> {
> + path = APR_ARRAY_IDX(targets, i, const char *);
> + if (svn_path_is_url(path))
> + return svn_error_return
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,

Also, please don't put any whitespace (like spaces or newlines) between
a function name and the opening brace.

This would be the preferred style:

+ return svn_error_return(
+ svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,

Thanks,
Stefan

> + NULL,
> + _("'%s' is not a local path"),
> + path));
> + }
> +
> + for (i = 2; i < targets->nelts; i++)
> + {
> svn_pool_clear(subpool);
> path = APR_ARRAY_IDX(targets, i, const char *);
> SVN_ERR(svn_client_relocate2(path, from, to, ignore_externals,
Received on 2010-11-18 13:01:30 CET

This is an archived mail posted to the Subversion Dev mailing list.