Re: git-svn SVN 1.7 fix, take 2
- From
Michael G Schwern <schwern@pobox.com>
- Date
- Jul 25, 2012, 01:00 UTC
- Message-ID
- <500F4530.7030207@pobox.com>
- In-Reply-To
- <7va9yoenb2.fsf@alter.siamese.dyndns.org>
On 2012.7.24 4:45 PM, Junio C Hamano wrote:
Show 16 quoted lines
>>> git log -p schwern/git-svn/extract-classes..schwern/git-svn/fix-canonical
>>>
>>> That should give you the information you need...
>>
>> I guess so. May we have your sign-off on these changes? (A simple
>> reply of "yes" is enough, no need to resend patches to do this.)
>>
>> Here it is in patch form for reviewers. If I understand correctly,
>> the idea is to replace accesses to $gs->{path} with calls to a
>> $gs->path function that canonicalizes (and likewise for s/path/url/).
>>
>> There are probably other subtleties, but that seems to be the gist.
>
> The impression I am getting is that the updated code wants to handle
> URL and paths without any funny encoding, but it is unclear from my
> cursory read (e.g. what goes on with escape_url?).No, now it's just canonicalizing as early as possible. Preferably within the object accessor rather than at the point of use. So in the code below, $full_url is already escaped/canonicalized.
In general this blob patch isn't going to make a lot of overall sense. I'm working with Jonathan to get it submitted in manageable pieces.
Show 6 quoted lines
>> if ($old_url =~ m#^svn(\+ssh)?://# ||
>> ($full_url =~ m#^https?://# &&
>> - escape_url($full_url) ne $full_url)) {
>> + $full_url ne $full_url)) {
>
> How can the latter part of this conditional be true?Good point. More importantly, what was it trying to accomplish before and does it need to be preserved?
"If the URL is svn OR its http and needs to be escaped... do something special". I don't really understand what the special stuff in the following block is. Anything that undef's the invocant (ie. $self) is probably broken.
a51cdb0c0420ee3bef26bbd1a9aa75e1d464e5b7 and 2a679c7a3148978a3f58f1c12100383638e744c5 shed some light. 2a679 looks like it specifically holds off on escaping $full_url. It would be very nice if that was not necessary. It would be helpful if the bug mentioned in 2a679 could be reproduced to see if it still applies or can be dealt with in another way.
--
185. My name is not a killing word.
-- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army
http://skippyslist.com/list/