General and development mailing list for Git, a distributed version control system

Ben Walton | 2 Nov 2011 17:38

Picon

Picon

(unknown)

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 <at> artsci.utoronto.ca>
Subject: 
In-Reply-To:

Ben Walton | 2 Nov 2011 17:38

Picon

Picon

[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 <at> 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
 <at>  <at>  -5366,6 +5366,9  <at>  <at>  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)://(.*)$#) {

(Continue reading)

Jonathan Nieder | 2 Nov 2011 19:20

Picon

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 <at> artsci.utoronto.ca>
> ---
>  git-svn.perl |    3 +++

(Continue reading)

Ben Walton | 2 Nov 2011 20:05

Picon

Picon

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:

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

(Continue reading)

Eric Wong | 2 Nov 2011 23:09

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

Ben Walton <bwalton <at> 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

(Continue reading)

Ben Walton | 4 Nov 2011 03:11

Picon

Picon

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

(Continue reading)

Jonathan Nieder | 4 Nov 2011 07:38

Picon

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

(Continue reading)

Michael Witten | 16 Jul 2012 22:16

Picon

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

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?

(Continue reading)

Jonathan Nieder | 17 Dec 2011 09:45

Picon

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 | 17 Dec 2011 10:50

Picon

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.

[....]
> --- a/git-svn.perl
> +++ b/git-svn.perl
>  <at>  <at>  -5366,6 +5366,9  <at>  <at>  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

(Continue reading)

Ben Walton | 18 Dec 2011 00:48

Picon

Picon

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.

> 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

(Continue reading)

Return

Return to gmane.comp.version-control.git.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane