(unknown),

(unknown),

[Ben Walton]

Hi Eric,

When running the test suite against subversion 1.7 on solaris, t9134
was failing with an assertion failure in the SVN bindings and a
subsequent core dump from perl.

The behaviour of 1.7 has seemingly changed such that even file paths
must be eascaped/canonicalized now.  The core dump behaviour is
something that still needs to be investigated with the subversion
folks.  A colleague and I will push that on their side.

See the initial discussion of this issue on the svn list here:
http://article.gmane.org/gmane.comp.version-control.subversion.devel/132227

With the following patch, the test suite once again passes.  I don't
know how it will behave against an older client though so maybe a
conditional check for the version of the bindings is still required?

Thanks
-Ben

From: Ben Walton <bwalton@artsci.utoronto.ca>
Subject: 
In-Reply-To: 

[PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Ben Walton]

Previously only http/https URL's were uri escaped.  When building
against subversion 1.7, this was causing a segfault in perl after
an assertion failure in the SVN::Ra bindings during in t9134.

Changing 'trash directory' to 'trash_directory' worked around the
problem.

After a colleague reported this problem to the subversion list, it
was determined that the problem is in git, not svn.[1]  The SVN code
expects URL's and paths to be pre-escaped.

We now also escape file:// URL's in the Git::SVN::Ra->escape_url
code path.

[1] http://news.gmane.org/gmane.comp.version-control.subversion.devel

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 git-svn.perl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 351e743..b00c188 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5366,6 +5366,9 @@ sub escape_url {
 	if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
 		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
 		$url = "$scheme://$domain$uri";
+	} elsif ($url =~ m#^(file)://(.*)$#) {
+		my ($scheme, $uri) = ($1, escape_uri_only($2));
+		$url = "$scheme://$uri";
 	}
 	$url;
 }
-- 
1.7.7.1

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Jonathan Nieder]

Hi,

Ben Walton wrote:

> After a colleague reported this problem to the subversion list, it
> was determined that the problem is in git, not svn.[1]  The SVN code
> expects URL's and paths to be pre-escaped.

Thanks for your work on this!  I'm not really sure how one can decide
that the problem is not in svn --- some existing functions changed ABI
in such a way as to break existing applications and require code
changes and a recompile.  It would be better for Subversion to
silently fix up paths provided by bad callers, or at least to return a
sensible error code.

So the problem is that nobody who cared was testing prereleases of
subversion and reporting bugs early enough for it to get this fixed
before the 1.7 release.  But yes, that's water under the bridge and
git-svn (and libsvn-perl, and pysvn, and ...) should just adjust to
the new world order.

> [1] http://news.gmane.org/gmane.comp.version-control.subversion.devel

Do you mean
http://thread.gmane.org/gmane.comp.version-control.subversion.devel/132250
?

> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
>  git-svn.perl |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Sounds sensible in principle, though I haven't checked the patch in
detail.  When I run "make test" with svn 1.7 with this patch applied,
I get the following result, unfortunately:

| expecting success: 
|         git svn clone -s "$svnrepo" g &&
|         (
|                 cd g &&
|                 test x`git rev-parse --verify refs/remotes/trunk^0` = \
|                      x`git rev-parse --verify refs/heads/master^0`
|         )
|
| Initialized empty Git repository in /home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/g/.git/
| Using higher level of URL: file:///home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/svnrepo => file:///home/jrn/src/git2/t/trash%20directory.t9145-git-svn-master-branch/svnrepo
| svn-remote.svn: remote ref '///home/jrn/src/git2/t/trash directory.t9145-git-svn-master-branch/svnrepo/trunk:refs/remotes/trunk' must start with 'refs/'
|
| not ok - 2 git svn clone --stdlayout sets up trunk as master
| #
| #               git svn clone -s "$svnrepo" g &&
| #               (
| #                       cd g &&
| #                       test x`git rev-parse --verify refs/remotes/trunk^0` = \
| #                            x`git rev-parse --verify refs/heads/master^0`
| #               )
| #
|
| # failed 1 among 2 test(s)
| 1..2
| make[3]: *** [t9145-git-svn-master-branch.sh] Error 1

Does it work for you?  This is with a merge of git 1.7.8-rc0 and 1.7.7.2.

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Ben Walton]

Excerpts from Jonathan Nieder's message of Wed Nov 02 14:20:38 -0400 2011:

> Thanks for your work on this!  I'm not really sure how one can
> decide that the problem is not in svn --- some existing functions
> changed ABI in such a way as to break existing applications and
> require code changes and a recompile.  It would be better for
> Subversion to silently fix up paths provided by bad callers, or at
> least to return a sensible error code.

