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

Junio C Hamano [off-list ref] wrote:

Eric Wong [off-list ref] writes:

quoted

quoted

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!).

quoted

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 :)

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