threads / discuss / 28826

(unknown)

Subject: (unknown)

## tl;dr

11 messages between Nov 2, 2011 and Jul 16, 2012.

replies: 10people: 4as markdown or json

Ben Walton· Nov 2, 2011, 16:38 UTC · lore
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: 
Ben Walton· Nov 2, 2011, 16:38 UTC · re: Ben Walton · lore

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

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
Jonathan Nieder· Nov 2, 2011, 18:20 UTC · re: Ben Walton · lore

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

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.
Ben Walton· Nov 2, 2011, 19:05 UTC · re: Jonathan Nieder · lore

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

Excerpts from Jonathan Nieder's message of Wed Nov 02 14:20:38 -0400 2011:
Show 6 quoted lines
> 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.

Show 11 quoted lines
> 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.
Show 6 quoted lines
> | # 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

Eric Wong· Nov 2, 2011, 22:09 UTC · re: Ben Walton · lore

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

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

Ben Walton· Nov 4, 2011, 02:11 UTC · re: Eric Wong · lore

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

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

Jonathan Nieder· Nov 4, 2011, 06:38 UTC · re: Ben Walton · lore

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

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

Michael Witten· Jul 16, 2012, 20:05 UTC · re: Ben Walton · lore

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

On 2011-11-04 02:11:49 GMT, Ben Walton wrote:
Show 34 quoted lines
> 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

Jonathan Nieder· Dec 17, 2011, 08:45 UTC · re: Eric Wong · lore

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

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

Jonathan Nieder· Dec 17, 2011, 09:50 UTC · re: Ben Walton · lore

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

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.
[....]
Show 9 quoted lines
> --- 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
Ben Walton· Dec 17, 2011, 23:48 UTC · re: Jonathan Nieder · lore

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

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.

Show 5 quoted lines
> 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

← back to recent threads