Yes, my apologies.  I wrote this commit message before fully realizing
just how wrong the response on the svn list was.  It core dumps, after
all...If the patch is useful, I'll resubmit with a better commit
message.

> So the problem is that nobody who cared was testing prereleases of
> subversion and reporting bugs early enough for it to get this fixed
> before the 1.7 release.  But yes, that's water under the bridge and
> git-svn (and libsvn-perl, and pysvn, and ...) should just adjust to
> the new world order.
> 
> > [1] http://news.gmane.org/gmane.comp.version-control.subversion.devel
> 
> Do you mean
> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/132250
> ?

No, the link should have been:
http://article.gmane.org/gmane.comp.version-control.subversion.devel/132227

I'm not sure how it got mangled like that.

> | # failed 1 among 2 test(s)
> | 1..2
> | make[3]: *** [t9145-git-svn-master-branch.sh] Error 1
> 
> Does it work for you?  This is with a merge of git 1.7.8-rc0 and
> 1.7.7.2.

Yikes...I had svn tests turned off in my global build script still and
only validated this against t9134.  My apologies.  I'll submit
something proper later this evening.  (Assuming I get a working patch
that passes the whole suite.)

Sorry for the clumsy patch.

Thanks
-Ben


--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Eric Wong]

Ben Walton <bwalton@artsci.utoronto.ca> wrote:
> Sorry for the clumsy patch.

I don't have much time to help you fix it, but I got numerous errors on
SVN 1.6.x (svn 1.6.12).  Can you make sure things continue to work on
1.6 and earlier, also?

Maybe just enable the escaping for file:// on >= SVN 1.7

Here are the tests that failed for me:

make[1]: *** [t9100-git-svn-basic.sh] Error 1
make[1]: *** [t9103-git-svn-tracked-directory-removed.sh] Error 1
make[1]: *** [t9104-git-svn-follow-parent.sh] Error 1
make[1]: *** [t9105-git-svn-commit-diff.sh] Error 1
make[1]: *** [t9107-git-svn-migrate.sh] Error 1
make[1]: *** [t9108-git-svn-glob.sh] Error 1
make[1]: *** [t9109-git-svn-multi-glob.sh] Error 1
make[1]: *** [t9110-git-svn-use-svm-props.sh] Error 1
make[1]: *** [t9111-git-svn-use-svnsync-props.sh] Error 1
make[1]: *** [t9114-git-svn-dcommit-merge.sh] Error 1
make[1]: *** [t9116-git-svn-log.sh] Error 1
make[1]: *** [t9117-git-svn-init-clone.sh] Error 1
make[1]: *** [t9118-git-svn-funky-branch-names.sh] Error 1
make[1]: *** [t9120-git-svn-clone-with-percent-escapes.sh] Error 1
make[1]: *** [t9125-git-svn-multi-glob-branch-names.sh] Error 1
make[1]: *** [t9127-git-svn-partial-rebuild.sh] Error 1
make[1]: *** [t9128-git-svn-cmd-branch.sh] Error 1
make[1]: *** [t9130-git-svn-authors-file.sh] Error 1
make[1]: *** [t9135-git-svn-moved-branch-empty-file.sh] Error 1
make[1]: *** [t9136-git-svn-recreated-branch-empty-file.sh] Error 1
make[1]: *** [t9141-git-svn-multiple-branches.sh] Error 1
make[1]: *** [t9145-git-svn-master-branch.sh] Error 1
make[1]: *** [t9146-git-svn-empty-dirs.sh] Error 1
make[1]: *** [t9150-svk-mergetickets.sh] Error 1
make[1]: *** [t9151-svn-mergeinfo.sh] Error 1
make[1]: *** [t9153-git-svn-rewrite-uuid.sh] Error 1
make[1]: *** [t9154-git-svn-fancy-glob.sh] Error 1
make[1]: *** [t9155-git-svn-fetch-deleted-tag.sh] Error 1
make[1]: *** [t9156-git-svn-fetch-deleted-tag-2.sh] Error 1
make[1]: *** [t9157-git-svn-fetch-merge.sh] Error 1
make[1]: *** [t9159-git-svn-no-parent-mergeinfo.sh] Error 1
make[1]: *** [t9161-git-svn-mergeinfo-push.sh] Error 1

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Ben Walton]

Excerpts from Eric Wong's message of Wed Nov 02 18:09:41 -0400 2011:

Hi Eric,

> I don't have much time to help you fix it, but I got numerous errors
> on SVN 1.6.x (svn 1.6.12).  Can you make sure things continue to
> work on 1.6 and earlier, also?

