> -----Original Message-----
> From: Branko Čibej [mailto:br...@wandisco.com]
> Sent: donderdag 5 september 2013 22:00
> To: dev@subversion.apache.org
> Subject: Re: upgrade_tests 7 FAIL when working directory has a "+" character
>
> On 05.09.2013 21:20, Philip Martin wrote:
> > Philip Martin <philip.mar...@wandisco.com> writes:
> >
> >> Daniel Shahaf <danie...@apache.org> writes:
> >>
> >>> On Wed, Sep 04, 2013 at 11:50:49PM -0400, James McCoy wrote:
> >>>> This was with svn 1.7.9. I haven't had a chance to test newer
> >>>> versions, but looking through the code in trunk, it looks like it
> >>>> would still be a problem.
> >>> Confirmed, it reproduces in trunk:
> >>>
> >>> Index: subversion/tests/cmdline/upgrade_tests.py
> >>>
> ==========================================================
> =========
> >>> --- subversion/tests/cmdline/upgrade_tests.py (revision 1520363)
> >>> +++ subversion/tests/cmdline/upgrade_tests.py (working copy)
> >>> @@ -425,7 +425,7 @@ def simple_entries_replace(path, from_url,
> to_url)
> >>> def basic_upgrade_1_0(sbox):
> >>> "test upgrading a working copy created with 1.0.0"
> >>>
> >>> - sbox.build(create_wc = False)
> >>> + sbox.build(name='foo+', create_wc = False)
> >>> replace_sbox_with_tarfile(sbox, 'upgrade_1_0.tar.bz2')
> >>>
> >>> url = sbox.repo_url
> >>>
> >>> % ../runpytest upgrade basic_upgrade_1_0 2>&1 | head
> >>> W: lt-svn: subversion/libsvn_subr/path.c:119: svn_path_join_internal:
> Assertion `svn_path_is_canonical_internal(base, pool)' failed.
> >>>
> >>> W: CMD: /home/danielsh/src/svn/t1/subversion/svn/svn upgrade 'svn-
> test-work/working_copies/foo+' --config-dir
> /home/danielsh/src/svn/t1/subversion/tests/cmdline/svn-test-
> work/local_tmp/config --password rayjandom --no-auth-cache --username
> jrandom terminated by signal 6
> >> It looks like the %2B is failing svn_uri_is_canonical. Looking at
> >> svn_uri__char_validity in libsvn_subr/path.c the table says that '+'
> >> should not be % encoded. The problem is that the Python testsuite uses
> >> urllib.pathname2url(self.repo_dir) and that has different ideas, it
> >> does % encode '+'.
> > Index: subversion/tests/cmdline/svntest/sandbox.py
> >
> ==========================================================
> =========
> > --- subversion/tests/cmdline/svntest/sandbox.py (revision 1520259)
> > +++ subversion/tests/cmdline/svntest/sandbox.py (working copy)
> > @@ -57,7 +57,7 @@
> > if not read_only:
> > self.repo_dir = os.path.join(svntest.main.general_repo_dir,
> > self.name)
> > self.repo_url = (svntest.main.options.test_area_url + '/'
> > - + urllib.pathname2url(self.repo_dir))
> > + + urllib.quote(self.repo_dir, '/+'))
> > self.add_test_path(self.repo_dir)
> > else:
> > self.repo_dir = svntest.main.pristine_greek_repos_dir
> >
> > That works for '+'. A real patch would need to ensure that the safe set
> > passed to urllib.quote matches the safe set required by Subversion.
>
> This:
>
> http://tools.ietf.org/html/rfc3986#section-2
>
> appears to say that '+' /should/ be %-encoded if it is in the path name
> part of the URL. So it would appear the svn_uri__char_validity is wrong.
> A '+' in the query part is a different matter, it's usually substituted
> for a space in that case. But Subversion does not generate URLs with
> non-empty queries.
Changing this table requires a working copy format bump...
I'm more surprised on how the url can enter the low level libraries without
being canonicalized?
Especially since we re-canonicalize paths from 'entries' on upgrading to
wc_db.db based formats. (Over the years we found a few extreme rare corner
cases, but '+' in a path doesn't look uncommon to me)
Bert
