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

Hi.

I found a typo in comment.
I resend a patch.

-- 
Masaru Tsuchiyama <m.tmatma_at_gmail.com>
> Hi.
> 
> Thank you for the review.
> I attach a fixed patch.
> 
> [[[
>    issue #4299: add sub command 'youngest' to svn
> 
>    * subversion/svn/cl.h
>      (svn_cl__opt_state_t): Add no_newline member.
>      (): Add svn_cl__youngest to the command procedure list.
>      
>    * subversion/svn/svn.c
>      (svn_cl__options): Add --new-line option.
>      (svn_cl__longopt_t): Add opt_no_newline to svn_cl__longopt_t.
>      (svn_cl__cmd_table): Add 'youngest' entry.
>      (sub_main): Add handler for opt_no_newline.
>    * subversion/svn/youngest-cmd.c
>      (svn_cl__youngest): implement the 'youngest' sub command handler.
>    ]]]
> 
> -- 
> Masaru Tsuchiyama <m.tmatma_at_gmail.com>
> 
> > 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 *);