Re: [PATCH] git-svn: don't escape tilde ('~') for http(s) URLs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 21, 2008, 21:53 UTC
- Message-ID
- <7vtzb5wr6v.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20081021211131.GA21606@yp-box.dyndns.org>
Eric Wong <normalperson@yhbt.net> writes:
Show 9 quoted lines
>> 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 "<" --- which is quite different from what the s/// substitutions in the patch seem to be doing.
Show 7 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.
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.