> -----Original Message-----
> From: Bert Huijben [mailto:bert_at_qqmail.nl]
> Sent: donderdag 5 september 2013 22:59
> To: 'Philip Martin'; 'Daniel Shahaf'
> Cc: 'James McCoy'; dev_at_subversion.apache.org
> Subject: RE: upgrade_tests 7 FAIL when working directory has a "+"
character
>
>
>
> > -----Original Message-----
> > From: Philip Martin [mailto:philip.martin_at_wandisco.com]
> > Sent: donderdag 5 september 2013 21:20
> > To: Daniel Shahaf
> > Cc: James McCoy; dev_at_subversion.apache.org
> > Subject: Re: upgrade_tests 7 FAIL when working directory has a "+"
> character
> >
> > Philip Martin <philip.martin_at_wandisco.com> writes:
> >
> > > Daniel Shahaf <danielsh_at_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.
>
> How does this path enter 'svn'?
>
> Shouldn't this just be handled by the 'svn_uri_canonicalize()' that
handles
> all url commandline arguments?
>
> (Or does this code introduce it in the raw 'entries' files before the
> conversion?)
Yes, basic_upgrade_1_0 (=upgrade tests 7) does indeed do a raw search and
replace on the entries file via xml_entries_relocate().
So the fix is probably to just add the necessary svn_uri_canonicalize() call
in the right place of the upgrade (=read from 'entries') code.
[Note the other mail: Changing the canonicalization rules requires a format
bump, as that can make urls in wc.db invalid]
> Bert
Received on 2013-09-05 23:07:16 CEST