phabstatus: fail gracefully if necessary arcrc settings are missing

Summary:
If the user does not have necesary credentials defined in their arc
configuration, catch the KeyError and convert it into an ArcConfigError.

The existing call sites in the phabstatus and arcdiff extensions catch and
handle ArcConfigError, but not generic KeyErrors.

This also fixes the phabstatus warning messages to end with a newline.

Test Plan: Added a unit test.

Reviewers: #sourcecontrol, quark, simonfar, wez, rmcelroy

Reviewed By: wez, rmcelroy

Subscribers: rmcelroy, net-systems-diffs@fb.com, yogeshwer, mjpieters

Differential Revision: https://phabricator.intern.facebook.com/D4800977

Tasks: 17002914

Signature: t1:4800977:1490847078:e18bba042e3ff57100e0a7b25c610b5cad17fa2e

hgext3rd/phabstatus.py

@@ -80,12 +80,12 @@ def getdiffstatus(repo, *diffid):
     except conduit.ClientError as ex:
         msg = _('Error talking to phabricator. No diff information can be '
                 'provided.\n')
-        hint = _("Error info: ") + str(ex)
+        hint = _("Error info: %s\n") % str(ex)
         return _fail(repo, diffid, msg, hint)
     except arcconfig.ArcConfigError as ex:
         msg = _('arcconfig configuration problem. No diff information can be '
                 'provided.\n')
-        hint = _("Error info: ") + str(ex)
+        hint = _("Error info: %s\n") % str(ex)
         return _fail(repo, diffid, msg, hint)
 
     if not resp:

phabricator/conduit.py

@@ -34,9 +34,13 @@ class Client(object):
 
     def apply_arcconfig(self, config):
         self._host = config.get('conduit_uri', DEFAULT_HOST)
-        hostconfig = config['hosts'][self._host]
-        self._user = hostconfig['user']
-        self._cert = hostconfig['cert']
+        try:
+            hostconfig = config['hosts'][self._host]
+            self._user = hostconfig['user']
+            self._cert = hostconfig['cert']
+        except KeyError:
+            raise arcconfig.ArcConfigError('arcrc is missing user credentials '
+                                           'for host %s' % self._host)
         self._actas = self._user
         self._connection = None
 

tests/test-phabstatus.t

@@ -17,7 +17,8 @@ With an invalid arc configuration
 
   $ hg log -T '{phabstatus}\n' -r .
   arcconfig configuration problem. No diff information can be provided.
-  Error info: no .arcconfig foundError
+  Error info: no .arcconfig found
+  Error
 
 Configure arc...
 
@@ -37,7 +38,8 @@ And now with bad responses:
   > EOF
   $ HG_ARC_CONDUIT_MOCK=$TESTTMP/mockduit hg log -T '{phabstatus}\n' -r .
   Error talking to phabricator. No diff information can be provided.
-  Error info: failed, yoError
+  Error info: failed, yo
+  Error
 
 Missing status field is treated as an error
 
@@ -62,3 +64,12 @@ Make sure the template keywords are documented correctly
   $ hg help templates | egrep 'phabstatus|syncstatus'
       phabstatus    String. Return the diff approval status for a given hg rev
       syncstatus    String. Return whether the local revision is in sync with
+
+Make sure we get decent error messages when .arcrc is missing credential
+information.  We intentionally do not use HG_ARC_CONDUIT_MOCK for this test,
+so it tries to parse the (empty) arc config files.
+
+  $ hg log -T '{phabstatus}\n' -r .
+  arcconfig configuration problem. No diff information can be provided.
+  Error info: arcrc is missing user credentials for host https://phabricator.fb.com/api/
+  Error