Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]
From: Masaru Tsuchiyama <m.tmatma_at_gmail.com>
Date: Mon, 22 Jul 2013 22:23:42 +0900
I found a typo in comment.
-- 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 *);
This is an archived mail posted to the Subversion Dev mailing list.