Make minor correctness and display improvements to pull logs

Summary:
Depends on D18915. Ref T13046.

  - Distinguish between HTTP and HTTPS.
  - Use more constants and fewer magical strings.
  - For HTTP responses, give them better type information and more helpful UI behaviors.

Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

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

src/applications/diffusion/controller/DiffusionServeController.php

@@ -104,15 +104,29 @@ final class DiffusionServeController extends DiffusionController {
     try {
       $remote_addr = $request->getRemoteAddress();
 
+      if ($request->isHTTPS()) {
+        $remote_protocol = PhabricatorRepositoryPullEvent::PROTOCOL_HTTPS;
+      } else {
+        $remote_protocol = PhabricatorRepositoryPullEvent::PROTOCOL_HTTP;
+      }
+
       $pull_event = id(new PhabricatorRepositoryPullEvent())
         ->setEpoch(PhabricatorTime::getNow())
         ->setRemoteAddress($remote_addr)
-        ->setRemoteProtocol('http');
+        ->setRemoteProtocol($remote_protocol);
 
       if ($response) {
+        $response_code = $response->getHTTPResponseCode();
+
+        if ($response_code == 200) {
           $pull_event
-          ->setResultType('wild')
-          ->setResultCode($response->getHTTPResponseCode());
+            ->setResultType(PhabricatorRepositoryPullEvent::RESULT_PULL)
+            ->setResultCode($response_code);
+        } else {
+          $pull_event
+            ->setResultType(PhabricatorRepositoryPullEvent::RESULT_ERROR)
+            ->setResultCode($response_code);
+        }
 
         if ($response instanceof PhabricatorVCSResponse) {
           $pull_event->setProperties(
@@ -122,7 +136,7 @@ final class DiffusionServeController extends DiffusionController {
         }
       } else {
         $pull_event
-          ->setResultType('exception')
+          ->setResultType(PhabricatorRepositoryPullEvent::RESULT_EXCEPTION)
           ->setResultCode(500)
           ->setProperties(
             array(

src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php

@@ -61,11 +61,11 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow {
 
     if ($err) {
       $pull_event
-        ->setResultType('error')
+        ->setResultType(PhabricatorRepositoryPullEvent::RESULT_ERROR)
         ->setResultCode($err);
     } else {
       $pull_event
-        ->setResultType('pull')
+        ->setResultType(PhabricatorRepositoryPullEvent::RESULT_PULL)
         ->setResultCode(0);
     }
 

src/applications/diffusion/ssh/DiffusionSSHWorkflow.php

@@ -272,7 +272,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
     return id(new PhabricatorRepositoryPullEvent())
       ->setEpoch(PhabricatorTime::getNow())
       ->setRemoteAddress($remote_address)
-      ->setRemoteProtocol('ssh')
+      ->setRemoteProtocol(PhabricatorRepositoryPullEvent::PROTOCOL_SSH)
       ->setPullerPHID($viewer->getPHID())
       ->setRepositoryPHID($repository->getPHID());
   }

src/applications/diffusion/view/DiffusionPullLogListView.php

@@ -94,7 +94,7 @@ final class DiffusionPullLogListView extends AphrontView {
           pht('From'),
           pht('Via'),
           null,
-          pht('Error'),
+          pht('Code'),
           pht('Date'),
         ))
       ->setColumnClasses(

src/applications/repository/storage/PhabricatorRepositoryPullEvent.php

@@ -15,6 +15,14 @@ final class PhabricatorRepositoryPullEvent
 
   private $repository = self::ATTACHABLE;
 
+  const RESULT_PULL = 'pull';
+  const RESULT_ERROR = 'error';
+  const RESULT_EXCEPTION = 'exception';
+
+  const PROTOCOL_HTTP = 'http';
+  const PROTOCOL_HTTPS = 'https';
+  const PROTOCOL_SSH = 'ssh';
+
   public static function initializeNewEvent(PhabricatorUser $viewer) {
     return id(new PhabricatorRepositoryPushEvent())
       ->setPusherPHID($viewer->getPHID());
@@ -62,8 +70,9 @@ final class PhabricatorRepositoryPullEvent
 
   public function getRemoteProtocolDisplayName() {
     $map = array(
-      'ssh' => pht('SSH'),
-      'http' => pht('HTTP'),
+      self::PROTOCOL_SSH => pht('SSH'),
+      self::PROTOCOL_HTTP => pht('HTTP'),
+      self::PROTOCOL_HTTPS => pht('HTTPS'),
     );
 
     $protocol = $this->getRemoteProtocol();
@@ -74,25 +83,53 @@ final class PhabricatorRepositoryPullEvent
   public function newResultIcon() {
     $icon = new PHUIIconView();
     $type = $this->getResultType();
+    $code = $this->getResultCode();
+
+    $protocol = $this->getRemoteProtocol();
+
+    $is_any_http =
+      ($protocol === self::PROTOCOL_HTTP) ||
+      ($protocol === self::PROTOCOL_HTTPS);
+
+    // If this was an HTTP request and we responded with a 401, that means
+    // the user didn't provide credentials. This is technically an error, but
+    // it's routine and just causes the client to prompt them. Show a more
+    // comforting icon and description in the UI.
+    if ($is_any_http) {
+      if ($code == 401) {
+        return $icon
+          ->setIcon('fa-key blue')
+          ->setTooltip(pht('Authentication Required'));
+      }
+    }
 
     switch ($type) {
-      case 'wild':
+      case self::RESULT_ERROR:
         $icon
-          ->setIcon('fa-question indigo')
-          ->setTooltip(pht('Unknown ("%s")', $type));
+          ->setIcon('fa-times red')
+          ->setTooltip(pht('Error'));
         break;
-      case 'pull':
+      case self::RESULT_EXCEPTION:
+        $icon
+          ->setIcon('fa-exclamation-triangle red')
+          ->setTooltip(pht('Exception'));
+        break;
+      case self::RESULT_PULL:
         $icon
           ->setIcon('fa-download green')
           ->setTooltip(pht('Pull'));
         break;
+      default:
+        $icon
+          ->setIcon('fa-question indigo')
+          ->setTooltip(pht('Unknown ("%s")', $type));
+        break;
     }
 
     return $icon;
   }
 
 
-
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */