Troy Curtis Jr wrote on Sun, 4 May 2008 at 15:36 -0500:
> On Thu, May 1, 2008 at 8:30 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> > Troy Curtis Jr wrote on Thu, 1 May 2008 at 06:54 -0500:
> > > On Thu, May 1, 2008 at 3:02 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> > > > Troy Curtis Jr wrote on Wed, 30 Apr 2008 at 22:28 -0500:
> > > > > Add peg revision support to svn_opt__arg_canonicalize_url() and
> > > > > svn_opt__arg_canonicalize_path(). These functions are meant to process
> > > > > user-input and so should handle the peg revision case.
> > > > >
> > > > > * subversion/libsvn_subr/opt.c
> > > > > (split_arg_at_peg_revision): New function.
> > > >
> > > > How is it different from svn_opt_parse_path ?
> > > >
> > >
> > > split_arg_at_peg_revision() deals strictly with text, whereas
> > > svn_opt_parse_path() puts the peg revision in to a revision struct.
> >
> > In other words, the former returns the peg as string, and the latter as
> > an svn_opt_revision_t.
> >
> > In this case, it seems to me that you use split_arg_at_peg_revision() as
> > a helper function in svn_opt_parse_path(), instead of embedding the
> > knowledge of peg-rev syntax in both of them.
> >
>
> Really split_arg_at_peg_revision() was created to prevent
> duplication in the svn_opt__arg_canonicalize_*() functions. It seems to me
> that this patch has a specific purpose of teaching
> svn_opt__arg_canonicalize_*() about peg revision syntax. Perhaps a follow on
> patch could also make use of split_arg_at_peg_revision() in
> svn_opt_parse_path(), but I don't think it really 'fits' this patch.
>
Logically, you extract the existing functionality and repack it in
reusable form before you reuse it in a new place, so the "follow on"
should actually come first. :)
It could be one patch, even, if breaking out the helper isn't noisy and
doesn't distract or drown the main part of the patch (peg support).
> > Looking deeper, I see now that svn_opt_args_to_target_array3() returns
> > an array of const char *, and subcommands expect to get an array of
> > targets with peg-revs appended, which -- eventually -- is why you have
> > to fold everything back to strings. It would be nicer to use
> > svn_opt_parse_path and teach subcommands to expect structs, of course,
> > but this can be done later.
>
> Yeah this was discussed when I was originally moving the arg processing into
> libsvn_client. We talked about returning an array of structure with
> several bits
> of useful information. I actually started down the path to figure out how to do
> it, but the expectation of an array of const char * was extremely deeply
> embedded.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-04 23:43:32 CEST