Review Board

beta

Initial Monotone Support

Submitted
Updated 2 months, 3 weeks ago

Gary Kramlich Reviewers
reviewboard
None Review Board SVN 
This basically works.  It depends on a local database which we somehow need to determine how to update.

Aside from that, binary files are not currently marked, and empty files cause the diff viewer to blow up.

I can provide some diffs for testing if anyone would so desire.

---

Round two, I think I got everything, although I'm not sure how to handle the FileNotFound, since I'm using file id's which don't contain the name or anything...

 

 




    








  

 










 

  




 

 

  
  
Christian Hammond

  
Posted 6 months ago (May 21st, 2008, 11:21 p.m.)

 

 

   
This is a good first pass. After these changes (mostly stylistic) we should get the post-review support written and get the known issues fixed up.

   

  
    
   
  




   



    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         1
         
# vi: et:sw=4 ts=4

       


      

     

    

    

     
Maybe place this last in the file. We don't tend to use these, but I'm not opposed to it. Might be nice to standardize on them.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         6
         
from reviewboard.diffviewer.parser import \

       


       

         
         

         7
         
    DiffParser

       


       

         
         

         8
         
from reviewboard.scmtools.core import \

       


       

         
         

         9
         
    FileNotFoundError, SCMError, SCMTool

       


      

     

    

    

     
The things we're importing should start on the same line as the "from ... import". We should be able to fit one or two per line. Then continue them on the next line, indented with the first thing we're importing.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         9
         
    FileNotFoundError, SCMError, SCMTool

       


       

         
         

         10
         

       


       

         
         

         11
         
class MonotoneClient:

       


      

     

    

    

     
Two blank lines here.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         15
         
        if not os.path.isfile(self.path):

       


       

         
         

         16
         
            raise ImportError

       


      

     

    

    

     
I don't think we want to throw an ImportError. We might want something, but I'd rather we introduce a RepositoryNotFound exception in scmtools/core.py and use that.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         28
         
            if err.find("mtn: misuse: no file") >= 0:

       


      

     

    

    

     
if "mtn: misuse: no file" in err:


	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         29
         
                raise FileNotFoundError

       


      

     

    

    

     
This should include the path and revision as arguments.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         33
         
        return out

       


       

         
         

         34
         

       


       

         
         

         35
         
class MonotoneTool(SCMTool):

       


      

     

    

    

     
Two blank lines here.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         35
         
class MonotoneTool(SCMTool):

       


      

     

    

    

     
SCMTool subclasses tend to be the first class in the file. Not a big deal, but we should try to standardize on that.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         69
         
        return MonotoneDiffParser(data)

       


       

         
         

         70
         

       


       

         
         

         71
         
class MonotoneDiffParser(DiffParser):

       


      

     

    

    

     
Two blank lines here.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         76
         
            if self.lines[linenum].find("is binary"):

       


      

     

    

    

     
If "is binary" in self.lines[linenum]:

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         81
         
                linenum += 1

       


       

         
         

         82
         
        return linenum

       


      

     

    

    

     
Blank line before the return.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /mtn.py
		 

		  (Diff revision 1)

		 
        
       

      

      

       

         
         

         83
         

       


      

     

    

    

     
Excess blank line we can get rid of.

	 

  
    
   
  



    


   


  
 




  

 









 

  




 

 

  
  
Christian Hammond

  
Posted 5 months, 2 weeks ago (June 7th, 2008, 12:06 a.m.)

 

 

   
Looking better.

   

  
    
   
  




   



    

     
     

      
       
       
       
       
      
      
       

        
         /scmtools/mtn.py
		 

		  (Diff revision 2)

		 
        
       

      

      

       

         
         

         67
         
            raise ImportError

       


      

     

    

    

     
This should return an SCMError of some sort. SCMError("Repository %s does not exist" % path) would probably work.

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /scmtools/mtn.py
		 

		  (Diff revision 2)

		 
        
       

      

      

       

         
         

         76
         
        status = p.wait()

       


       

         
         

         77
         

       


       

         
         

         78
         
        if status:

       


      

     

    

    

     
Call status "failure" like the other tools use. You can then squash the next few lines:

if not failure:
    return out

if "mtn: ...."

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /scmtools/mtn.py
		 

		  (Diff revision 2)

		 
        
       

      

      

       

         
         

         79
         
            if "mtn: misuse: no file" in err >= 0:

       


      

     

    

    

     
No ">= 0"

	 

  
    
   
  



    


    

     
     

      
       
       
       
       
      
      
       

        
         /scmtools/mtn.py
		 

		  (Diff revision 2)

		 
        
       

      

      

       

         
         

         80
         
                raise FileNotFoundError

       


      

     

    

    

     
This should include the file and revision information, if they exist. (Provide as much as possible)

	 

  
    
   
  



    


   


  
 




  

 











 

  




 

 

  
Ship it!

  
David Trowbridge

  
Posted 2 months, 3 weeks ago (August 26th, 2008, 12:10 a.m.)

 

 

   
I've gotten tired of seeing this review request moulder here, so I've made the changes Christian asked for, listed the known limitations in a comment, and committed it. Hopefully someone else will come pick this up and finish it.