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

[Junio C Hamano <gitster@pobox.com>]

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.

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

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.