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


Review Board SVN (deprecated)


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.
  2. 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. Yeah, I was thinking about this after I posted the patch.  The way I normally do this is with (see ), but I didn't want to introduce an external dependency.  I'll figure something out.
      Thanks much.
    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. 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?
    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. Fixed some of the remaining indentation issues and line 80 column line wrapping and committed in r1244. Thanks!