You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Masaru Tsuchiyama <m....@gmail.com> on 2013/07/20 02:37:07 UTC
[PATCH] add sub command 'youngest' to svnrdump
Hi.
I attach a patch to add sub command 'youngest' to svnrdump.
[[[
add sub command 'youngest' to svnrdump
* subversion/svnrdump/svnrdump.c
(svnrdump__cmd_table): Add an entry for youngest sub command.
(main): Print youngest revision and exit when setting youngest option.
]]]
--
Masaru Tsuchiyama <m....@gmail.com>
Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hi.
This is the log message for the 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__cmd_table): Add 'youngest' entry.
(sub_main): Add 'n' option.
* subversion/svn/youngest-cmd.c
(svn_cl__youngest): implement the 'youngest' sub command handler.
]]]
--
Masaru Tsuchiyama <m....@gmail.com>
> Hi.
>
> I implement 'youngest' sub command to svn, and attach a patch.
>
> --
> Masaru Tsuchiyama <m....@gmail.com>
>
>
> > On 07/19/2013 08:37 PM, Masaru Tsuchiyama wrote:
> > > Hi.
> > >
> > > I attach a patch to add sub command 'youngest' to svnrdump.
> > >
> > > [[[
> > > add sub command 'youngest' to svnrdump
> > >
> > > * subversion/svnrdump/svnrdump.c
> > > (svnrdump__cmd_table): Add an entry for youngest sub command.
> > > (main): Print youngest revision and exit when setting youngest option.
> > > ]]]
> > >
> > >
> >
> > Note that this patch could be the resolution for issue #4299 (http://subversion.tigris.org/issues/show_bug.cgi?id=4299). But that simply begs the question of whether the functionality would have been better implemented in 'svn' itself where it could enjoy the dual flavors of targets found therein: URL or WC-PATH.
Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hi.
I found a typo in comment.
I resend a patch.
--
Masaru Tsuchiyama <m....@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....@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 *);
Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]
Posted by Masaru Tsuchiyama <m....@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....@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 *);
Re: [PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add
sub command 'youngest' to svnrdump]
Posted by "C. Michael Pilato" <cm...@collab.net>.
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 *);
[PATCH] add sub command 'youngest' to svn [Was: Re: [PATCH] add sub command 'youngest' to svnrdump]
Posted by Masaru Tsuchiyama <m....@gmail.com>.
Hi.
I implement 'youngest' sub command to svn, and attach a patch.
--
Masaru Tsuchiyama <m....@gmail.com>
> On 07/19/2013 08:37 PM, Masaru Tsuchiyama wrote:
> > Hi.
> >
> > I attach a patch to add sub command 'youngest' to svnrdump.
> >
> > [[[
> > add sub command 'youngest' to svnrdump
> >
> > * subversion/svnrdump/svnrdump.c
> > (svnrdump__cmd_table): Add an entry for youngest sub command.
> > (main): Print youngest revision and exit when setting youngest option.
> > ]]]
> >
> >
>
> Note that this patch could be the resolution for issue #4299 (http://subversion.tigris.org/issues/show_bug.cgi?id=4299). But that simply begs the question of whether the functionality would have been better implemented in 'svn' itself where it could enjoy the dual flavors of targets found therein: URL or WC-PATH.
Re: [PATCH] add sub command 'youngest' to svnrdump
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/19/2013 08:37 PM, Masaru Tsuchiyama wrote:
> Hi.
>
> I attach a patch to add sub command 'youngest' to svnrdump.
>
> [[[
> add sub command 'youngest' to svnrdump
>
> * subversion/svnrdump/svnrdump.c
> (svnrdump__cmd_table): Add an entry for youngest sub command.
> (main): Print youngest revision and exit when setting youngest option.
> ]]]
>
>
Note that this patch could be the resolution for issue #4299
(http://subversion.tigris.org/issues/show_bug.cgi?id=4299). But that
simply begs the question of whether the functionality would have been
better implemented in 'svn' itself where it could enjoy the dual flavors
of targets found therein: URL or WC-PATH.