Thanks Stefan; I already spotted these and tweaked it before commit.
- Julian
On Thu, 2010-11-18 at 13:00 +0100, Stefan Sperling wrote:
> 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:05:27 CET