Improve visibility of repository credential errors

Summary:
Fixes T7310. We have a whole mechanism for surfacing update errors, but only surface actual update errors, not pull errors.

Instead, surface pull errors too.

Then format them a little more nicely.

Test Plan: {F309769}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7310

Differential Revision: https://secure.phabricator.com/D11821

src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php

@@ -963,11 +963,34 @@ final class DiffusionRepositoryEditMainController
     if ($message) {
       switch ($message->getStatusCode()) {
         case PhabricatorRepositoryStatusMessage::CODE_ERROR:
+          $message = $message->getParameter('message');
+
+          $suggestion = null;
+          if (preg_match('/Permission denied \(publickey\)./', $message)) {
+            $suggestion = pht(
+              'Public Key Error: This error usually indicates that the '.
+              'keypair you have configured does not have permission to '.
+              'access the repository.');
+          }
+
+          $message = phutil_escape_html_newlines($message);
+
+          if ($suggestion !== null) {
+            $message = array(
+              phutil_tag('strong', array(), $suggestion),
+              phutil_tag('br'),
+              phutil_tag('br'),
+              phutil_tag('em', array(), pht('Raw Error')),
+              phutil_tag('br'),
+              $message,
+            );
+          }
+
           $view->addItem(
             id(new PHUIStatusItemView())
               ->setIcon(PHUIStatusItemView::ICON_WARNING, 'red')
               ->setTarget(pht('Update Error'))
-              ->setNote($message->getParameter('message')));
+              ->setNote($message));
           return $view;
         case PhabricatorRepositoryStatusMessage::CODE_OKAY:
           $ago = (PhabricatorTime::getNow() - $message->getEpoch());

src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php

@@ -53,6 +53,12 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
     $repository = head($repos);
     $callsign = $repository->getCallsign();
 
+    try {
+      $lock_name = get_class($this).':'.$callsign;
+      $lock = PhabricatorGlobalLock::newLock($lock_name);
+
+      $lock->lock();
+
       $no_discovery = $args->getArg('no-discovery');
 
       id(new PhabricatorRepositoryPullEngine())
@@ -61,19 +67,14 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
         ->pullRepository();
 
       if ($no_discovery) {
+        $lock->unlock();
         return;
       }
 
-    // TODO: It would be nice to discover only if we pulled something, but this
+      // TODO: It would be nice to discover only if we pulled something, but
-    // isn't totally trivial. It's slightly more complicated with hosted
+      // this isn't totally trivial. It's slightly more complicated with hosted
       // repositories, too.
 
-    $lock_name = get_class($this).':'.$callsign;
-    $lock = PhabricatorGlobalLock::newLock($lock_name);
-
-    $lock->lock();
-
-    try {
       $repository->writeStatusMessage(
         PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
         null);