Fix pysvn handling of URLs with whitespace in list_dir.

 






 

 


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

 


















 

  

   Information

  





 
 

  hopfeld
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
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




   

   





  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/svn/pysvn.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/svn/pysvn.py


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
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. 



    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. 



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

      
 
      

      4. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Discarded