Re: [PATCH] git-svn: don't escape tilde ('~') for http(s) URLs

From: Eric Wong <normalperson@yhbt.net>
Date: Oct 22, 2008, 08:16 UTC
Message-ID: <20081022081653.GC14966@untitled>
In-Reply-To: <7vtzb5wr6v.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Eric Wong <normalperson@yhbt.net> writes:
> 
> >> strace revealed that git-svn url-encodes ~ while svn does not do that.
> >>
> >> For svn we have:
> >> write(5, "<S:update-report send-all=\"true\" xmlns:S=\"svn:\">
> >> <S:src-path>https://sucs.org/~welshbyte/svn/backuptool/trunk</S:src-path>...
> >>
> >> While git-svn shows:
> >> write(7, "<S:update-report send-all=\"true\" xmlns:S=\"svn:\">
> >> <S:src-path>https://sucs.org/%7Ewelshbyte/svn/backuptool/trunk</S:src-path>...
> 
> This looks like an XML based request sequence to me (and svn is talking
> WebDAV here, right?); it makes me wonder what exact quoting rules are used
> there.  I would expect $path in <S:src-path>$path</S:src-path> to quote
> a letters in it e.g. '<' as "&lt;" --- which is quite different from what
> the s/// substitutions in the patch seem to be doing.

I agree. I haven't checked if the SVN libraries do proper XML escaping for us (but the problem hasn't shown up yet :). I was already completely disappointed that git-svn had to do its own escaping when transmitting data using the SVN libraries (and dependent on the protocol being used, too!).

Show 16 quoted lines

> > diff --git a/git-svn.perl b/git-svn.perl
> > index ef6d773..a97049a 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -852,7 +852,7 @@ sub escape_uri_only {
> > -		s/([^\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg;
> > +		s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg;
> 
> Admittedly I do not know git-svn (nor Perl svn bindings it uses), and I
> suspect that some of the XML-level escaping is done in the libsvn side,
> but it would be nice if somebody can at least verify that the code after
> the patch works with repositories with funny characters in pathnames
> (perhaps list all the printables including "<&>?*!@.+-%^").  Even nicer
> would be a log message that says "the resulting code covers all cases
> because it follows _that_ spec to escape _all_ problematic letters",
> pointing at some in svn (or libsvn-perl) resource.

Help with looking at what SVN does and writing testcases would definitely be appreciated on this matter. Or perhaps this can be done at GitTogether :)

Show 5 quoted lines

> The patch may make a path with '~' work, but it (neither in the patch text
> nor in the commit log message) does not have much to give readers enough
> confidence that the code after the patch is the _final_ one, as opposed to
> being just a band-aid for a single symptom that happened to have been
> discovered this time.

This is definitely a band-aid fix until I or somebody else takes the time to figure out:

 1. exactly which characters need to be escaped
 2. for which protocols those characters need to be escaped
 3. which part(s) of the URI they need to be escaped for
    (repository root vs SVN path)
 4. which versions of SVN needs more (or less) escaping rules

(I vote for somebody else, especially for #4 :)

-- 
Eric Wong