Stefan Sperling <stsp_at_elego.de> writes:
> 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,
>
In several parts of the code base I have seen similar style, that is the
reason why I used that. I will keep in mind that what you mentioned is
the preferred style.
Thanks and Regards
Noorul
Received on 2010-11-18 13:21:31 CET