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 <[email protected]>
> 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 *);
svn-youngest.07.22.patch
Description: Binary data
