Correct post-review error messages when run outside of a checkout directory, when Mercurial is not installed

 






 

 


  Review Request #322
  —
  Created March 28, 2008 and submitted

 


















 

  

   Information

  





 
 

  marc
 









 
 

  Review Board SVN (deprecated)
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Currently, if you call post-review outside of a checkout directory (somewhere that no SCM info is available), and you do not have Mercurial installed, you will see this error (using a bash shell):

Failed to execute command: hg root
/bin/sh: hg: command not found

That's not a very helpful or clear error message.  This patch causes the following error message to appear instead:

The current directory does not contain a checkout from a
supported source code repository.

That's much more clear and is a correct statement of the problem.

It looks like the Mercurial code was only tested on machines where Mercurial is installed.  I think the code as written would work fine in that case -- but, not all of us have it installed.

 








  


 
 

  
I don't have Mercurial installed (hence this patch), but I have tested that post-review still works outside of a source controlled directory (giving a valid error), and inside of a Subversion checkout directory.

 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       http://reviewboard.googlecode.com/svn/trunk/reviewboard/contrib/tools/post-review
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    This isn't going to work. It may say "hg: command not found" on your shell, but on mine it says "command not found: hg" and other shells may say other things entirely. We need a better solution that we can apply to each SCMClient.

I suggest adding a function to check if the binary is in the path. We can call it in each SCMClient before running it.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      here (bash) hg root produces a path on stdout or nothing; nothing or an error message on stderr.
I guess changing execute so that it does not (optionally?) maps stderr to stdout in its call to subprocess.Popen, and testing for empty result might work for every case.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Yeah, I was thinking about this after I posted the patch.  The way I normally do this is with which.py (see http://trentm.com/projects/which/ ), but I didn't want to introduce an external dependency.  I'll figure something out.

Thanks much.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Christian, I made the change you suggested, but only added it to the MercurialClient so far.  If this looks right, let me know and I'll add the same idea to the other SCMClient subclasses.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      That looks good. Yeah, if you could apply it to the other classes, that'd be great.

Can you also make sure the trailing whitespace (as shown in the diff viewer) is removed?

Thanks!
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Done.  (I chopped the trailing whitespace, too.  There are a bunch of indentation inconsistencies, but I'll deal with those as another change.)
      
 
      

      6. 



    6. 
 
 
      
 
      
  
 
      
 
      
  
      Fixed some of the remaining indentation issues and line 80 column line wrapping and committed in r1244. Thanks!
      
 
      

      7. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4.