On Thu, May 22, 2008 at 6:07 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Troy,
>
> I saw that others were reviewing this for you, so I didn't join in. Now
> there's been a pause, so I took a quick look at this. My thoughts are:
>
> The pointer arithmetic was perfectly fine as it was! Might be simpler to use
> strrchr("/") followed by strrchr("@") though.
>
> From the point of view of parsing command-line syntax, I think removing the
> peg revision specifier comes logically before canonicalising the rest of the
> argument. Therefore, may I recommend not making the
> svn_opt__arg_canonicalize_path()/_url() functions "cope with" a peg
> revision, but leave them as they were, to operate on just a path or URL.
> Instead, make your split_arg_at_peg_revision() function semi-public (maybe
> called "svn_opt__arg_split_at_peg") and use it where previously the
> *_args_to_target_array*() and other functions did their own parsing.
>
> If your split_arg_at_peg_revision() will return the peg string with the "@"
> still on it, then it could represent "no peg" with either NULL or the empty
> string. I suggest it should never return NULL but rather the empty string
> when there is no peg, so that callers can simply re-concatenate the peg
> without checking for NULL. This seems a reasonable way to go for now, given
> that (as you point out) some client functions still want to add the peg back
> on and then take it off again later. In the long term, the clean way to
> organise it would be for this initial splitting to pass the peg back
> separately (without its "@") and for the path and the peg to be passed
> around separately thereafter. But there's no need to go into that now.
>
> That seems to make it all simpler. Does that make sense and is there
> anything else I could help with? It would be great to see an update when you
> have time. Thanks.
>
> - Julian
>
>
That would certainly work, I was just trying to keep from having to
put another implementation in the relative url parsing block of the
code. I think that is an excellent compromise between my patch and
Karl's suggestions. I'll work up the patch and send it in when I get
a chance.
Troy
--
"Beware of spyware. If you can, use the Firefox browser." - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
---------------------------------------------------------------------
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-23 05:13:13 CEST