On Fri, May 23, 2008 at 5:40 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> Troy Curtis Jr wrote:
>>
>> On Thu, May 22, 2008 at 6:07 AM, Julian Foad <julianfoad_at_btopenworld.com>
>> wrote:
>
> [...]
>>>
>>> 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.
>>
>> 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 don't quite follow what you mean by this.
>
Sorry I was a bit terse wasn't I. When I set out to fix the peg
revision support for relative urls I realized I would need to
duplicate the peg revision code in the relative url resolving code
block that occurred directly after the main target processing loop.
My initial thought was something like I will be implementing now.
Simply encapsulating the peg revision splitting code.
But I thought that I could instead teach the *arg_canonicalize*()
functions (using a common arg splitting function) and so I would get
the peg revision support inside the relative url parsing block "for
free". Of course needing to check for the adm directory made my
solution a bit less optimal.
I decided to stick with it because at least there I was just doing the
extra rev parsing in one place. With the way I am going now (similar
to what I was initially thinking) I will have to split and concatenate
in two places. I felt that at that point they were basically equal,
and I had already implemented most of the patch that I subsequently
submitted.
Is that a better explanation of my thoughts? I just sort of banged
out that other email and clicked send.
Troy
>> 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.
>
> - Julian
>
--
"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-24 04:14:30 CEST