Yes, it's a bit of a mess, I think.  It looks as though the
modification required within Git::SVN::Ra is going to negatively
impact other code paths that interact with that package from the
outside.

For example, when doing git svn init --minimize-url ..., the minimized
url is not escaped while the url is.  The minimized url is used to
strip off the head from the full url using a regex.  This now breaks
because of the escaping.

Fixing this locally to the use of the minimized url let me move on
farther but I then got another core dump.

> Maybe just enable the escaping for file:// on >= SVN 1.7

I think that it would be best if this change was only effective for
1.7.

I wonder if all URL-ish objects should be (conditionally iff svn >=
1.7) subjected to escaping?

This would require some restructuring and will take me a bit of time
to work out as I need to familiarize myself with the code to a deeper
level.

Pointers welcomed. :)

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Jonathan Nieder]

Ben Walton wrote:

> Fixing this locally to the use of the minimized url let me move on
> farther but I then got another core dump.

If it continues like this, it might be possible to get help from svn
developers.

E.g., I would love to see a nice summary of the relevant API changes,
like so (except with more truth):

  Previously the svn_frob() function would accept a filename with
  spaces in it in its "path" argument.  Only the svn_plugh() function
  and its relatives required escaped paths.  And all functions
  returning paths would unescape them.

  With Subversion 1.7, passing a filename with a space in it to
  svn_frob() trips an assertion, so we have to escape the "path"
  argument.  This requires ... changes in application code.

  Unfortunately, back in Subversion 1.6, svn_frob() escapes its
  argument, so an application modified as above will find its "path"
  argument to be double-escaped.  There does not seem to be any
  way for applications to target both Subversion 1.6 and 1.7 without
  doing ...

  Subversion 1.8 should follow the following simple, consistent
  semantics when requested with a flag, which would allow me to easily
  target my code against both SVN >= 1.8 and <= 1.6 with a few "if"
  statements (forgetting 1.7 as if it were a bad dream).
 
Or to put it another way: if getting git-svn to cooperate starts to
prove difficult, maybe abstracting away from git and understanding
what changed for libsvn callers in general could help.  On one hand,
it could help in making sure the guarantees that libsvn offers are
well documented, stable in the future, and clear.  On the other hand,
it might make the appropriate changes to git more obvious.

Sorry for the ramble.  I wish I could offer a fix instead.
Jonathan

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Jonathan Nieder]

Hi Eric,

Eric Wong wrote:

> I don't have much time to help you fix it, but I got numerous errors on
> SVN 1.6.x (svn 1.6.12).  Can you make sure things continue to work on
> 1.6 and earlier, also?
[...]
> Here are the tests that failed for me:
>
> make[1]: *** [t9100-git-svn-basic.sh] Error 1
[...]

I tried the patch with subversion 1.6.17dfsg-3 from Debian and all
tests passed.  Odd.  Could you send output from running the test with
-v -i with Ben's patch applied?

Ah, the subversion CHANGES file mentions a bugfix that might explain
the difference:

| Version 1.6.13
[...]
|    * properly canonicalize a URL (r984928, -31)

Thanks,
Jonathan

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Jonathan Nieder]

Hi again,

Ben Walton wrote:

> Previously only http/https URL's were uri escaped.  When building
> against subversion 1.7, this was causing a segfault in perl after
> an assertion failure in the SVN::Ra bindings during in t9134.

(Not a segfault, just a core dump.)  Thanks.

[....]
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -5366,6 +5366,9 @@ sub escape_url {
>  	if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
>  		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
>  		$url = "$scheme://$domain$uri";
> +	} elsif ($url =~ m#^(file)://(.*)$#) {
> +		my ($scheme, $uri) = ($1, escape_uri_only($2));
> +		$url = "$scheme://$uri";

This has two obvious effects, one good and one bad.

The good effect is that it converts spaces to %20.  Both old and new
versions of Subversion seem to be happy to treat %20 as a space, and
new versions of Subversion are not happy to treat a space as a space,
so this conversion can only be a good thing.

The bad effect is that it converts percent signs to %25.  So commands
like "git svn clone file:///path/to/test%20repository" that previously
worked might not work any more, if v1.6.5-rc0~61 (svn: assume URLs
from the command-line are URI-encoded, 2009-08-16) did not do its job
completely.

In other words, it seems to me like you are on the right track. ;-)

Another possible approach: to imitate the svn command line tools, we
could use SVN::Client::url_from_path in some appropriate place. 

Next steps:

 - track down the trouble on svn 1.6.x that Eric mentioned
 - fix any remaining tests that still don't pass

Thanks for getting this started.  Will sleep and play with it a little
more.

