Review Board

beta

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

Submitted
Updated 7 months, 3 weeks ago

Marc Hedlund Reviewers
reviewboard
None Review Board SVN 
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.
Christian Hammond
Posted 7 months, 3 weeks ago (March 29th, 2008, 4:14 a.m.)

   

  
    
   
  




   



    

     
     

      
       
       
       
       
      
      
       

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

		  (Diff revision 1)

		 
        
       

      

      

       

         876
         
        if data.startswith('abort:'):

         876
         
        if data.startswith('abort:') or data.find('hg: command not found') != -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. 
    
    
     
    
      
      Santiago Gala
      7 months, 3 weeks ago (March 29th, 2008, 4:58 a.m.)
      
     
    
     
    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. 
    
    
     
    
      
      Marc Hedlund
      7 months, 3 weeks ago (March 29th, 2008, 7:50 a.m.)
      
     
    
     
    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. 
    
    
     
    
      
      Marc Hedlund
      7 months, 3 weeks ago (March 29th, 2008, 4:29 p.m.)
      
     
    
     
    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. 
    
    
     
    
      
      Christian Hammond
      7 months, 3 weeks ago (March 29th, 2008, 4:51 p.m.)
      
     
    
     
    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. 
    
    
     
    
      
      Marc Hedlund
      7 months, 3 weeks ago (March 29th, 2008, 6:02 p.m.)
      
     
    
     
    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. 
    
    
     
    
      
      Christian Hammond
      7 months, 3 weeks ago (March 30th, 2008, 1:41 a.m.)
      
     
    
     
    Fixed some of the remaining indentation issues and line 80 column line wrapping and committed in r1244. Thanks!
    
    
    
   
    7.