Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]

On 07/20/2013 02:42 AM, Masaru Tsuchiyama wrote:
> Hi.
>
> I implement 'youngest' sub command to svn, and attach a patch.
>

Masaru,

Thanks for the patch. I didn't do a full review of it, but it looks as
if you're taking the correct approach. I did quickly notice some things
that could be improved:

* The patch introduces tab characters. Our project guidelines insist
that only space characters be used for code indentation.

* "SVN_ERR( svn..."
           ^^^ No space here. Should be "SVN_ERR(svn..."

* I like the introduction of --no-newline, but the project is stingy
with short options, reserving them for operations believed to be
extremely common. I'm not convinced that 'svn youngest --no-newline' is
such an operation, so I cannot approve the allocation of "-n" for that.

* Finally, you should ensure that the user passed exactly 0 or 1 targets
for the operation before trying to access members of the 'targets' array:

   /* We want exactly 0 or 1 targets for this subcommand. */
   if (targets->nelts > 1)
     return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);

   /* Parse the first target into path-or-url. */
   target_path = APR_ARRAY_IDX(targets, 0, const char *);
Received on 2013-07-22 14:28:55 CEST