Improved Bazaar SCM support

 






 

 


  Review Request #668
  —
  Created Dec. 15, 2008 and submitted

 


















 

  

   Information

  





 
 

  aogail
 









 
 

  Review Board SVN (deprecated)
 









 
 

  trunk
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Improved BZRTool. The highlights are:

  * It no longer chokes on diffs with new files -- and it properly identifies them as such.
  * It pays attention to basedir. If you point Review Board to a bzr repo, you can use the basedir field to specify the branch to which your diff applies.

 








  


 
 

  
Created reviews & viewed their diffs against a bzr repo, including added, modified, and deleted files.

 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/bzr.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    I know this is already done in the function, but I don't think str(revision) is needed. The revision should already be a string.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/bzr.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Weird, but okay. Can we make this a constant in the class instead of hard-coding here?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/bzr.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    You might be able to simplify this:

parts = [self.repository.path.strip("/")]

if basedir:
    parts.append(basedir.strip("/"))

parts.append(path.strip("/"))

return "/".join(parts)


It's becoming clear that we need our own version of the os.path functions so we can os.path.join this and force the separator to be "/".
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Cool; my python-fu is mediocre, so I was hoping someone would point out a better way to deal with those dir separators.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/core.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    I don't like this change, really. Feels like a hack. Also, I'm not sure basedir is what we really want, since this is not the base directory of a diff but rather a project name in the repository, right?

I don't know bzr enough to really say one way or another. Maybe basedir is okay, but I don't want to make it a parameter of these functions. It should be able to be parsed out of the string instead. I think it would have to be anyway, because the common code is going to prepend it to the filenames.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Well, we're using basedir just like the common SVN use case: You set up a repository in Review Board, with the URL to the root of the repo; then you create reviews that point to a particular branch within the repo -- and the path to the branch is specified in basedir.

I don't think I know what you mean by "parsed out of the string." In parse_diff_revision, file_str does not include the basedir. That was fine, since most of the other SCMTool classes don't hit the repo in that function, but BZRTool does. (It has to, to retrieve revnos... whether revnos are necessary is another question.)
      
 
      

      2. 


 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   





 AO





   

   

  

 



 















 



 

  

   





 AO





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  









  

   

    Change Summary:
   

   

    
We've been using Review Board w/ Bazaar more the past few days, and as a result I have made further changes to Review Board's bzr support.

The main thing is: no more converting from timestamps to revnos. It really isn't necessary, and makes the code simpler to just work with timestamps, and makes it work with all the diffs we discovered that it had trouble with. Using timestamps also makes it unnecessary to have basedir in parse_diff_revision, so that objectionable change has gone away. ;)

   

  








  

   

    Diff:
   

   

    
 Revision 4 (+47 -45)
Show changes
 
    http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/bzr.py 
 

   

  












  

 
















 





 

  

   





 PL





   

   





  

 



 

  


   

   

    
 
plumpy


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/scmtools/bzr.py
       


        (Diff revision 4)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    I don't think this function is ever used (in any of the backends as far as I can tell). I noticed this because first you use a variable local_datetime and then later you call it local_timestamp. I was wondering what was going on, but it turns out this function just never gets called (as far as I can tell).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks good. I fixed the local_datetime vs. local_timestamp issue. We can consider removing this function later, since it's unused.

Committed as r1641. Thanks!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.