Fix pysvn handling of URLs with whitespace in list_dir.

Review Request #8151 — Created May 9, 2016 and discarded

Information

Review Board
master

Reviewers

The pysvn Client.list_dir() method did not properly handle SVN URLs which contain whitespace. pysvn.Client internally converts whitespace in URLs to '%20' which causes the url string to be 2 chars longer than the whitespace version.

list_dir() did rely on the length of the repo_path string to extract the directory name. This resulted in directory names prefixed with two bytes from the original URL.

Example:
list_dir("http://my-svn-server.local/svn/123 project/")
returned 'cttrunk/', 'ctbranches', 'cttags/' when in reality the
directories trunk, branches and tags were present

This fix uses self.repository_info['url'] as repository root, which is already parsed by the pysvn.Client library and contains the '%20' replacements.

Ran unit tests.

When creating a new review request to a Subversion repository containing whitespace in the URL, the trunk/branches/tags selection field did not work. This now is fixed and the branches/trunk/tags are listed as expected.

Did not check whether this affects the subvertpy implementation as well.

reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/svn/pysvn.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/svn/pysvn.py
    
    
  2. 
      
chipx86
  1. Can you add unit tests for this? We require them for checkin, and to make sure it doesn't regress down the road.

    1. Sure. This shall be a general unittest covering both pysvn and subvertpy implementation, since both have to deal with URLs containing whitespace.

      I'll have a look into this.

    2. Looking at the current Subversion test infrastructure I found a repository with test data below
      reviewboard/scmtools/testdata/svn_repo.
      This repository is structed as a typical single project SVN repostiory.

      In order to develop a good testcase for this fix, a SVN repository with a bit more depth will be required:

      /
      projects/
      100 FirstProject/
      trunk
      branches
      ...
      200 SecondProject/
      trunk
      branches
      ...

      I need some advise whether this directory structure shall be added to the current testdata/svn_repo or shall a new repository be created?
      What is the preferred solution?

    3. Martin, I'll have to defer to the Beanbag team to give you a definitive answer but I know when I had a similar question in regards to adding test data for RBTools the answer was it was okay to add data for new tests (https://groups.google.com/d/msg/reviewboard-dev/RkPA07yCIjo/O4cu8rKm73wJ).

      I looked into this a bit today to see if I could come up with any slick approaches, but none were obvious to me. You may have already figured this out, but I'll share what I learned. I think the primary challenge is that self.repopath is where you need the whitespace to show up and that is typically not going to happen since this path nominally corresponds to .../reviewboard/scmtools/testdata/svn_repo. As far as I can tell, adding new folders with spaces in their names into the existing repo hierarchy isn't going to cause any existing tests to fail. I thought about some setup-time trick to symlink or rename svn_repo to something with spaces, but I think that is just asking for trouble when a test terminates and then you are left with dangling or unexpected paths.

      What I ultimately ended up doing is svn copying branches/branch1 from the testdata repo to branches/branch2 with spaces. Then I added a new test and played some tricks to force self.repopath to contain the full path with spaces. This required your fix to pass the test for PySVN, and also required a hack for SubvertPy. I didn't dig into this too much, but it seemed unhappy with the spaces and I had to escape them to get things to work.

      For reference, here is what I hacked together:

      diff --git a/reviewboard/scmtools/svn/pysvn.py b/reviewboard/scmtools/svn/pysvn.py
      index a681548..9fbc770 100644
      --- a/reviewboard/scmtools/svn/pysvn.py
      +++ b/reviewboard/scmtools/svn/pysvn.py
      @@ -220,7 +220,7 @@ class Client(base.Client):
               norm_path = self.normalize_path(path)
               dirents = self.client.list(norm_path, recurse=False)[1:]
      
      -        repo_path_len = len(self.repopath)
      +        repo_path_len = len(self.repository_info['url'])
      
               for dirent, unused in dirents:
                   name = dirent['path'].split('/')[-1]
      diff --git a/reviewboard/scmtools/svn/subvertpy.py b/reviewboard/scmtools/svn/subvertpy.py
      index 9374cd6..57153ad 100644
      --- a/reviewboard/scmtools/svn/subvertpy.py
      +++ b/reviewboard/scmtools/svn/subvertpy.py
      @@ -267,6 +267,8 @@ class Client(base.Client):
               # subvertpy asserts that svn_uri not ends with slash
               norm_path = B(self.normalize_path(path)).rstrip('/')
      
      +        norm_path = norm_path.replace(' ', '%20')
      +
               dirents = self.client.list(norm_path, None, depth)
      
               for name, dirent in six.iteritems(dirents):
      diff --git a/reviewboard/scmtools/testdata/svn_repo/db/current b/reviewboard/scmtools/testdata/svn_repo/db/current
      index 51f314e..566fd37 100644
      --- a/reviewboard/scmtools/testdata/svn_repo/db/current
      +++ b/reviewboard/scmtools/testdata/svn_repo/db/current
      @@ -1 +1 @@
      -10 l 3
      +11 n 4
      diff --git a/reviewboard/scmtools/tests.py b/reviewboard/scmtools/tests.py
      index 675fbc0..fadc099 100644
      --- a/reviewboard/scmtools/tests.py
      +++ b/reviewboard/scmtools/tests.py
      @@ -1233,17 +1233,35 @@ class CommonSVNTestsBase(SpyAgency, SCMTestCase):
               self.assertEqual(files[0].insert_count, 0)
               self.assertEqual(files[0].delete_count, 0)
      
      +    def test_get_branches2(self):
      +        """Testing SVN (<backend>) get_branches2"""
      +        p = os.path.join(os.path.dirname(__file__),
      +                         'testdata/svn_repo/branches/branch2 with spaces')
      +        self.repository.path = 'file://' + p
      +
      +        self.tool = self.repository.get_scmtool()
      +
      +        branches = self.tool.get_branches()
      +
      +        self.assertEqual(len(branches), 1)
      +        self.assertEqual(branches[0], Branch(id='doc',
      +                                             name='doc',
      +                                             commit='5', default=True))
      +
           def test_get_branches(self):
               """Testing SVN (<backend>) get_branches"""
               branches = self.tool.get_branches()
      
      -        self.assertEqual(len(branches), 3)
      +        self.assertEqual(len(branches), 4)
               self.assertEqual(branches[0], Branch(id='trunk', name='trunk',
                                                    commit='9', default=True))
               self.assertEqual(branches[1], Branch(id='branches/branch1',
                                                    name='branch1',
                                                    commit='7', default=False))
      -        self.assertEqual(branches[2], Branch(id='top-level-branch',
      +        self.assertEqual(branches[2], Branch(id='branches/branch2 with spaces',
      +                                             name='branch2 with spaces',
      +                                             commit='11', default=False))
      +        self.assertEqual(branches[3], Branch(id='top-level-branch',
                                                    name='top-level-branch',
                                                    commit='10', default=False))
      
  2. 
      
david
Review request changed

Status: Discarded

Loading...