Jonathan

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Ben Walton]

Excerpts from Jonathan Nieder's message of Sat Dec 17 04:50:19 -0500 2011:

Hi Jonathan,

Thanks for following up on this.  I've intermittently spent time
digging away at it but other than a few poorly placed fixes that
allowed me to get further into the test suite before failure, I
haven't found a workable fix yet.  I've included my wip patch below as
it may help others that are more familiar with the workings of git-svn
isolate a nice clean place to solve the problem.

> The bad effect is that it converts percent signs to %25.  So
> commands like "git svn clone file:///path/to/test%20repository" that
> previously worked might not work any more, if v1.6.5-rc0~61 (svn:
> assume URLs from the command-line are URI-encoded, 2009-08-16) did
> not do its job completely.

I wonder if simply doing a uri decode followed by a uri encode might
work?  That would decode things that already had some encoding and
then re-encode everything to handle a mixed encodings...I really think
that the svn code should take 'raw' strings and encode internally but
that ship has sailed.

> Another possible approach: to imitate the svn command line tools, we
> could use SVN::Client::url_from_path in some appropriate place.

I've been looking at these functions too.

Thanks
-Ben

>From 98a6b6b4f26a644db5089fce68be6cf7261e4fe1 Mon Sep 17 00:00:00 2001
From: Ben Walton <bwalton@opencsw.org>
Date: Wed, 9 Nov 2011 02:39:05 +0100
Subject: [PATCH] selectively use svn functions to sanitize urls

Signed-off-by: Ben Walton <bwalton@opencsw.org>
---
 git-svn.perl |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 351e743..fb1ce65 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1342,9 +1342,13 @@ sub escape_uri_only {
 
 sub escape_url {
 	my ($url) = @_;
-	if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
-		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
-		$url = "$scheme://$domain$uri";
+	if ($SVN::Core::VERSION =~ /^1\.[0-6]/) {
+		if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) {
+			my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
+			$url = "$scheme://$domain$uri";
+		}
+	} else {
+		$url = SVN::_Core::svn_uri_canonicalize($url);
 	}
 	$url;
 }
@@ -2222,7 +2226,7 @@ sub init_remote_config {
 					     "$url => $min_url\n";
 			}
 			my $old_path = $self->{path};
-			$self->{path} = $url;
+			$self->{path} = SVN::_Core::svn_uri_canonicalize($url);
 			$self->{path} =~ s!^\Q$min_url\E(/|$)!!;
 			if (length $old_path) {
 				$self->{path} .= "/$old_path";
@@ -5363,9 +5367,13 @@ sub escape_uri_only {
 
 sub escape_url {
 	my ($url) = @_;
-	if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
-		my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
-		$url = "$scheme://$domain$uri";
+	if ($SVN::Core::VERSION =~ /^1\.[0-6]/) {
+		if ($url =~ m#^(https?)://([^/]+)(.*)$#) {
+			my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3));
+			$url = "$scheme://$domain$uri";
+		}
+	} else {
+		$url = SVN::_Core::svn_uri_canonicalize($url);
 	}
 	$url;
 }
-- 
1.7.6.1
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Escape file:// URL's to meet subversion SVN::Ra requirements

[Michael Witten]

On 2011-11-04 02:11:49 GMT, Ben Walton wrote:

> Excerpts from Eric Wong's message of Wed Nov 02 18:09:41 -0400 2011:
>
> Hi Eric,
> 
>> I don't have much time to help you fix it, but I got numerous errors
>> on SVN 1.6.x (svn 1.6.12).  Can you make sure things continue to
>> work on 1.6 and earlier, also?
>
> Yes, it's a bit of a mess, I think.  It looks as though the
> modification required within Git::SVN::Ra is going to negatively
> impact other code paths that interact with that package from the
> outside.
>
> For example, when doing git svn init --minimize-url ..., the minimized
> url is not escaped while the url is.  The minimized url is used to
> strip off the head from the full url using a regex.  This now breaks
> because of the escaping.
>
> Fixing this locally to the use of the minimized url let me move on
> farther but I then got another core dump.
>
>> Maybe just enable the escaping for file:// on >= SVN 1.7
>
> I think that it would be best if this change was only effective for
> 1.7.
>
> I wonder if all URL-ish objects should be (conditionally iff svn >=
> 1.7) subjected to escaping?
>
> This would require some restructuring and will take me a bit of time
> to work out as I need to familiarize myself with the code to a deeper
> level.
>
> Pointers welcomed. :)

This problem still exists. It should be fixed---preferably by the people
who built this apparently unwieldy contraption.

Sincerely,
Michael Witten