Add support for finding SVN repository to post-review

 






 

 


  Review Request #351
  —
  Created April 6, 2008 and submitted

 


















 

  

   Information

  





 
 

  plumpy
 









 
 

  Review Board SVN (deprecated)
 









 
 

  trunk
 









 
 

  451
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
The patch resolves the problem described in this thread:
http://groups.google.com/group/reviewboard/browse_thread/thread/c9027e1c73cb0c86

Specifically, users can have many different URLs for the same subversion repository, none of which may match the URL that Review Board uses.  There may be differences in hostnames (is it "svn" or "svn.scm" or "svn.scm.example.com") or even more drastic changes in URLs.  For example, a user could have the repository NFS mounted, in which case she'd have a URL like "file:///mnt/svn/repos/").

This patch uses the JSON "get repository info" API (committed as r1263).  If the local repository is Subversion, we loop through each repository on the server.  First we try to find one that has the same UUID.  If we get that, we check to make sure our local checkout's directory is somewhere beneath the repository URL specified on the server.  If it is, we calculate the new base_path for our diff which is relative to the server's URL and send that up with our diff.

This magic won't work if the server uses DOS '\' delimited paths, but it won't be any worse in that case either... it will just revert to the old behaviour.  And I think that is probably a rare use case anyway (most people will use 'http' URLs).

 








  


 
 

  
I posted many review drafts with varying local repository URLs and server URLs.

 







 









 





  








 









 






  

 








 














 





 

  

   





 PL





   

   





  

 



 

  


   

   

    
 
plumpy


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/webapi/json.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    None of the other errors here had trailing periods, so I removed this one for consistency.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Pinging on this review.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I'll attach an updated review that addresses these comments in a second. Thanks.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      I can see an updated patch (r2 on the diff page), but don't see any mention of it below.  Doesn't notification of new diffs normally show up on this page?

Anyway, not much to add aside from saying this patch works nicely for me.  Very useful.
      
 
      

      4. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
    Can you add a comment describing this function a bit? It's a little weird looking how it may return a new RepositoryInfo or itself, and this should be documented.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Done. Let me know if it makes sense. (There's also a comment on the function it overrides in RepositoryInfo.)
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
    Should probably use isinstanceof().
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      The repository objects are just dicts that come back from the server (JSON objects), not actual useful python classes. So type(repository) is 'dict' and type(repository['tool']) is 'unicode'.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      My mistake, I was thinking wrong when I wrote this.
      
 
      

      3. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
    We'll need to coordinate a little with the change in /r/451/ as that's going to need this repository base path as well.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I looked at that review, but didn't entirely understand what you meant. So I cheerfully ignored this.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I don't know what I meant back then either. *shrug* :) Perhaps I got the number wrong.
      
 
      

      3. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/contrib/tools/post-review
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    What you really want is to do something like:

common_path = os.path.commonprefix([root, path])

if common_path == path:
    return "/"
else:
    return path[len(common_path):]


If you have, say, a base path of: "/svn/foo" and a path of "/svn/foo/trunk/abc", common_path will be "/svn/foo". Your resulting path would then be "/trunk/abc"

If the common prefix is the same as the whole path, then "/".
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Wow, yeah, I was happy to read about that... it would be a lot of reduced code. But os.path.commonprefix is broken to the point of uselessness. From the docs: "Note that this may return invalid paths because it works a character at a time."

In particular, it doesn't work with empty path elements:
>>> os.path.commonprefix(['/svn//foo/bar', '/svn/foo'])
'/svn/'

and even worse, it doesn't actually compare path elements, just characters:
>>> os.path.commonprefix(['/svn/foobar/hello', '/svn/foo'])
'/svn/foo'

      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Yeah that's pretty bad. *sigh*

The first case could be fixed through some path normalization, but the second is just stupid.
      
 
      

      3. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/contrib/tools/post-review
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    No need for the else, due to the return.
    
  
    
 
    

 
    
  



 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/contrib/tools/post-review
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Could just put m.group(1) in the parameter list directly.
    
  
    
 
    

 
    
  



 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Just a few trivial changes left.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
    I'd prefer this written as "rsp = e.args[0]"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
    Sentence casing please.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/contrib/tools/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I think you can compare lists directly:

if rootdirs != pathdirs:
    return None
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      /trunk/reviewboard/contrib/tools/post-review
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Sentence casing, please.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    




    
    Sentence casing, please.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   





 PL





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Just the few changes requested by David.





 



  

   
Diff:

   
    
 Revision 3 (+119 -9)
Show changes
 
    /trunk/reviewboard/contrib/tools/post-review 
 

   		
  



 










  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good now. Committed to SVN as r1642. Thanks!!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...