From 0bf0718fad275dd8fb3a00a454f4e75b076a8b0c Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Fri, 13 Apr 2018 11:25:41 -0700
Subject: [PATCH 01/19] Add isClusterDevice to Almanac query

Summary: Ref T13076. This will be used by the metric collection system to iterate over the cluster devices.

Test Plan: Created some cluster and non-cluster devices, searched and saw expected results.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T13076

Differential Revision: https://secure.phabricator.com/D19368
---
 .../almanac/query/AlmanacDeviceQuery.php            | 13 +++++++++++++
 .../almanac/query/AlmanacDeviceSearchEngine.php     | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/src/applications/almanac/query/AlmanacDeviceQuery.php b/src/applications/almanac/query/AlmanacDeviceQuery.php
index 29461bfbfd..0d38070e0b 100644
--- a/src/applications/almanac/query/AlmanacDeviceQuery.php
+++ b/src/applications/almanac/query/AlmanacDeviceQuery.php
@@ -8,6 +8,7 @@ final class AlmanacDeviceQuery
   private $names;
   private $namePrefix;
   private $nameSuffix;
+  private $isClusterDevice;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -40,6 +41,11 @@ final class AlmanacDeviceQuery
       $ngrams);
   }
 
+  public function withIsClusterDevice($is_cluster_device) {
+    $this->isClusterDevice = $is_cluster_device;
+    return $this;
+  }
+
   public function newResultObject() {
     return new AlmanacDevice();
   }
@@ -90,6 +96,13 @@ final class AlmanacDeviceQuery
         $this->nameSuffix);
     }
 
+    if ($this->isClusterDevice !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'device.isBoundToClusterService = %d',
+        (int)$this->isClusterDevice);
+    }
+
     return $where;
   }
 
diff --git a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php
index 6e28d7e5b8..1d0a1d8475 100644
--- a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php
+++ b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php
@@ -25,6 +25,13 @@ final class AlmanacDeviceSearchEngine
         ->setLabel(pht('Exact Names'))
         ->setKey('names')
         ->setDescription(pht('Search for devices with specific names.')),
+      id(new PhabricatorSearchThreeStateField())
+        ->setLabel(pht('Cluster Device'))
+        ->setKey('isClusterDevice')
+        ->setOptions(
+          pht('Both Cluster and Non-cluster Devices'),
+          pht('Cluster Devices Only'),
+          pht('Non-cluster Devices Only')),
     );
   }
 
@@ -39,6 +46,10 @@ final class AlmanacDeviceSearchEngine
       $query->withNames($map['names']);
     }
 
+    if ($map['isClusterDevice'] !== null) {
+      $query->withIsClusterDevice($map['isClusterDevice']);
+    }
+
     return $query;
   }
 

From 6d1e00707680cbe596e7f2a8c2a66400d1ddddd1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 13 Apr 2018 12:39:49 -0700
Subject: [PATCH 02/19] Try a more conventional spelling of "Convereted"

Summary: This is a good spelling, but maybe a better spelling is possible.

Test Plan: hmmm

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19369
---
 .../management/PhabricatorDifferentialMigrateHunkWorkflow.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
index 99125645e7..0e76bd5ab3 100644
--- a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
+++ b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
@@ -48,13 +48,13 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
         $hunk->saveAsText();
         $this->logOkay(
           pht('TEXT'),
-          pht('Convereted hunk to text storage.'));
+          pht('Converted hunk to text storage.'));
         break;
       case DifferentialHunk::DATATYPE_FILE:
         $hunk->saveAsFile();
         $this->logOkay(
           pht('FILE'),
-          pht('Convereted hunk to file storage.'));
+          pht('Converted hunk to file storage.'));
         break;
     }
 

From e3de7d09c0d8c0a4c1fdd96b005b87746f9a357a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 13 Apr 2018 13:15:46 -0700
Subject: [PATCH 03/19] Add an "--all" flag to "bin/differential migrate-hunk"

Summary:
Depends on D19369. Ref T13120. Add a flag to migrate every hunk.

This isn't terribly useful on its own, but I'm going to add an `--auto` flag next so that you can run `--auto --all` to migrate hunks to the preferred hunk format.

Test Plan: Ran `bin/differential migrate-hunk --all --to text`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19370
---
 ...ricatorDifferentialMigrateHunkWorkflow.php | 92 ++++++++++++++-----
 1 file changed, 67 insertions(+), 25 deletions(-)

diff --git a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
index 0e76bd5ab3..18afd3f359 100644
--- a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
+++ b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
@@ -6,7 +6,9 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
   protected function didConstruct() {
     $this
       ->setName('migrate-hunk')
-      ->setExamples('**migrate-hunk** --id __hunk__ --to __storage__')
+      ->setExamples(
+        "**migrate-hunk** --id __hunk__ --to __storage__\n".
+        "**migrate-hunk** --all")
       ->setSynopsis(pht('Migrate storage engines for a hunk.'))
       ->setArguments(
         array(
@@ -20,14 +22,27 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
             'param' => 'storage',
             'help' => pht('Storage engine to migrate to.'),
           ),
+          array(
+            'name' => 'all',
+            'help' => pht('Migrate all hunks.'),
+          ),
         ));
   }
 
   public function execute(PhutilArgumentParser $args) {
     $id = $args->getArg('id');
-    if (!$id) {
+    $is_all = $args->getArg('all');
+
+    if ($is_all && $id) {
       throw new PhutilArgumentUsageException(
-        pht('Specify a hunk to migrate with --id.'));
+        pht(
+          'Options "--all" (to migrate all hunks) and "--id" (to migrate a '.
+          'specific hunk) are mutually exclusive.'));
+    } else if (!$is_all && !$id) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Specify a hunk to migrate with "--id", or migrate all hunks '.
+          'with "--all".'));
     }
 
     $storage = $args->getArg('to');
@@ -40,31 +55,30 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
           pht('Specify a hunk storage engine with --to.'));
     }
 
-    $hunk = $this->loadHunk($id);
-    $old_data = $hunk->getChanges();
-
-    switch ($storage) {
-      case DifferentialHunk::DATATYPE_TEXT:
-        $hunk->saveAsText();
-        $this->logOkay(
-          pht('TEXT'),
-          pht('Converted hunk to text storage.'));
-        break;
-      case DifferentialHunk::DATATYPE_FILE:
-        $hunk->saveAsFile();
-        $this->logOkay(
-          pht('FILE'),
-          pht('Converted hunk to file storage.'));
-        break;
+    if ($id) {
+      $hunk = $this->loadHunk($id);
+      $hunks = array($hunk);
+    } else {
+      $hunks = new LiskMigrationIterator(new DifferentialHunk());
     }
 
-    $hunk = $this->loadHunk($id);
-    $new_data = $hunk->getChanges();
+    foreach ($hunks as $hunk) {
+      try {
+        $this->migrateHunk($hunk, $storage);
+      } catch (Exception $ex) {
+        // If we're migrating a single hunk, just throw the exception. If
+        // we're migrating multiple hunks, warn but continue.
+        if ($id) {
+          throw $ex;
+        }
 
-    if ($old_data !== $new_data) {
-      throw new Exception(
-        pht(
-          'Integrity check failed: new file data differs fom old data!'));
+        $this->logWarn(
+          pht('WARN'),
+          pht(
+            'Failed to migrate hunk %d: %s',
+            $hunk->getID(),
+            $ex->getMessage()));
+      }
     }
 
     return 0;
@@ -82,5 +96,33 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
     return $hunk;
   }
 
+  private function migrateHunk(DifferentialHunk $hunk, $format) {
+    $old_data = $hunk->getChanges();
+
+    switch ($format) {
+      case DifferentialHunk::DATATYPE_TEXT:
+        $hunk->saveAsText();
+        $this->logOkay(
+          pht('TEXT'),
+          pht('Converted hunk to text storage.'));
+        break;
+      case DifferentialHunk::DATATYPE_FILE:
+        $hunk->saveAsFile();
+        $this->logOkay(
+          pht('FILE'),
+          pht('Converted hunk to file storage.'));
+        break;
+    }
+
+    $hunk = $this->loadHunk($hunk->getID());
+    $new_data = $hunk->getChanges();
+
+    if ($old_data !== $new_data) {
+      throw new Exception(
+        pht(
+          'Integrity check failed: new file data differs fom old data!'));
+    }
+  }
+
 
 }

From b5f23b023e3c3a055e1d446a3a7a60341a3590cc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 16 Apr 2018 06:38:14 -0700
Subject: [PATCH 04/19] Add an "--auto" flag to "bin/differential migrate-hunk"

Summary:
Depends on D19370. See T13124. See PHI549. The particular install in PHI549 migrated a large amount of data via the fallback hunk migration script, which does not compress hunks.

Add a mode to `bin/differential migrate-hunk` that amounts to "compress all the hunks which would benefit from compression".

Test Plan: Ran `bin/differential migrate-hunk` with `--auto`, `--all`, `--to`, `--id`, and `--dry-run` in various mixtures. Forced a bunch of hunks to raw ("byte") format, saw it cleanly upgrade them to compressed ("gzde") format.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19371
---
 ...ricatorDifferentialMigrateHunkWorkflow.php | 116 +++++++++++++++---
 .../differential/storage/DifferentialHunk.php |  22 ++--
 2 files changed, 114 insertions(+), 24 deletions(-)

diff --git a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
index 18afd3f359..0db158e171 100644
--- a/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
+++ b/src/applications/differential/management/PhabricatorDifferentialMigrateHunkWorkflow.php
@@ -26,10 +26,20 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
             'name' => 'all',
             'help' => pht('Migrate all hunks.'),
           ),
+          array(
+            'name' => 'auto',
+            'help' => pht('Select storage format automatically.'),
+          ),
+          array(
+            'name' => 'dry-run',
+            'help' => pht('Show planned writes but do not perform them.'),
+          ),
         ));
   }
 
   public function execute(PhutilArgumentParser $args) {
+    $is_dry_run = $args->getArg('dry-run');
+
     $id = $args->getArg('id');
     $is_all = $args->getArg('all');
 
@@ -45,14 +55,34 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
           'with "--all".'));
     }
 
+    $is_auto = $args->getArg('auto');
     $storage = $args->getArg('to');
-    switch ($storage) {
-      case DifferentialHunk::DATATYPE_TEXT:
-      case DifferentialHunk::DATATYPE_FILE:
-        break;
-      default:
+    if ($is_auto && $storage) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Options "--to" (to choose a specific storage format) and "--auto" '.
+          '(to select a storage format automatically) are mutually '.
+          'exclusive.'));
+    } else if (!$is_auto && !$storage) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Use "--to" to choose a storage format, or "--auto" to select a '.
+          'format automatically.'));
+    }
+
+    $types = array(
+      DifferentialHunk::DATATYPE_TEXT,
+      DifferentialHunk::DATATYPE_FILE,
+    );
+    $types = array_fuse($types);
+    if (strlen($storage)) {
+      if (!isset($types[$storage])) {
         throw new PhutilArgumentUsageException(
-          pht('Specify a hunk storage engine with --to.'));
+          pht(
+            'Storage type "%s" is unknown. Supported types are: %s.',
+            $storage,
+            implode(', ', array_keys($types))));
+      }
     }
 
     if ($id) {
@@ -64,7 +94,7 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
 
     foreach ($hunks as $hunk) {
       try {
-        $this->migrateHunk($hunk, $storage);
+        $this->migrateHunk($hunk, $storage, $is_auto, $is_dry_run);
       } catch (Exception $ex) {
         // If we're migrating a single hunk, just throw the exception. If
         // we're migrating multiple hunks, warn but continue.
@@ -96,31 +126,85 @@ final class PhabricatorDifferentialMigrateHunkWorkflow
     return $hunk;
   }
 
-  private function migrateHunk(DifferentialHunk $hunk, $format) {
+  private function migrateHunk(
+    DifferentialHunk $hunk,
+    $type,
+    $is_auto,
+    $is_dry_run) {
+
+    $old_type = $hunk->getDataType();
+
+    if ($is_auto) {
+      // By default, we're just going to keep hunks in the same storage
+      // engine. In the future, we could perhaps select large hunks stored in
+      // text engine and move them into file storage.
+      $new_type = $old_type;
+    } else {
+      $new_type = $type;
+    }
+
+    // Figure out if the storage format (e.g., plain text vs compressed)
+    // would change if we wrote this hunk anew today.
+    $old_format = $hunk->getDataFormat();
+    $new_format = $hunk->getAutomaticDataFormat();
+
+    $same_type = ($old_type === $new_type);
+    $same_format = ($old_format === $new_format);
+
+    // If we aren't going to change the storage engine and aren't going to
+    // change the storage format, just bail out.
+    if ($same_type && $same_format) {
+      $this->logInfo(
+        pht('SKIP'),
+        pht(
+          'Hunk %d is already stored in the preferred engine ("%s") '.
+          'with the preferred format ("%s").',
+          $hunk->getID(),
+          $new_type,
+          $new_format));
+      return;
+    }
+
+    if ($is_dry_run) {
+      $this->logOkay(
+        pht('DRY RUN'),
+        pht(
+          'Hunk %d would be rewritten (storage: "%s" -> "%s"; '.
+          'format: "%s" -> "%s").',
+          $hunk->getID(),
+          $old_type,
+          $new_type,
+          $old_format,
+          $new_format));
+      return;
+    }
+
     $old_data = $hunk->getChanges();
 
-    switch ($format) {
+    switch ($new_type) {
       case DifferentialHunk::DATATYPE_TEXT:
         $hunk->saveAsText();
-        $this->logOkay(
-          pht('TEXT'),
-          pht('Converted hunk to text storage.'));
         break;
       case DifferentialHunk::DATATYPE_FILE:
         $hunk->saveAsFile();
-        $this->logOkay(
-          pht('FILE'),
-          pht('Converted hunk to file storage.'));
         break;
     }
 
+    $this->logOkay(
+      pht('MIGRATE'),
+      pht(
+        'Converted hunk %d to "%s" storage (with format "%s").',
+        $hunk->getID(),
+        $new_type,
+        $hunk->getDataFormat()));
+
     $hunk = $this->loadHunk($hunk->getID());
     $new_data = $hunk->getChanges();
 
     if ($old_data !== $new_data) {
       throw new Exception(
         pht(
-          'Integrity check failed: new file data differs fom old data!'));
+          'Integrity check failed: new file data differs from old data!'));
     }
   }
 
diff --git a/src/applications/differential/storage/DifferentialHunk.php b/src/applications/differential/storage/DifferentialHunk.php
index 3defb3566b..6bfb38b789 100644
--- a/src/applications/differential/storage/DifferentialHunk.php
+++ b/src/applications/differential/storage/DifferentialHunk.php
@@ -290,14 +290,24 @@ final class DifferentialHunk
     return array(self::DATAFORMAT_RAW, $data);
   }
 
+  public function getAutomaticDataFormat() {
+    // If the hunk is already stored deflated, just keep it deflated. This is
+    // mostly a performance improvement for "bin/differential migrate-hunk" so
+    // that we don't have to recompress all the stored hunks when looking for
+    // stray uncompressed hunks.
+    if ($this->dataFormat === self::DATAFORMAT_DEFLATED) {
+      return self::DATAFORMAT_DEFLATED;
+    }
+
+    list($format) = $this->formatDataForStorage($this->getRawData());
+
+    return $format;
+  }
+
   public function saveAsText() {
     $old_type = $this->getDataType();
     $old_data = $this->getData();
 
-    if ($old_type == self::DATATYPE_TEXT) {
-      return $this;
-    }
-
     $raw_data = $this->getRawData();
 
     $this->setDataType(self::DATATYPE_TEXT);
@@ -317,10 +327,6 @@ final class DifferentialHunk
     $old_type = $this->getDataType();
     $old_data = $this->getData();
 
-    if ($old_type == self::DATATYPE_FILE) {
-      return $this;
-    }
-
     $raw_data = $this->getRawData();
 
     list($format, $data) = $this->formatDataForStorage($raw_data);

From 25965260c43189043ebddde2b118c326759f27b4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 16 Apr 2018 07:09:44 -0700
Subject: [PATCH 05/19] Add a rough "!history" email command to get an entire
 object history via email

Summary:
See PHI505. Ref T13124. If you're an agent of a hostile state trying to exfiltrate corporate secrets, you might find yourself foiled if Phabricator is secured behind a VPN.

To assist users in this situation, provide a "!history" command which will dump the entire history of an object in a nice text format and get through the troublesome VPN.

Some issues with this:

  - You currently get all the "X added a comment." up top, and then all the comments below. This isn't terribly useful.
  - This goes through the "Must Encrypt" flag, but possibly should not? (On the other hand, this is a pretty willful way to bypass it the flag.)

Test Plan: Used `bin/mail receive-test ...` to send `!history` commands, got somewhat-useful response mail.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19372
---
 .../constants/PhabricatorTransactions.php     |  1 +
 ...habricatorApplicationTransactionEditor.php | 67 ++++++++++++++++++-
 .../PhabricatorApplicationTransaction.php     | 11 ++-
 3 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/src/applications/transactions/constants/PhabricatorTransactions.php b/src/applications/transactions/constants/PhabricatorTransactions.php
index 4089187ca4..7a606fe65c 100644
--- a/src/applications/transactions/constants/PhabricatorTransactions.php
+++ b/src/applications/transactions/constants/PhabricatorTransactions.php
@@ -15,6 +15,7 @@ final class PhabricatorTransactions extends Phobject {
   const TYPE_CREATE = 'core:create';
   const TYPE_COLUMNS = 'core:columns';
   const TYPE_SUBTYPE = 'core:subtype';
+  const TYPE_HISTORY = 'core:history';
 
   const COLOR_RED         = 'red';
   const COLOR_ORANGE      = 'orange';
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 70644b1974..685f760b15 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -83,6 +83,7 @@ abstract class PhabricatorApplicationTransactionEditor
   private $webhookMap = array();
 
   private $transactionQueue = array();
+  private $sendHistory = false;
 
   const STORAGE_ENCODING_BINARY = 'binary';
 
@@ -300,6 +301,7 @@ abstract class PhabricatorApplicationTransactionEditor
     $types = array();
 
     $types[] = PhabricatorTransactions::TYPE_CREATE;
+    $types[] = PhabricatorTransactions::TYPE_HISTORY;
 
     if ($this->object instanceof PhabricatorEditEngineSubtypeInterface) {
       $types[] = PhabricatorTransactions::TYPE_SUBTYPE;
@@ -377,6 +379,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     switch ($type) {
       case PhabricatorTransactions::TYPE_CREATE:
+      case PhabricatorTransactions::TYPE_HISTORY:
         return null;
       case PhabricatorTransactions::TYPE_SUBTYPE:
         return $object->getEditEngineSubtype();
@@ -468,6 +471,7 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_INLINESTATE:
       case PhabricatorTransactions::TYPE_SUBTYPE:
+      case PhabricatorTransactions::TYPE_HISTORY:
         return $xaction->getNewValue();
       case PhabricatorTransactions::TYPE_SPACE:
         $space_phid = $xaction->getNewValue();
@@ -520,6 +524,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     switch ($xaction->getTransactionType()) {
       case PhabricatorTransactions::TYPE_CREATE:
+      case PhabricatorTransactions::TYPE_HISTORY:
         return true;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $field = $this->getCustomFieldForTransaction($object, $xaction);
@@ -604,6 +609,7 @@ abstract class PhabricatorApplicationTransactionEditor
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->applyApplicationTransactionInternalEffects($xaction);
       case PhabricatorTransactions::TYPE_CREATE:
+      case PhabricatorTransactions::TYPE_HISTORY:
       case PhabricatorTransactions::TYPE_SUBTYPE:
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
@@ -665,6 +671,7 @@ abstract class PhabricatorApplicationTransactionEditor
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->applyApplicationTransactionExternalEffects($xaction);
       case PhabricatorTransactions::TYPE_CREATE:
+      case PhabricatorTransactions::TYPE_HISTORY:
       case PhabricatorTransactions::TYPE_SUBTYPE:
       case PhabricatorTransactions::TYPE_EDGE:
       case PhabricatorTransactions::TYPE_TOKEN:
@@ -800,6 +807,9 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_SPACE:
         $this->scrambleFileSecrets($object);
         break;
+      case PhabricatorTransactions::TYPE_HISTORY:
+        $this->sendHistory = true;
+        break;
     }
   }
 
@@ -1317,6 +1327,13 @@ abstract class PhabricatorApplicationTransactionEditor
       $this->publishFeedStory($object, $xactions, $mailed);
     }
 
+    if ($this->sendHistory) {
+      $history_mail = $this->buildHistoryMail($object);
+      if ($history_mail) {
+        $messages[] = $history_mail;
+      }
+    }
+
     // NOTE: This actually sends the mail. We do this last to reduce the chance
     // that we send some mail, hit an exception, then send the mail again when
     // retrying.
@@ -2557,6 +2574,25 @@ abstract class PhabricatorApplicationTransactionEditor
       $unexpandable = array();
     }
 
+    $messages = $this->buildMailWithRecipients(
+      $object,
+      $xactions,
+      $email_to,
+      $email_cc,
+      $unexpandable);
+
+    $this->runHeraldMailRules($messages);
+
+    return $messages;
+  }
+
+  private function buildMailWithRecipients(
+    PhabricatorLiskDAO $object,
+    array $xactions,
+    array $email_to,
+    array $email_cc,
+    array $unexpandable) {
+
     $targets = $this->buildReplyHandler($object)
       ->setUnexpandablePHIDs($unexpandable)
       ->getMailTargets($email_to, $email_cc);
@@ -2603,8 +2639,6 @@ abstract class PhabricatorApplicationTransactionEditor
       }
     }
 
-    $this->runHeraldMailRules($messages);
-
     return $messages;
   }
 
@@ -3671,6 +3705,7 @@ abstract class PhabricatorApplicationTransactionEditor
       'mailMutedPHIDs',
       'webhookMap',
       'silent',
+      'sendHistory',
     );
   }
 
@@ -4328,4 +4363,32 @@ abstract class PhabricatorApplicationTransactionEditor
     return true;
   }
 
+  private function buildHistoryMail(PhabricatorLiskDAO $object) {
+    $viewer = $this->requireActor();
+    $recipient_phid = $this->getActingAsPHID();
+
+    // Load every transaction so we can build a mail message with a complete
+    // history for the object.
+    $query = PhabricatorApplicationTransactionQuery::newQueryForObject($object);
+    $xactions = $query
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($object->getPHID()))
+      ->execute();
+    $xactions = array_reverse($xactions);
+
+    $mail_messages = $this->buildMailWithRecipients(
+      $object,
+      $xactions,
+      array($recipient_phid),
+      array(),
+      array());
+    $mail = head($mail_messages);
+
+    // Since the user explicitly requested "!history", force delivery of this
+    // message regardless of their other mail settings.
+    $mail->setForceDelivery(true);
+
+    return $mail;
+  }
+
 }
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 69d11fe753..ed88ac98db 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -545,11 +545,18 @@ abstract class PhabricatorApplicationTransaction
       return false;
     }
 
+    $xaction_type = $this->getTransactionType();
+
+    // Always hide requests for object history.
+    if ($xaction_type === PhabricatorTransactions::TYPE_HISTORY) {
+      return true;
+    }
+
     // Hide creation transactions if the old value is empty. These are
-    // transactions like "alice set the task tile to: ...", which are
+    // transactions like "alice set the task title to: ...", which are
     // essentially never interesting.
     if ($this->getIsCreateTransaction()) {
-      switch ($this->getTransactionType()) {
+      switch ($xaction_type) {
         case PhabricatorTransactions::TYPE_CREATE:
         case PhabricatorTransactions::TYPE_VIEW_POLICY:
         case PhabricatorTransactions::TYPE_EDIT_POLICY:

From f9b3673fbbe114beb46989df3173d867c816cc5f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 16 Apr 2018 07:49:47 -0700
Subject: [PATCH 06/19] When mail (like "!history" mail) has multiple comments,
 label them separately

Summary:
Depends on D19372. Ref T13124. See PHI505. Currently, if you `!history` a task with a lot of comments, you get output like this:

> alice added a comment.
> bailey added a comment.
> alice added a comment.
> alice added a comment.
>
> AAAA
>
> BBBB
>
> AAAA
>
> AAAA

This is impossible to read. Put the "alice added a comment." headers above the actual comments for comments after the first.

These types of mail messages are unusual, but occur in several cases:

  - The new `!history` command.
  - Multiple comments on a draft revision before it promotes out of draft.
  - (Probably?) Conduit API updates which submit multiple comment transactions for some reason.

Test Plan: Used `bin/mail receive-test` to send a `!history` command to a task, saw a much more readable rendering of the transaction log in the resulting email.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19373
---
 ...habricatorApplicationTransactionEditor.php | 75 +++++++++++++++----
 1 file changed, 60 insertions(+), 15 deletions(-)

diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 685f760b15..f76f66df66 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -2969,34 +2969,62 @@ abstract class PhabricatorApplicationTransactionEditor
     $object_label = null,
     $object_href = null) {
 
+    // First, remove transactions which shouldn't be rendered in mail.
+    foreach ($xactions as $key => $xaction) {
+      if ($xaction->shouldHideForMail($xactions)) {
+        unset($xactions[$key]);
+      }
+    }
+
     $headers = array();
     $headers_html = array();
     $comments = array();
     $details = array();
 
+    $seen_comment = false;
     foreach ($xactions as $xaction) {
-      if ($xaction->shouldHideForMail($xactions)) {
-        continue;
-      }
 
-      $header = $xaction->getTitleForMail();
-      if ($header !== null) {
-        $headers[] = $header;
-      }
+      // Most mail has zero or one comments. In these cases, we render the
+      // "alice added a comment." transaction in the header, like a normal
+      // transaction.
 
-      $header_html = $xaction->getTitleForHTMLMail();
-      if ($header_html !== null) {
-        $headers_html[] = $header_html;
-      }
+      // Some mail, like Differential undraft mail or "!history" mail, may
+      // have two or more comments. In these cases, we'll put the first
+      // "alice added a comment." transaction in the header normally, but
+      // move the other transactions down so they provide context above the
+      // actual comment.
 
       $comment = $xaction->getBodyForMail();
       if ($comment !== null) {
-        $comments[] = $comment;
+        $is_comment = true;
+        $comments[] = array(
+          'xaction' => $xaction,
+          'comment' => $comment,
+          'initial' => !$seen_comment,
+        );
+      } else {
+        $is_comment = false;
+      }
+
+      if (!$is_comment || !$seen_comment) {
+        $header = $xaction->getTitleForMail();
+        if ($header !== null) {
+          $headers[] = $header;
+        }
+
+        $header_html = $xaction->getTitleForHTMLMail();
+        if ($header_html !== null) {
+          $headers_html[] = $header_html;
+        }
       }
 
       if ($xaction->hasChangeDetailsForMail()) {
         $details[] = $xaction;
       }
+
+      if ($is_comment) {
+        $seen_comment = true;
+      }
     }
 
     $headers_text = implode("\n", $headers);
@@ -3029,8 +3057,7 @@ abstract class PhabricatorApplicationTransactionEditor
         $object_label);
     }
 
-    $xactions_style = array(
-    );
+    $xactions_style = array();
 
     $header_action = phutil_tag(
       'td',
@@ -3057,7 +3084,25 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $body->addRawHTMLSection($headers_html);
 
-    foreach ($comments as $comment) {
+    foreach ($comments as $spec) {
+      $xaction = $spec['xaction'];
+      $comment = $spec['comment'];
+      $is_initial = $spec['initial'];
+
+      // If this is not the first comment in the mail, add the header showing
+      // who wrote the comment immediately above the comment.
+      if (!$is_initial) {
+        $header = $xaction->getTitleForMail();
+        if ($header !== null) {
+          $body->addRawPlaintextSection($header);
+        }
+
+        $header_html = $xaction->getTitleForHTMLMail();
+        if ($header_html !== null) {
+          $body->addRawHTMLSection($header_html);
+        }
+      }
+
       $body->addRemarkupSection(null, $comment);
     }
 

From 21bb0215dbc77ef58045ca45f1b0b1f9259326b2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 17 Apr 2018 05:53:31 -0700
Subject: [PATCH 07/19] Remove obsoleted "diffusion-browse-file" behavior for
 coverage

Summary: Ref T13105. After moving Diffusion to DocumnentEngine, this no longer has callers. It will become part of the document behavior.

Test Plan: Grepped for calls to the `diffusion-browse-file` behavior, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19377
---
 resources/celerity/map.php                    |  8 ----
 .../behavior-diffusion-browse-file.js         | 47 -------------------
 2 files changed, 55 deletions(-)
 delete mode 100644 webroot/rsrc/js/application/diffusion/behavior-diffusion-browse-file.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 6c1240fea0..49a4f233f7 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -384,7 +384,6 @@ return array(
     'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'd835b03a',
     'rsrc/js/application/diffusion/behavior-commit-branches.js' => 'bdaf4d04',
     'rsrc/js/application/diffusion/behavior-commit-graph.js' => '75b83cbb',
-    'rsrc/js/application/diffusion/behavior-diffusion-browse-file.js' => '054a0f0b',
     'rsrc/js/application/diffusion/behavior-locate-file.js' => '6d3e1947',
     'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc',
     'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70',
@@ -597,7 +596,6 @@ return array(
     'javelin-behavior-differential-feedback-preview' => '51c5ad07',
     'javelin-behavior-differential-populate' => '419998ab',
     'javelin-behavior-differential-user-select' => 'a8d8459d',
-    'javelin-behavior-diffusion-browse-file' => '054a0f0b',
     'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04',
     'javelin-behavior-diffusion-commit-graph' => '75b83cbb',
     'javelin-behavior-diffusion-locate-file' => '6d3e1947',
@@ -932,12 +930,6 @@ return array(
       'javelin-util',
       'javelin-magical-init',
     ),
-    '054a0f0b' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-util',
-      'phabricator-tooltip',
-    ),
     '065227cc' => array(
       'javelin-behavior',
       'javelin-dom',
diff --git a/webroot/rsrc/js/application/diffusion/behavior-diffusion-browse-file.js b/webroot/rsrc/js/application/diffusion/behavior-diffusion-browse-file.js
deleted file mode 100644
index 41ff0cfcc3..0000000000
--- a/webroot/rsrc/js/application/diffusion/behavior-diffusion-browse-file.js
+++ /dev/null
@@ -1,47 +0,0 @@
-/**
- * @provides javelin-behavior-diffusion-browse-file
- * @requires javelin-behavior
- *           javelin-dom
- *           javelin-util
- *           phabricator-tooltip
- */
-
-JX.behavior('diffusion-browse-file', function(config, statics) {
-  if (statics.installed) {
-    return;
-  }
-  statics.installed = true;
-
-  var map = config.labels;
-
-  JX.Stratcom.listen(
-    ['mouseover', 'mouseout'],
-    ['phabricator-source', 'tag:td'],
-    function(e) {
-      var target = e.getTarget();
-
-      // NOTE: We're using raw classnames instead of sigils and metadata here
-      // because these elements are unusual: there are a lot of them on the
-      // page, and rendering all the extra metadata to do this in a normal way
-      // would be needlessly expensive. This is an unusual case.
-
-      if (!target.className.match(/cov-/)) {
-        return;
-      }
-
-      if (e.getType() == 'mouseout') {
-        JX.Tooltip.hide();
-        return;
-      }
-
-      for (var k in map) {
-        if (!target.className.match(k)) {
-          continue;
-        }
-
-        var label = map[k];
-        JX.Tooltip.show(target, 300, 'E', label);
-        break;
-      }
-    });
-});

From 665529ab60a15a17574fc64865fe0764638938a6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 17 Apr 2018 06:40:41 -0700
Subject: [PATCH 08/19] Restore coverage reporting to Diffusion browse UI

Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.

This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.

Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:

{F5525542}

Hovered over coverage, got labels and highlighting.

Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.

Faked some multi-column coverage, but you can't currently get this yourself today:

{F5525544}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13125, T13124, T13105

Differential Revision: https://secure.phabricator.com/D19378
---
 resources/celerity/map.php                    | 18 +++---
 .../controller/DiffusionBrowseController.php  |  2 -
 .../DiffusionDocumentRenderingEngine.php      |  5 ++
 .../files/document/PhabricatorDocumentRef.php | 12 ++++
 .../PhabricatorSourceDocumentEngine.php       |  4 ++
 .../PhabricatorTextDocumentEngine.php         |  6 ++
 .../PhabricatorDocumentRenderingEngine.php    | 11 ++++
 src/view/layout/PhabricatorSourceCodeView.php | 45 +++++++++++++-
 .../layout/phabricator-source-code-view.css   | 12 +++-
 .../files/behavior-document-engine.js         | 59 ++++++++++++++++++-
 10 files changed, 159 insertions(+), 15 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 49a4f233f7..6053b0846e 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -119,7 +119,7 @@ return array(
     'rsrc/css/font/font-lato.css' => 'c7ccd872',
     'rsrc/css/font/phui-font-icon-base.css' => '870a7360',
     'rsrc/css/layout/phabricator-filetree-view.css' => 'b912ad97',
-    'rsrc/css/layout/phabricator-source-code-view.css' => '09368218',
+    'rsrc/css/layout/phabricator-source-code-view.css' => 'fdbefca0',
     'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494',
     'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68',
     'rsrc/css/phui/button/phui-button.css' => '1863cc6e',
@@ -388,7 +388,7 @@ return array(
     'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc',
     'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '1db13e70',
     'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef',
-    'rsrc/js/application/files/behavior-document-engine.js' => '0333c0b6',
+    'rsrc/js/application/files/behavior-document-engine.js' => 'ee0deff8',
     'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab',
     'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888',
     'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909',
@@ -600,7 +600,7 @@ return array(
     'javelin-behavior-diffusion-commit-graph' => '75b83cbb',
     'javelin-behavior-diffusion-locate-file' => '6d3e1947',
     'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc',
-    'javelin-behavior-document-engine' => '0333c0b6',
+    'javelin-behavior-document-engine' => 'ee0deff8',
     'javelin-behavior-doorkeeper-tag' => '1db13e70',
     'javelin-behavior-drydock-live-operation-status' => '901935ef',
     'javelin-behavior-durable-column' => '2ae077e1',
@@ -776,7 +776,7 @@ return array(
     'phabricator-search-results-css' => '505dd8cf',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-slowvote-css' => 'a94b7230',
-    'phabricator-source-code-view-css' => '09368218',
+    'phabricator-source-code-view-css' => 'fdbefca0',
     'phabricator-standard-page-view' => '34ee718b',
     'phabricator-textareautils' => '320810c8',
     'phabricator-title' => '485aaa6c',
@@ -905,11 +905,6 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
-    '0333c0b6' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-stratcom',
-    ),
     '040fce04' => array(
       'javelin-behavior',
       'javelin-request',
@@ -2110,6 +2105,11 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    'ee0deff8' => array(
+      'javelin-behavior',
+      'javelin-dom',
+      'javelin-stratcom',
+    ),
     'efe49472' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 1053ef477c..54be7dd7f1 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -4,7 +4,6 @@ final class DiffusionBrowseController extends DiffusionController {
 
   private $lintCommit;
   private $lintMessages;
-  private $coverage;
   private $corpusButtons = array();
 
   public function shouldAllowPublic() {
@@ -182,7 +181,6 @@ final class DiffusionBrowseController extends DiffusionController {
 
         $corpus = $this->buildGitLFSCorpus($lfs_ref);
       } else {
-        $this->coverage = $drequest->loadCoverage();
         $show_editor = true;
 
         $ref = id(new PhabricatorDocumentRef())
diff --git a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
index 17abba5c4f..9615b35799 100644
--- a/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
+++ b/src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php
@@ -81,6 +81,11 @@ final class DiffusionDocumentRenderingEngine
     $ref
       ->setSymbolMetadata($this->getSymbolMetadata())
       ->setBlameURI($blame_uri);
+
+    $coverage = $drequest->loadCoverage();
+    if (strlen($coverage)) {
+      $ref->addCoverage($coverage);
+    }
   }
 
   private function getSymbolMetadata() {
diff --git a/src/applications/files/document/PhabricatorDocumentRef.php b/src/applications/files/document/PhabricatorDocumentRef.php
index 953ac7df5d..38e4491615 100644
--- a/src/applications/files/document/PhabricatorDocumentRef.php
+++ b/src/applications/files/document/PhabricatorDocumentRef.php
@@ -10,6 +10,7 @@ final class PhabricatorDocumentRef
   private $snippet;
   private $symbolMetadata = array();
   private $blameURI;
+  private $coverage = array();
 
   public function setFile(PhabricatorFile $file) {
     $this->file = $file;
@@ -151,4 +152,15 @@ final class PhabricatorDocumentRef
     return $this->blameURI;
   }
 
+  public function addCoverage($coverage) {
+    $this->coverage[] = array(
+      'data' => $coverage,
+    );
+    return $this;
+  }
+
+  public function getCoverage() {
+    return $this->coverage;
+  }
+
 }
diff --git a/src/applications/files/document/PhabricatorSourceDocumentEngine.php b/src/applications/files/document/PhabricatorSourceDocumentEngine.php
index 20a1c94a95..c88d979b4e 100644
--- a/src/applications/files/document/PhabricatorSourceDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorSourceDocumentEngine.php
@@ -57,6 +57,10 @@ final class PhabricatorSourceDocumentEngine
       $options['blame'] = $blame;
     }
 
+    if ($ref->getCoverage()) {
+      $options['coverage'] = $ref->getCoverage();
+    }
+
     return array(
       $messages,
       $this->newTextDocumentContent($ref, $content, $options),
diff --git a/src/applications/files/document/PhabricatorTextDocumentEngine.php b/src/applications/files/document/PhabricatorTextDocumentEngine.php
index 0377a24353..2fce98baf1 100644
--- a/src/applications/files/document/PhabricatorTextDocumentEngine.php
+++ b/src/applications/files/document/PhabricatorTextDocumentEngine.php
@@ -22,6 +22,7 @@ abstract class PhabricatorTextDocumentEngine
       $options,
       array(
         'blame' => 'optional wild',
+        'coverage' => 'optional list<wild>',
       ));
 
     if (is_array($content)) {
@@ -40,6 +41,11 @@ abstract class PhabricatorTextDocumentEngine
       $view->setBlameMap($blame);
     }
 
+    $coverage = idx($options, 'coverage');
+    if ($coverage !== null) {
+      $view->setCoverage($coverage);
+    }
+
     $message = null;
     if ($this->encodingMessage !== null) {
       $message = $this->newMessage($this->encodingMessage);
diff --git a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
index 4a16f53fdf..155ed28dfa 100644
--- a/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
+++ b/src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
@@ -145,6 +145,17 @@ abstract class PhabricatorDocumentRenderingEngine
         'uri' => $ref->getBlameURI(),
         'value' => null,
       ),
+      'coverage' => array(
+        'labels' => array(
+          // TODO: Modularize this properly, see T13125.
+          array(
+            'C' => pht('Covered'),
+            'U' => pht('Not Covered'),
+            'N' => pht('Not Executable'),
+            'X' => pht('Not Reachable'),
+          ),
+        ),
+      ),
     );
 
     $view_button = id(new PHUIButtonView())
diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php
index 19126d1c6a..ae20fe2501 100644
--- a/src/view/layout/PhabricatorSourceCodeView.php
+++ b/src/view/layout/PhabricatorSourceCodeView.php
@@ -10,6 +10,7 @@ final class PhabricatorSourceCodeView extends AphrontView {
   private $truncatedFirstLines = false;
   private $symbolMetadata;
   private $blameMap;
+  private $coverage = array();
 
   public function setLines(array $lines) {
     $this->lines = $lines;
@@ -59,6 +60,15 @@ final class PhabricatorSourceCodeView extends AphrontView {
     return $this->blameMap;
   }
 
+  public function setCoverage(array $coverage) {
+    $this->coverage = $coverage;
+    return $this;
+  }
+
+  public function getCoverage() {
+    return $this->coverage;
+  }
+
   public function render() {
     $blame_map = $this->getBlameMap();
     $has_blame = ($blame_map !== null);
@@ -97,6 +107,19 @@ final class PhabricatorSourceCodeView extends AphrontView {
 
     $base_uri = (string)$this->uri;
     $wrote_anchor = false;
+
+    $coverage = $this->getCoverage();
+    $coverage_count = count($coverage);
+    $coverage_data = ipull($coverage, 'data');
+
+    // TODO: Modularize this properly, see T13125.
+    $coverage_map = array(
+      'C' => 'background: #66bbff;',
+      'U' => 'background: #dd8866;',
+      'N' => 'background: #ddeeff;',
+      'X' => 'background: #aa00aa;',
+    );
+
     foreach ($lines as $line) {
       $row_attributes = array();
       if (isset($this->highlights[$line_number])) {
@@ -157,6 +180,25 @@ final class PhabricatorSourceCodeView extends AphrontView {
         $blame_cells = null;
       }
 
+      $coverage_cells = array();
+      foreach ($coverage as $coverage_idx => $coverage_spec) {
+        if (isset($coverage_spec['data'][$line_number - 1])) {
+          $coverage_char = $coverage_spec['data'][$line_number - 1];
+        } else {
+          $coverage_char = null;
+        }
+
+        $coverage_style = idx($coverage_map, $coverage_char, null);
+
+        $coverage_cells[] = phutil_tag(
+          'th',
+          array(
+            'class' => 'phabricator-source-coverage',
+            'style' => $coverage_style,
+            'data-coverage' => $coverage_idx.'/'.$coverage_char,
+          ));
+      }
+
       $rows[] = phutil_tag(
         'tr',
         $row_attributes,
@@ -174,7 +216,8 @@ final class PhabricatorSourceCodeView extends AphrontView {
               'class' => 'phabricator-source-code',
             ),
             $line),
-          ));
+          $coverage_cells,
+        ));
 
       $line_number++;
     }
diff --git a/webroot/rsrc/css/layout/phabricator-source-code-view.css b/webroot/rsrc/css/layout/phabricator-source-code-view.css
index 6f2864d067..b0c334da26 100644
--- a/webroot/rsrc/css/layout/phabricator-source-code-view.css
+++ b/webroot/rsrc/css/layout/phabricator-source-code-view.css
@@ -6,7 +6,6 @@
   overflow-x: auto;
   overflow-y: hidden;
   border: 1px solid {$paste.border};
-  border-radius: 3px;
 }
 
 .phui-oi .phabricator-source-code-container {
@@ -47,10 +46,12 @@ th.phabricator-source-line a:hover {
   text-decoration: none;
 }
 
+.phabricator-source-coverage-highlight .phabricator-source-code,
 .phabricator-source-highlight .phabricator-source-code {
   background: {$paste.highlight};
 }
 
+.phabricator-source-coverage-highlight .phabricator-source-line,
 .phabricator-source-highlight .phabricator-source-line {
   background: {$paste.border};
 }
@@ -96,7 +97,7 @@ th.phabricator-source-line a:hover {
 
 .phabricator-source-blame-info a {
   color: {$darkbluetext};
-  text-shadow: 1px 1px rgba(0, 0, 0, 0.111);
+  text-shadow: 1px 1px rgba(0, 0, 0, 0.05);
 }
 
 .phabricator-source-blame-skip a {
@@ -123,3 +124,10 @@ th.phabricator-source-line a:hover {
   background-size: 100% 100%;
   background-repeat: no-repeat;
 }
+
+th.phabricator-source-coverage {
+  padding: 0 8px;
+  border-left: 1px solid {$thinblueborder};
+  background: {$lightgreybackground};
+  cursor: w-resize;
+}
diff --git a/webroot/rsrc/js/application/files/behavior-document-engine.js b/webroot/rsrc/js/application/files/behavior-document-engine.js
index 2fc4887408..8b957e0ea0 100644
--- a/webroot/rsrc/js/application/files/behavior-document-engine.js
+++ b/webroot/rsrc/js/application/files/behavior-document-engine.js
@@ -322,7 +322,7 @@ JX.behavior('document-engine', function(config, statics) {
     var h_max = 0.44;
     var h = h_min + ((h_max - h_min) * epoch_value);
 
-    var s = 0.44;
+    var s = 0.25;
 
     var v_min = 0.92;
     var v_max = 1.00;
@@ -357,6 +357,57 @@ JX.behavior('document-engine', function(config, statics) {
     return 'rgb(' + r + ', ' + g + ', ' + b + ')';
   }
 
+  function onhovercoverage(data, e) {
+    if (e.getType() === 'mouseout') {
+      redraw_coverage(data, null);
+      return;
+    }
+
+    var target = e.getNode('tag:th');
+    var coverage = target.getAttribute('data-coverage');
+    if (!coverage) {
+      return;
+    }
+
+    redraw_coverage(data, target);
+  }
+
+  var coverage_row = null;
+  function redraw_coverage(data, node) {
+    if (coverage_row) {
+      JX.DOM.alterClass(
+        coverage_row,
+        'phabricator-source-coverage-highlight',
+        false);
+      coverage_row = null;
+    }
+
+    if (!node) {
+      JX.Tooltip.hide();
+      return;
+    }
+
+    var coverage = node.getAttribute('data-coverage');
+    coverage = coverage.split('/');
+
+    var idx = parseInt(coverage[0], 10);
+    var chr = coverage[1];
+
+    var map = data.coverage.labels[idx];
+    if (map) {
+      var label = map[chr];
+      if (label) {
+        JX.Tooltip.show(node, 300, 'W', label);
+
+        coverage_row = JX.DOM.findAbove(node, 'tr');
+        JX.DOM.alterClass(
+          coverage_row,
+          'phabricator-source-coverage-highlight',
+          true);
+      }
+    }
+  }
+
   if (!statics.initialized) {
     JX.Stratcom.listen('click', 'document-engine-view-dropdown', onmenu);
     statics.initialized = true;
@@ -374,6 +425,12 @@ JX.behavior('document-engine', function(config, statics) {
         blame(data);
         break;
     }
+
+    JX.DOM.listen(
+      JX.$(data.viewportID),
+      ['mouseover', 'mouseout'],
+      'tag:th',
+      JX.bind(null, onhovercoverage, data));
   }
 
 });

From a817aa6c71716d0603d1ae2ab3a1a59d7b77c87f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 16 Apr 2018 09:18:36 -0700
Subject: [PATCH 09/19] Add an "Abort Older Builds" build step to Harbormaster

Summary:
Ref T13124. See PHI531. When a revision is updated, builds against the older diff tend to stop being relevant. Add an option to abort outstanding older builds automatically.

At least for now, I'm adding this as a build step instead of some kind of special checkbox. An alternate implementation would be some kind of "Edit Options" action on plans with a checkbox like `[X] When this build starts, abort older builds.`

I think adding it as a build step is a bit simpler, and likely to lead to greater consistency and flexibility down the road, make it easier to add options, etc., and since we don't really have any other current use cases for "a bunch of checkboxes". This might change eventually if we add a bunch of checkboxes for some other reason.

The actual step activates //before// the build queues, so it doesn't need to wait in queue before it can actually act. T13088 discusses some plans here if this sticks.

Test Plan:
  - Created a "Sleep for 120 seconds" build plan and triggered it with Herald.
  - Added an "Abort Older Builds" step.
  - Updated a revision several times in a row, saw older builds abort.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19376
---
 src/__phutil_library_map__.php                |   4 +
 .../constants/HarbormasterBuildStatus.php     |  13 ++
 .../HarbormasterBuildActionController.php     |  13 +-
 .../query/HarbormasterBuildStepQuery.php      |  39 ++---
 ...bortOlderBuildsBuildStepImplementation.php | 135 ++++++++++++++++++
 .../HarbormasterBuildStepImplementation.php   |   9 ++
 .../HarbormasterControlBuildStepGroup.php     |  20 +++
 .../storage/HarbormasterBuildable.php         |   9 ++
 .../storage/build/HarbormasterBuild.php       |  29 ++++
 .../configuration/HarbormasterBuildStep.php   |  13 ++
 10 files changed, 248 insertions(+), 36 deletions(-)
 create mode 100644 src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php
 create mode 100644 src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index da5674cbbd..9eda923457 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1263,6 +1263,7 @@ phutil_register_library_map(array(
     'FundInitiativeTransactionType' => 'applications/fund/xaction/FundInitiativeTransactionType.php',
     'FundInitiativeViewController' => 'applications/fund/controller/FundInitiativeViewController.php',
     'FundSchemaSpec' => 'applications/fund/storage/FundSchemaSpec.php',
+    'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php',
     'HarbormasterArcLintBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php',
     'HarbormasterArcUnitBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php',
     'HarbormasterArtifact' => 'applications/harbormaster/artifact/HarbormasterArtifact.php',
@@ -1358,6 +1359,7 @@ phutil_register_library_map(array(
     'HarbormasterCircleCIBuildableInterface' => 'applications/harbormaster/interface/HarbormasterCircleCIBuildableInterface.php',
     'HarbormasterCircleCIHookController' => 'applications/harbormaster/controller/HarbormasterCircleCIHookController.php',
     'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php',
+    'HarbormasterControlBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php',
     'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php',
     'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php',
@@ -6636,6 +6638,7 @@ phutil_register_library_map(array(
     'FundInitiativeTransactionType' => 'PhabricatorModularTransactionType',
     'FundInitiativeViewController' => 'FundController',
     'FundSchemaSpec' => 'PhabricatorConfigSchemaSpec',
+    'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArcLintBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArcUnitBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterArtifact' => 'Phobject',
@@ -6771,6 +6774,7 @@ phutil_register_library_map(array(
     'HarbormasterCircleCIBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterCircleCIHookController' => 'HarbormasterController',
     'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod',
+    'HarbormasterControlBuildStepGroup' => 'HarbormasterBuildStepGroup',
     'HarbormasterController' => 'PhabricatorController',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod',
     'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability',
diff --git a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php
index dc63b5fd95..7437e48f6c 100644
--- a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php
+++ b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php
@@ -98,6 +98,19 @@ final class HarbormasterBuildStatus extends Phobject {
     );
   }
 
+  public static function getIncompleteStatusConstants() {
+    $map = self::getBuildStatusSpecMap();
+
+    $constants = array();
+    foreach ($map as $constant => $spec) {
+      if (!$spec['isComplete']) {
+        $constants[] = $constant;
+      }
+    }
+
+    return $constants;
+  }
+
   public static function getCompletedStatusConstants() {
     return array(
       self::STATUS_PASSED,
diff --git a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php
index ddab677faf..843ffd4702 100644
--- a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php
+++ b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php
@@ -51,18 +51,7 @@ final class HarbormasterBuildActionController
     }
 
     if ($request->isDialogFormPost() && $can_issue) {
-      $editor = id(new HarbormasterBuildTransactionEditor())
-        ->setActor($viewer)
-        ->setContentSourceFromRequest($request)
-        ->setContinueOnNoEffect(true)
-        ->setContinueOnMissingFields(true);
-
-      $xaction = id(new HarbormasterBuildTransaction())
-        ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
-        ->setNewValue($action);
-
-      $editor->applyTransactions($build, array($xaction));
-
+      $build->sendMessage($viewer, $action);
       return id(new AphrontRedirectResponse())->setURI($return_uri);
     }
 
diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php
index c14185116c..992dd4fad1 100644
--- a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php
+++ b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php
@@ -22,48 +22,39 @@ final class HarbormasterBuildStepQuery
     return $this;
   }
 
-  protected function loadPage() {
-    $table = new HarbormasterBuildStep();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+  public function newResultObject() {
+    return new HarbormasterBuildStep();
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
-    $where = array();
+  protected function loadPage() {
+    return $this->loadStandardPage($this->newResultObject());
+  }
 
-    if ($this->ids) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+
+    if ($this->ids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'phid in (%Ls)',
         $this->phids);
     }
 
-    if ($this->buildPlanPHIDs) {
+    if ($this->buildPlanPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'buildPlanPHID in (%Ls)',
         $this->buildPlanPHIDs);
     }
 
-    $where[] = $this->buildPagingClause($conn_r);
-
-    return $this->formatWhereClause($where);
+    return $where;
   }
 
   protected function willFilterPage(array $page) {
diff --git a/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php
new file mode 100644
index 0000000000..d6c2a46734
--- /dev/null
+++ b/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php
@@ -0,0 +1,135 @@
+<?php
+
+final class HarbormasterAbortOlderBuildsBuildStepImplementation
+  extends HarbormasterBuildStepImplementation {
+
+  public function getName() {
+    return pht('Abort Older Builds');
+  }
+
+  public function getGenericDescription() {
+    return pht(
+      'When building a revision, abort copies of this build plan which are '.
+      'currently running against older diffs.');
+  }
+
+  public function getBuildStepGroupKey() {
+    return HarbormasterControlBuildStepGroup::GROUPKEY;
+  }
+
+  public function getEditInstructions() {
+    return pht(<<<EOTEXT
+When run against a revision, this build step will abort any older copies of
+the same build plan which are currently running against older diffs.
+
+There are some nuances to the behavior:
+
+  - if this build step is triggered manually, it won't abort anything;
+  - this build step won't abort manual builds;
+  - this build step won't abort anything if the diff it is building isn't
+    the active diff when it runs.
+
+Build results on outdated diffs often aren't very important, so this may
+reduce build queue load without any substantial cost.
+EOTEXT
+      );
+  }
+
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan,
+    HarbormasterBuildStep $step) {
+
+    if ($buildable->getIsManualBuildable()) {
+      // Don't abort anything if this is a manual buildable.
+      return;
+    }
+
+    $object_phid = $buildable->getBuildablePHID();
+    if (phid_get_type($object_phid) !== DifferentialDiffPHIDType::TYPECONST) {
+      // If this buildable isn't building a diff, bail out. For example, we
+      // might be building a commit. In this case, this step has no effect.
+      return;
+    }
+
+    $diff = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($object_phid))
+      ->executeOne();
+    if (!$diff) {
+      return;
+    }
+
+    $revision_id = $diff->getRevisionID();
+
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($revision_id))
+      ->executeOne();
+    if (!$revision) {
+      return;
+    }
+
+    $active_phid = $revision->getActiveDiffPHID();
+    if ($active_phid !== $object_phid) {
+      // If we aren't building the active diff, bail out.
+      return;
+    }
+
+    $diffs = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withRevisionIDs(array($revision_id))
+      ->execute();
+    $abort_diff_phids = array();
+    foreach ($diffs as $diff) {
+      if ($diff->getPHID() !== $active_phid) {
+        $abort_diff_phids[] = $diff->getPHID();
+      }
+    }
+
+    if (!$abort_diff_phids) {
+      return;
+    }
+
+    // We're fetching buildables even if they have "passed" or "failed"
+    // because they may still have ongoing builds. At the time of writing
+    // only "failed" buildables may still be ongoing, but it seems likely that
+    // "passed" buildables may be ongoing in the future.
+
+    $abort_buildables = id(new HarbormasterBuildableQuery())
+      ->setViewer($viewer)
+      ->withBuildablePHIDs($abort_diff_phids)
+      ->withManualBuildables(false)
+      ->execute();
+    if (!$abort_buildables) {
+      return;
+    }
+
+    $statuses = HarbormasterBuildStatus::getIncompleteStatusConstants();
+
+    $abort_builds = id(new HarbormasterBuildQuery())
+      ->setViewer($viewer)
+      ->withBuildablePHIDs(mpull($abort_buildables, 'getPHID'))
+      ->withBuildPlanPHIDs(array($plan->getPHID()))
+      ->withBuildStatuses($statuses)
+      ->execute();
+    if (!$abort_builds) {
+      return;
+    }
+
+    foreach ($abort_builds as $abort_build) {
+      $abort_build->sendMessage(
+        $viewer,
+        HarbormasterBuildCommand::COMMAND_ABORT);
+    }
+  }
+
+  public function execute(
+    HarbormasterBuild $build,
+    HarbormasterBuildTarget $build_target) {
+    return;
+  }
+
+}
diff --git a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
index 47af992630..ccf97451b7 100644
--- a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
+++ b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
@@ -308,6 +308,15 @@ abstract class HarbormasterBuildStepImplementation extends Phobject {
           'enabled in configuration.'));
   }
 
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan,
+    HarbormasterBuildStep $step) {
+    return;
+  }
+
 
 /* -(  Automatic Targets  )-------------------------------------------------- */
 
diff --git a/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php b/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php
new file mode 100644
index 0000000000..f49fd0b127
--- /dev/null
+++ b/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php
@@ -0,0 +1,20 @@
+<?php
+
+final class HarbormasterControlBuildStepGroup
+  extends HarbormasterBuildStepGroup {
+
+  const GROUPKEY = 'harbormaster.control';
+
+  public function getGroupName() {
+    return pht('Flow Control');
+  }
+
+  public function getGroupOrder() {
+    return 5000;
+  }
+
+  public function shouldShowIfEmpty() {
+    return false;
+  }
+
+}
diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php
index 7d4f2e7c83..b24c4805e2 100644
--- a/src/applications/harbormaster/storage/HarbormasterBuildable.php
+++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php
@@ -141,6 +141,15 @@ final class HarbormasterBuildable
 
     $build->save();
 
+    $steps = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withBuildPlanPHIDs(array($plan->getPHID()))
+      ->execute();
+
+    foreach ($steps as $step) {
+      $step->willStartBuild($viewer, $this, $build, $plan);
+    }
+
     PhabricatorWorker::scheduleTask(
       'HarbormasterBuildWorker',
       array(
diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
index ae0f6b13f3..6acbac6468 100644
--- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
@@ -356,6 +356,35 @@ final class HarbormasterBuild extends HarbormasterDAO
     }
   }
 
+  public function sendMessage(PhabricatorUser $viewer, $command) {
+    // TODO: This should not be an editor transaction, but there are plans to
+    // merge BuildCommand into BuildMessage which should moot this. As this
+    // exists today, it can race against BuildEngine.
+
+    // This is a bogus content source, but this whole flow should be obsolete
+    // soon.
+    $content_source = PhabricatorContentSource::newForSource(
+      PhabricatorConsoleContentSource::SOURCECONST);
+
+    $editor = id(new HarbormasterBuildTransactionEditor())
+      ->setActor($viewer)
+      ->setContentSource($content_source)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true);
+
+    $viewer_phid = $viewer->getPHID();
+    if (!$viewer_phid) {
+      $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID();
+      $editor->setActingAsPHID($acting_phid);
+    }
+
+    $xaction = id(new HarbormasterBuildTransaction())
+      ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND)
+      ->setNewValue($command);
+
+    $editor->applyTransactions($this, array($xaction));
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
index b29958882c..54c069e97d 100644
--- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
+++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
@@ -100,6 +100,19 @@ final class HarbormasterBuildStep extends HarbormasterDAO
     return ($this->getStepAutoKey() !== null);
   }
 
+  public function willStartBuild(
+    PhabricatorUser $viewer,
+    HarbormasterBuildable $buildable,
+    HarbormasterBuild $build,
+    HarbormasterBuildPlan $plan) {
+    return $this->getStepImplementation()->willStartBuild(
+      $viewer,
+      $buildable,
+      $build,
+      $plan,
+      $this);
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

From 0a83f253ed03829826a2e69e891358e80a3c3ba7 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Wed, 18 Apr 2018 11:45:50 -0700
Subject: [PATCH 10/19] Add unique constraint for Almanac network names

Summary:
The name of networks should be unique.

Also adds support for exact-name queries for AlamanacNetworks.

Test Plan: Applied migration with existing duplicates, saw networks renamed, attempted to add duplicates, got a nice error message.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19379
---
 .../20180418.almanac.network.unique.php       | 46 +++++++++++++++++++
 .../almanac/query/AlmanacNetworkQuery.php     | 13 ++++++
 .../almanac/storage/AlmanacNetwork.php        |  9 +++-
 .../almanac/util/AlmanacNames.php             | 33 ++++++-------
 .../xaction/AlmanacNetworkNameTransaction.php | 31 +++++++++++++
 5 files changed, 115 insertions(+), 17 deletions(-)
 create mode 100644 resources/sql/autopatches/20180418.almanac.network.unique.php

diff --git a/resources/sql/autopatches/20180418.almanac.network.unique.php b/resources/sql/autopatches/20180418.almanac.network.unique.php
new file mode 100644
index 0000000000..c81c59823e
--- /dev/null
+++ b/resources/sql/autopatches/20180418.almanac.network.unique.php
@@ -0,0 +1,46 @@
+<?php
+
+$table = new AlmanacNetwork();
+$conn = $table->establishConnection('w');
+
+queryfx(
+  $conn,
+  'LOCK TABLES %T WRITE',
+  $table->getTableName());
+
+$seen = array();
+foreach (new LiskMigrationIterator($table) as $network) {
+  $name = $network->getName();
+
+  // If this is the first copy of this row we've seen, mark it as seen and
+  // move on.
+  if (empty($seen[$name])) {
+    $seen[$name] = 1;
+    continue;
+  }
+
+  // Otherwise, rename this row.
+  while (true) {
+    $new_name = $name.'-'.$seen[$name];
+    if (empty($seen[$new_name])) {
+      $network->setName($new_name);
+      try {
+        $network->save();
+        break;
+      } catch (AphrontDuplicateKeyQueryException $ex) {
+        // New name is a dupe of a network we haven't seen yet.
+      }
+    }
+    $seen[$name]++;
+  }
+  $seen[$new_name] = 1;
+}
+
+queryfx(
+  $conn,
+  'ALTER TABLE %T ADD UNIQUE KEY `key_name` (name)',
+  $table->getTableName());
+
+queryfx(
+  $conn,
+  'UNLOCK TABLES');
diff --git a/src/applications/almanac/query/AlmanacNetworkQuery.php b/src/applications/almanac/query/AlmanacNetworkQuery.php
index a09da0093b..13176f7de1 100644
--- a/src/applications/almanac/query/AlmanacNetworkQuery.php
+++ b/src/applications/almanac/query/AlmanacNetworkQuery.php
@@ -5,6 +5,7 @@ final class AlmanacNetworkQuery
 
   private $ids;
   private $phids;
+  private $names;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -20,6 +21,11 @@ final class AlmanacNetworkQuery
     return new AlmanacNetwork();
   }
 
+  public function withNames(array $names) {
+    $this->names = $names;
+    return $this;
+  }
+
   public function withNameNgrams($ngrams) {
     return $this->withNgramsConstraint(
       new AlmanacNetworkNameNgrams(),
@@ -47,6 +53,13 @@ final class AlmanacNetworkQuery
         $this->phids);
     }
 
+    if ($this->names !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'network.name IN (%Ls)',
+        $this->names);
+    }
+
     return $where;
   }
 
diff --git a/src/applications/almanac/storage/AlmanacNetwork.php b/src/applications/almanac/storage/AlmanacNetwork.php
index 6a530b4cb3..6d2f23032e 100644
--- a/src/applications/almanac/storage/AlmanacNetwork.php
+++ b/src/applications/almanac/storage/AlmanacNetwork.php
@@ -24,8 +24,15 @@ final class AlmanacNetwork
     return array(
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_COLUMN_SCHEMA => array(
-        'name' => 'text128',
+        'name' => 'sort128',
         'mailKey' => 'bytes20',
+
+      ),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_name' => array(
+            'columns' => array('name'),
+            'unique' => true,
+          ),
       ),
     ) + parent::getConfiguration();
   }
diff --git a/src/applications/almanac/util/AlmanacNames.php b/src/applications/almanac/util/AlmanacNames.php
index 68a387cb9b..cee3c81151 100644
--- a/src/applications/almanac/util/AlmanacNames.php
+++ b/src/applications/almanac/util/AlmanacNames.php
@@ -6,57 +6,58 @@ final class AlmanacNames extends Phobject {
     if (strlen($name) < 3) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names must be '.
-          'at least 3 characters long.'));
+          'Almanac service, device, property, network and namespace names '.
+          'must be at least 3 characters long.'));
     }
 
     if (strlen($name) > 100) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may not '.
-          'be more than 100 characters long.'));
+          'Almanac service, device, property, network and namespace names '.
+          'may not be more than 100 characters long.'));
     }
 
     if (!preg_match('/^[a-z0-9.-]+\z/', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may only '.
-          'contain lowercase letters, numbers, hyphens, and periods.'));
+          'Almanac service, device, property, network and namespace names '.
+          'may only contain lowercase letters, numbers, hyphens, and '.
+          'periods.'));
     }
 
     if (preg_match('/(^|\\.)\d+(\z|\\.)/', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may not '.
-          'have any segments containing only digits.'));
+          'Almanac service, device, network, property and namespace names '.
+          'may not have any segments containing only digits.'));
     }
 
     if (preg_match('/\.\./', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may not '.
-          'contain multiple consecutive periods.'));
+          'Almanac service, device, property, network and namespace names '.
+          'may not contain multiple consecutive periods.'));
     }
 
     if (preg_match('/\\.-|-\\./', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may not '.
-          'contain hyphens adjacent to periods.'));
+          'Almanac service, device, property, network and namespace names '.
+          'may not contain hyphens adjacent to periods.'));
     }
 
     if (preg_match('/--/', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names may not '.
-          'contain multiple consecutive hyphens.'));
+          'Almanac service, device, property, network and namespace names '.
+          'may not contain multiple consecutive hyphens.'));
     }
 
     if (!preg_match('/^[a-z0-9].*[a-z0-9]\z/', $name)) {
       throw new Exception(
         pht(
-          'Almanac service, device, property and namespace names must begin '.
-          'and end with a letter or number.'));
+          'Almanac service, device, property, network and namespace names '.
+          'must begin and end with a letter or number.'));
     }
   }
 
diff --git a/src/applications/almanac/xaction/AlmanacNetworkNameTransaction.php b/src/applications/almanac/xaction/AlmanacNetworkNameTransaction.php
index 140c1a9661..f049587b26 100644
--- a/src/applications/almanac/xaction/AlmanacNetworkNameTransaction.php
+++ b/src/applications/almanac/xaction/AlmanacNetworkNameTransaction.php
@@ -38,6 +38,37 @@ final class AlmanacNetworkNameTransaction
         pht('Network name is required.'));
     }
 
+    foreach ($xactions as $xaction) {
+      $name = $xaction->getNewValue();
+
+      $message = null;
+      try {
+        AlmanacNames::validateName($name);
+      } catch (Exception $ex) {
+        $message = $ex->getMessage();
+      }
+
+      if ($message !== null) {
+        $errors[] = $this->newInvalidError($message, $xaction);
+        continue;
+      }
+
+      if ($name === $object->getName()) {
+        continue;
+      }
+
+      $other = id(new AlmanacNetworkQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withNames(array($name))
+        ->executeOne();
+      if ($other && ($other->getID() != $object->getID())) {
+        $errors[] = $this->newInvalidError(
+          pht('Almanac networks must have unique names.'),
+          $xaction);
+        continue;
+      }
+    }
+
     return $errors;
   }
 

From e81b2173ad08b7035e5e1a92247c11cca5721ccc Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Thu, 19 Apr 2018 13:38:37 -0700
Subject: [PATCH 11/19] Add edge tables for Phlux

Summary: Fixes T13129. This at least makes the existing UI work again before we banish Phlux to the shadow realm.

Test Plan: Edited the visibility for a Phlux variable, didn't get an error. Nothing showed up in the edge tables when I made those changes, but at least it doesn't error out anymore.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13129

Differential Revision: https://secure.phabricator.com/D19387
---
 .../sql/autopatches/20180419.phlux.edges.sql     | 16 ++++++++++++++++
 src/__phutil_library_map__.php                   |  2 ++
 .../phlux/storage/PhluxSchemaSpec.php            | 10 ++++++++++
 3 files changed, 28 insertions(+)
 create mode 100644 resources/sql/autopatches/20180419.phlux.edges.sql
 create mode 100644 src/applications/phlux/storage/PhluxSchemaSpec.php

diff --git a/resources/sql/autopatches/20180419.phlux.edges.sql b/resources/sql/autopatches/20180419.phlux.edges.sql
new file mode 100644
index 0000000000..1a63aa4d1f
--- /dev/null
+++ b/resources/sql/autopatches/20180419.phlux.edges.sql
@@ -0,0 +1,16 @@
+CREATE TABLE {$NAMESPACE}_phlux.edge (
+  src VARBINARY(64) NOT NULL,
+  type INT UNSIGNED NOT NULL,
+  dst VARBINARY(64) NOT NULL,
+  dateCreated INT UNSIGNED NOT NULL,
+  seq INT UNSIGNED NOT NULL,
+  dataID INT UNSIGNED,
+  PRIMARY KEY (src, type, dst),
+  KEY `src` (src, type, dateCreated, seq),
+  UNIQUE KEY `key_dst` (dst, type, src)
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
+
+CREATE TABLE {$NAMESPACE}_phlux.edgedata (
+  id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT,
+  data LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9eda923457..5f1543aa91 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4716,6 +4716,7 @@ phutil_register_library_map(array(
     'PhluxDAO' => 'applications/phlux/storage/PhluxDAO.php',
     'PhluxEditController' => 'applications/phlux/controller/PhluxEditController.php',
     'PhluxListController' => 'applications/phlux/controller/PhluxListController.php',
+    'PhluxSchemaSpec' => 'applications/phlux/storage/PhluxSchemaSpec.php',
     'PhluxTransaction' => 'applications/phlux/storage/PhluxTransaction.php',
     'PhluxTransactionQuery' => 'applications/phlux/query/PhluxTransactionQuery.php',
     'PhluxVariable' => 'applications/phlux/storage/PhluxVariable.php',
@@ -10693,6 +10694,7 @@ phutil_register_library_map(array(
     'PhluxDAO' => 'PhabricatorLiskDAO',
     'PhluxEditController' => 'PhluxController',
     'PhluxListController' => 'PhluxController',
+    'PhluxSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'PhluxTransaction' => 'PhabricatorApplicationTransaction',
     'PhluxTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhluxVariable' => array(
diff --git a/src/applications/phlux/storage/PhluxSchemaSpec.php b/src/applications/phlux/storage/PhluxSchemaSpec.php
new file mode 100644
index 0000000000..ee8a38b6f5
--- /dev/null
+++ b/src/applications/phlux/storage/PhluxSchemaSpec.php
@@ -0,0 +1,10 @@
+<?php
+
+final class PhluxSchemaSpec
+  extends PhabricatorConfigSchemaSpec {
+
+  public function buildSchemata() {
+    $this->buildEdgeSchemata(new PhluxVariable());
+  }
+
+}

From 70d67a3908b8dc32ec122bbc22a3629882957f5a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 19 Apr 2018 10:49:30 -0700
Subject: [PATCH 12/19] Fix the most significant "phantom notification" badness

Summary:
Ref T13124. Ref T13131. Fixes T8953. See PHI512.

When you receieve a notification about an object and then someone hides that object from you (or deletes it), you get a phantom notification which is very difficult to clear.

For now, test that notifications are visible when you open the menu and clear any that are not.

This could be a little more elegant than it is, but the current behavior is very clearly broken. This unbreaks it, at least.

Test Plan:
  - As Alice, configured task stuff to notify me (instead of sending email).
  - As Bailey, added Alice as a subscriber to a task, then commented on it.
  - As Alice, loaded home and saw a notification count. Didn't click it yet.
  - As Bailey, set the task to private.
  - As Alice, clicked the notification bell menu icon.
    - Before change: no unread notifications, bell menu is semi-stuck in a phantom state which you can't clear.
    - After change: bad notifications automatically cleared.

{F5530005}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13131, T13124, T8953

Differential Revision: https://secure.phabricator.com/D19384
---
 resources/celerity/map.php                    |  6 +-
 ...PhabricatorNotificationPanelController.php | 86 ++++++++++++++++++-
 .../query/PhabricatorNotificationQuery.php    | 12 +--
 .../PhabricatorUSEnglishTranslation.php       |  8 ++
 .../application/base/notification-menu.css    |  6 +-
 5 files changed, 105 insertions(+), 13 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 6053b0846e..dc018326bf 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
-    'core.pkg.css' => '39061f68',
+    'core.pkg.css' => 'c30f6eaa',
     'core.pkg.js' => 'e1f0f7bd',
     'differential.pkg.css' => '06dc617c',
     'differential.pkg.js' => 'c2ca903a',
@@ -38,7 +38,7 @@ return array(
     'rsrc/css/application/almanac/almanac.css' => 'dbb9b3af',
     'rsrc/css/application/auth/auth.css' => '0877ed6e',
     'rsrc/css/application/base/main-menu-view.css' => '1802a242',
-    'rsrc/css/application/base/notification-menu.css' => '10685bd4',
+    'rsrc/css/application/base/notification-menu.css' => 'ef480927',
     'rsrc/css/application/base/phui-theme.css' => '9f261c6b',
     'rsrc/css/application/base/standard-page-view.css' => '34ee718b',
     'rsrc/css/application/chatlog/chatlog.css' => 'd295b020',
@@ -768,7 +768,7 @@ return array(
     'phabricator-nav-view-css' => '694d7723',
     'phabricator-notification' => '4f774dac',
     'phabricator-notification-css' => '457861ec',
-    'phabricator-notification-menu-css' => '10685bd4',
+    'phabricator-notification-menu-css' => 'ef480927',
     'phabricator-object-selector-css' => '85ee8ce6',
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => '77b0ae28',
diff --git a/src/applications/notification/controller/PhabricatorNotificationPanelController.php b/src/applications/notification/controller/PhabricatorNotificationPanelController.php
index 3e30ead9e7..1e956a60ea 100644
--- a/src/applications/notification/controller/PhabricatorNotificationPanelController.php
+++ b/src/applications/notification/controller/PhabricatorNotificationPanelController.php
@@ -6,6 +6,10 @@ final class PhabricatorNotificationPanelController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $request->getViewer();
 
+    $unread_count = $viewer->getUnreadNotificationCount();
+
+    $warning = $this->prunePhantomNotifications($unread_count);
+
     $query = id(new PhabricatorNotificationQuery())
       ->setViewer($viewer)
       ->withUserPHIDs(array($viewer->getPHID()))
@@ -66,13 +70,12 @@ final class PhabricatorNotificationPanelController
       ));
 
     $content = hsprintf(
-      '%s%s%s',
+      '%s%s%s%s',
       $header,
+      $warning,
       $content,
       $connection_ui);
 
-    $unread_count = $viewer->getUnreadNotificationCount();
-
     $json = array(
       'content' => $content,
       'number'  => (int)$unread_count,
@@ -80,4 +83,81 @@ final class PhabricatorNotificationPanelController
 
     return id(new AphrontAjaxResponse())->setContent($json);
   }
+
+  private function prunePhantomNotifications($unread_count) {
+    // See T8953. If you have an unread notification about an object you
+    // do not have permission to view, it isn't possible to clear it by
+    // visiting the object. Identify these notifications and mark them as
+    // read.
+
+    $viewer = $this->getViewer();
+
+    if (!$unread_count) {
+      return null;
+    }
+
+    $table = new PhabricatorFeedStoryNotification();
+    $conn = $table->establishConnection('r');
+
+    $rows = queryfx_all(
+      $conn,
+      'SELECT chronologicalKey, primaryObjectPHID FROM %T
+        WHERE userPHID = %s AND hasViewed = 0',
+      $table->getTableName(),
+      $viewer->getPHID());
+    if (!$rows) {
+      return null;
+    }
+
+    $map = array();
+    foreach ($rows as $row) {
+      $map[$row['primaryObjectPHID']][] = $row['chronologicalKey'];
+    }
+
+    $handles = $viewer->loadHandles(array_keys($map));
+    $purge_keys = array();
+    foreach ($handles as $handle) {
+      $phid = $handle->getPHID();
+      if ($handle->isComplete()) {
+        continue;
+      }
+
+      foreach ($map[$phid] as $chronological_key) {
+        $purge_keys[] = $chronological_key;
+      }
+    }
+
+    if (!$purge_keys) {
+      return null;
+    }
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+
+    $conn = $table->establishConnection('w');
+    queryfx(
+      $conn,
+      'UPDATE %T SET hasViewed = 1
+        WHERE userPHID = %s AND chronologicalKey IN (%Ls)',
+      $table->getTableName(),
+      $viewer->getPHID(),
+      $purge_keys);
+
+    PhabricatorUserCache::clearCache(
+      PhabricatorUserNotificationCountCacheType::KEY_COUNT,
+      $viewer->getPHID());
+
+    unset($unguarded);
+
+    return phutil_tag(
+      'div',
+      array(
+        'class' => 'phabricator-notification phabricator-notification-warning',
+      ),
+      pht(
+        '%s notification(s) about objects which no longer exist or which '.
+        'you can no longer see were discarded.',
+        phutil_count($purge_keys)));
+  }
+
+
 }
diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php
index aa430a2518..b40bdee066 100644
--- a/src/applications/notification/query/PhabricatorNotificationQuery.php
+++ b/src/applications/notification/query/PhabricatorNotificationQuery.php
@@ -76,31 +76,31 @@ final class PhabricatorNotificationQuery
     return $stories;
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
-    $where = array();
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
 
     if ($this->userPHIDs !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'notif.userPHID IN (%Ls)',
         $this->userPHIDs);
     }
 
     if ($this->unread !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'notif.hasViewed = %d',
         (int)!$this->unread);
     }
 
     if ($this->keys) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'notif.chronologicalKey IN (%Ls)',
         $this->keys);
     }
 
-    return $this->formatWhereClause($where);
+    return $where;
   }
 
   protected function getResultCursor($item) {
diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
index eb74f8debf..d33f042d0b 100644
--- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
+++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
@@ -1651,6 +1651,14 @@ final class PhabricatorUSEnglishTranslation
         'Destroyed %s credentials of type "%s".',
       ),
 
+      '%s notification(s) about objects which no longer exist or which '.
+      'you can no longer see were discarded.' => array(
+        'One notification about an object which no longer exists or which '.
+        'you can no longer see was discarded.',
+        '%s notifications about objects which no longer exist or which '.
+        'you can no longer see were discarded.',
+      ),
+
     );
   }
 
diff --git a/webroot/rsrc/css/application/base/notification-menu.css b/webroot/rsrc/css/application/base/notification-menu.css
index 8e2391d57c..8db2436891 100644
--- a/webroot/rsrc/css/application/base/notification-menu.css
+++ b/webroot/rsrc/css/application/base/notification-menu.css
@@ -68,6 +68,10 @@
   color: {$lightgreytext};
 }
 
+.phabricator-notification-warning {
+  background: {$sh-yellowbackground};
+}
+
 .phabricator-notification-list .phabricator-notification-unread,
 .phabricator-notification-menu .phabricator-notification-unread {
   background: {$hoverblue};
@@ -95,7 +99,7 @@
 .phabricator-notification-unread .phabricator-notification-foot
   .phabricator-notification-status {
     font-size: 7px;
-    color: {$lightgreytext};
+    color: {$lightbluetext};
     position: absolute;
     display: inline-block;
     top: 6px;

From 19403fdb8e2939101d5bfbbb1d7ee5cf10b4a90a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 19 Apr 2018 11:28:24 -0700
Subject: [PATCH 13/19] Improve color use in "[+++-   ]" element for colorblind
 users

Summary:
Ref T13127. Users with red/green colorblindness may have difficulty using this element in its current incarnation.

We could give it different behavior if the "Accessibility" option is set for red/green colorblind users, but try a one-size-fits-all approach since the red/green aren't wholly clear anwyay.

Test Plan: {F5530050}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13127

Differential Revision: https://secure.phabricator.com/D19385
---
 resources/celerity/map.php                       |  6 +++---
 .../view/DifferentialRevisionListView.php        | 10 +++++++++-
 .../css/phui/object-item/phui-oi-list-view.css   | 16 ++++++++++------
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index dc018326bf..e41220b8b5 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
-    'core.pkg.css' => 'c30f6eaa',
+    'core.pkg.css' => 'cb8ae4dc',
     'core.pkg.js' => 'e1f0f7bd',
     'differential.pkg.css' => '06dc617c',
     'differential.pkg.js' => 'c2ca903a',
@@ -131,7 +131,7 @@ return array(
     'rsrc/css/phui/object-item/phui-oi-color.css' => 'cd2b9b77',
     'rsrc/css/phui/object-item/phui-oi-drag-ui.css' => '08f4ccc3',
     'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '9d9685d6',
-    'rsrc/css/phui/object-item/phui-oi-list-view.css' => 'ae1404ba',
+    'rsrc/css/phui/object-item/phui-oi-list-view.css' => '7c5c1291',
     'rsrc/css/phui/object-item/phui-oi-simple-ui.css' => 'a8beebea',
     'rsrc/css/phui/phui-action-list.css' => '0bcd9a45',
     'rsrc/css/phui/phui-action-panel.css' => 'b4798122',
@@ -838,7 +838,7 @@ return array(
     'phui-oi-color-css' => 'cd2b9b77',
     'phui-oi-drag-ui-css' => '08f4ccc3',
     'phui-oi-flush-ui-css' => '9d9685d6',
-    'phui-oi-list-view-css' => 'ae1404ba',
+    'phui-oi-list-view-css' => '7c5c1291',
     'phui-oi-simple-ui-css' => 'a8beebea',
     'phui-pager-css' => 'edcbc226',
     'phui-pinboard-view-css' => '2495140e',
diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php
index 4c69d76274..e53bccb892 100644
--- a/src/applications/differential/view/DifferentialRevisionListView.php
+++ b/src/applications/differential/view/DifferentialRevisionListView.php
@@ -229,22 +229,30 @@ final class DifferentialRevisionListView extends AphrontView {
     $classes = array();
     $classes[] = 'differential-revision-size';
 
+    $tip = array();
+    $tip[] = pht('%s Lines', new PhutilNumber($n));
+
     if ($plus_count <= 1) {
       $classes[] = 'differential-revision-small';
+      $tip[] = pht('Smaller Change');
     }
 
     if ($plus_count >= 4) {
       $classes[] = 'differential-revision-large';
+      $tip[] = pht('Larger Change');
     }
 
+    $tip = phutil_implode_html(" \xC2\xB7 ", $tip);
+
     return javelin_tag(
       'span',
       array(
         'class' => implode(' ', $classes),
         'sigil' => 'has-tooltip',
         'meta' => array(
-          'tip' => pht('%s Lines', new PhutilNumber($n)),
+          'tip' => $tip,
           'align' => 'E',
+          'size' => 400,
         ),
       ),
       $size);
diff --git a/webroot/rsrc/css/phui/object-item/phui-oi-list-view.css b/webroot/rsrc/css/phui/object-item/phui-oi-list-view.css
index 9436d171df..6f2421ca2f 100644
--- a/webroot/rsrc/css/phui/object-item/phui-oi-list-view.css
+++ b/webroot/rsrc/css/phui/object-item/phui-oi-list-view.css
@@ -697,22 +697,26 @@ ul.phui-oi-list-view .phui-oi-selectable
 
 .differential-revision-size .phui-icon-view {
   margin: 0 1px 0 1px;
-  font-size: smaller;
-  color: {$blueborder};
+  font-size: 7px;
+  position: relative;
+  top: -2px;
+  color: {$lightbluetext};
 }
 
 .differential-revision-large {
-  background: {$sh-redbackground};
+  background: {$sh-orangebackground};
 }
 
+/* NOTE: These are intentionally using nonstandard colors, see T13127. */
+
 .differential-revision-large .phui-icon-view {
-  color: {$red};
+  color: #e5ae7e;
 }
 
 .differential-revision-small {
-  background: {$sh-greenbackground};
+  background: #f2f7ff;
 }
 
 .differential-revision-small .phui-icon-view {
-  color: {$green};
+  color: #6699ba;
 }

From 843bfb4fd8ac72457691d0583330807d74671c97 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 19 Apr 2018 11:39:11 -0700
Subject: [PATCH 14/19] Add a "commits" attachment to
 "differential.diff.search" for retrieving local commit information

Summary:
Ref T13124. See PHI593.

When you `arc diff` in a Git or Mercurial repository, we upload some information about the local commits in your working copy which the change was generated from.

In the future (for example, with T1508) we may increase the prominence of this feature.

Provide a stable way to read this information back via the API. This roughly mirrors the information we provide about commits in "diffusion.commit.search", although the latter is less fleshed-out today.

Test Plan: Used `differential.diff.search` to retrieve commit information about Git, Mercurial, and Subversion diffs. (There's no info for Subversion, but it doesn't crash or anything.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19386
---
 src/__phutil_library_map__.php                |  2 +
 ...ferentialCommitsSearchEngineAttachment.php | 77 +++++++++++++++++++
 .../differential/storage/DifferentialDiff.php |  5 +-
 .../storage/PhabricatorRepositoryCommit.php   |  4 +
 4 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 src/applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 5f1543aa91..beec5058a6 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -455,6 +455,7 @@ phutil_register_library_map(array(
     'DifferentialCommitMessageParser' => 'applications/differential/parser/DifferentialCommitMessageParser.php',
     'DifferentialCommitMessageParserTestCase' => 'applications/differential/parser/__tests__/DifferentialCommitMessageParserTestCase.php',
     'DifferentialCommitsField' => 'applications/differential/customfield/DifferentialCommitsField.php',
+    'DifferentialCommitsSearchEngineAttachment' => 'applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php',
     'DifferentialConduitAPIMethod' => 'applications/differential/conduit/DifferentialConduitAPIMethod.php',
     'DifferentialConflictsCommitMessageField' => 'applications/differential/field/DifferentialConflictsCommitMessageField.php',
     'DifferentialController' => 'applications/differential/controller/DifferentialController.php',
@@ -5733,6 +5734,7 @@ phutil_register_library_map(array(
     'DifferentialCommitMessageParser' => 'Phobject',
     'DifferentialCommitMessageParserTestCase' => 'PhabricatorTestCase',
     'DifferentialCommitsField' => 'DifferentialCustomField',
+    'DifferentialCommitsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment',
     'DifferentialConduitAPIMethod' => 'ConduitAPIMethod',
     'DifferentialConflictsCommitMessageField' => 'DifferentialCommitMessageField',
     'DifferentialController' => 'PhabricatorController',
diff --git a/src/applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php b/src/applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php
new file mode 100644
index 0000000000..395541c9f9
--- /dev/null
+++ b/src/applications/differential/engineextension/DifferentialCommitsSearchEngineAttachment.php
@@ -0,0 +1,77 @@
+<?php
+
+final class DifferentialCommitsSearchEngineAttachment
+  extends PhabricatorSearchEngineAttachment {
+
+  public function getAttachmentName() {
+    return pht('Diff Commits');
+  }
+
+  public function getAttachmentDescription() {
+    return pht('Get the local commits (if any) for each diff.');
+  }
+
+  public function loadAttachmentData(array $objects, $spec) {
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID IN (%Ld) AND name = %s',
+      mpull($objects, 'getID'),
+      'local:commits');
+
+    $map = array();
+    foreach ($properties as $property) {
+      $map[$property->getDiffID()] = $property->getData();
+    }
+
+    return $map;
+  }
+
+  public function getAttachmentForObject($object, $data, $spec) {
+    $diff_id = $object->getID();
+    $info = idx($data, $diff_id, array());
+
+    // NOTE: This should be similar to the information returned about commits
+    // by "diffusion.commit.search".
+
+    $list = array();
+    foreach ($info as $commit) {
+      $author_epoch = idx($commit, 'time');
+      if ($author_epoch) {
+        $author_epoch = (int)$author_epoch;
+      }
+
+      // TODO: Currently, we don't upload the raw author string from "arc".
+      // Reconstruct a plausible version of it until we begin uploading this
+      // information.
+
+      $author_name = idx($commit, 'author');
+      $author_email = idx($commit, 'authorEmail');
+      if (strlen($author_name) && strlen($author_email)) {
+        $author_raw = (string)id(new PhutilEmailAddress())
+          ->setDisplayName($author_name)
+          ->setAddress($author_email);
+      } else if (strlen($author_email)) {
+        $author_raw = $author_email;
+      } else {
+        $author_raw = $author_name;
+      }
+
+      $list[] = array(
+        'identifier' => $commit['commit'],
+        'tree' => idx($commit, 'tree'),
+        'parents' => idx($commit, 'parents', array()),
+        'author' => array(
+          'name' => $author_name,
+          'email' => $author_email,
+          'raw' => $author_raw,
+          'epoch' => $author_epoch,
+        ),
+        'message' => idx($commit, 'message'),
+      );
+    }
+
+    return array(
+      'commits' => $list,
+    );
+  }
+
+}
diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php
index 728ec4b538..cb392c4306 100644
--- a/src/applications/differential/storage/DifferentialDiff.php
+++ b/src/applications/differential/storage/DifferentialDiff.php
@@ -815,7 +815,10 @@ final class DifferentialDiff
   }
 
   public function getConduitSearchAttachments() {
-    return array();
+    return array(
+      id(new DifferentialCommitsSearchEngineAttachment())
+        ->setAttachmentKey('commits'),
+    );
   }
 
 
diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
index 01088522e7..98f78e63e6 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -716,6 +716,10 @@ final class PhabricatorRepositoryCommit
   }
 
   public function getFieldValuesForConduit() {
+
+    // NOTE: This data should be similar to the information returned about
+    // commmits by "differential.diff.search" with the "commits" attachment.
+
     return array(
       'identifier' => $this->getCommitIdentifier(),
     );

From 4dc8e2de564f48365da1ec9e1bab38f99cb34d3a Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Wed, 18 Apr 2018 10:49:22 -0700
Subject: [PATCH 15/19] Add unique constraint to AlmanacInterfaces

Summary: See discussion in D19379. The 4-tuple of (device, network, address, port) should be unique.

Test Plan: Created lots of duplicate interfaces, bound those interfaces to various services, observed migration script clean things up correctly.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19388
---
 .../20180418.alamanc.interface.unique.php     | 85 +++++++++++++++++++
 .../almanac/editor/AlmanacInterfaceEditor.php | 18 ++++
 .../almanac/storage/AlmanacInterface.php      |  4 +
 3 files changed, 107 insertions(+)
 create mode 100644 resources/sql/autopatches/20180418.alamanc.interface.unique.php

diff --git a/resources/sql/autopatches/20180418.alamanc.interface.unique.php b/resources/sql/autopatches/20180418.alamanc.interface.unique.php
new file mode 100644
index 0000000000..0ad4fbea12
--- /dev/null
+++ b/resources/sql/autopatches/20180418.alamanc.interface.unique.php
@@ -0,0 +1,85 @@
+<?php
+
+$interface_table = new AlmanacInterface();
+$binding_table = new AlmanacBinding();
+$interface_conn = $interface_table->establishConnection('w');
+
+queryfx(
+  $interface_conn,
+  'LOCK TABLES %T WRITE, %T WRITE',
+  $interface_table->getTableName(),
+  $binding_table->getTableName());
+
+$seen = array();
+foreach (new LiskMigrationIterator($interface_table) as $interface) {
+  $device = $interface->getDevicePHID();
+  $network = $interface->getNetworkPHID();
+  $address = $interface->getAddress();
+  $port = $interface->getPort();
+  $key = "{$device}/{$network}/{$address}/{$port}";
+
+  // If this is the first copy of this row we've seen, mark it as seen and
+  // move on.
+  if (empty($seen[$key])) {
+    $seen[$key] = $interface->getID();
+    continue;
+  }
+
+  $survivor = queryfx_one(
+    $interface_conn,
+    'SELECT * FROM %T WHERE id = %d',
+    $interface_table->getTableName(),
+    $seen[$key]);
+
+  $bindings = queryfx_all(
+    $interface_conn,
+    'SELECT * FROM %T WHERE interfacePHID = %s',
+    $binding_table->getTableName(),
+    $interface->getPHID());
+
+  // Repoint bindings to the survivor.
+  foreach ($bindings as $binding) {
+    // Check if there's already a binding to the survivor.
+    $existing = queryfx_one(
+      $interface_conn,
+      'SELECT * FROM %T WHERE interfacePHID = %s and devicePHID = %s and '.
+      'servicePHID = %s',
+      $binding_table->getTableName(),
+      $survivor['phid'],
+      $binding['devicePHID'],
+      $binding['servicePHID']);
+
+    if (!$existing) {
+      // Reattach this binding to the survivor.
+      queryfx(
+        $interface_conn,
+        'UPDATE %T SET interfacePHID = %s WHERE id = %d',
+        $binding_table->getTableName(),
+        $survivor['phid'],
+        $binding['id']);
+    } else {
+      // Binding to survivor already exists. Remove this now-redundant binding.
+      queryfx(
+        $interface_conn,
+        'DELETE FROM %T WHERE id = %d',
+        $binding_table->getTableName(),
+        $binding['id']);
+    }
+  }
+
+  queryfx(
+    $interface_conn,
+    'DELETE FROM %T WHERE id = %d',
+    $interface_table->getTableName(),
+    $interface->getID());
+}
+
+queryfx(
+  $interface_conn,
+  'ALTER TABLE %T ADD UNIQUE KEY `key_unique` '.
+  '(devicePHID, networkPHID, address, port)',
+  $interface_table->getTableName());
+
+queryfx(
+  $interface_conn,
+  'UNLOCK TABLES');
diff --git a/src/applications/almanac/editor/AlmanacInterfaceEditor.php b/src/applications/almanac/editor/AlmanacInterfaceEditor.php
index 865402db36..771076efc9 100644
--- a/src/applications/almanac/editor/AlmanacInterfaceEditor.php
+++ b/src/applications/almanac/editor/AlmanacInterfaceEditor.php
@@ -15,4 +15,22 @@ final class AlmanacInterfaceEditor
     return pht('%s created %s.', $author, $object);
   }
 
+  protected function didCatchDuplicateKeyException(
+    PhabricatorLiskDAO $object,
+    array $xactions,
+    Exception $ex) {
+
+    $errors = array();
+
+    $errors[] = new PhabricatorApplicationTransactionValidationError(
+      null,
+      pht('Invalid'),
+      pht(
+        'Interfaces must have a unique combination of network, device, '.
+        'address, and port.'),
+      null);
+
+    throw new PhabricatorApplicationTransactionValidationException($errors);
+  }
+
 }
diff --git a/src/applications/almanac/storage/AlmanacInterface.php b/src/applications/almanac/storage/AlmanacInterface.php
index 4002651d08..5c7f65ddd1 100644
--- a/src/applications/almanac/storage/AlmanacInterface.php
+++ b/src/applications/almanac/storage/AlmanacInterface.php
@@ -35,6 +35,10 @@ final class AlmanacInterface
         'key_device' => array(
           'columns' => array('devicePHID'),
         ),
+        'key_unique' => array(
+          'columns' => array('devicePHID', 'networkPHID', 'address', 'port'),
+          'unique' => true,
+        ),
       ),
     ) + parent::getConfiguration();
   }

From 9bf4df2c1ddfa55dfbe375b2fa1a078e9d15fb71 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 17 Apr 2018 16:00:51 -0700
Subject: [PATCH 16/19] Allow demoted builds to automatically promote if builds
 pass after a restart

Summary:
Ref T13124. See PHI584. When you create a draft revision and it automatically demotes to "Changes Planned + Draft" because builds fail, let it promote to "Needs Review" automatically if builds pass. Usually, this will be because someone restarted the builds and they worked the second time.

Although I'm a little wary about adding even more state transitions to the diagram in T13110#237736, I think this one is reasonably natural and not ambiguous.

Test Plan:
  - Created a failing build plan with a "Throw Exception" step.
  - Created a revision which hit the build plan, saw it demote to "Changes Planned" when Harbormaster failed.
  - Edited the build plan to remove the "Throw Exception" step, restarted the build, got a pass.
  - Saw revision promote again:

{F5526104}

I didn't exhaustively test that the other 40 state transitions still work properly, but I think the scope of this change is small enough that it's unlikely I did much collateral damage.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19380
---
 .../editor/DifferentialTransactionEditor.php  | 36 ++++++++++++++++---
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 50094b02f4..82740459b0 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1555,13 +1555,41 @@ final class DifferentialTransactionEditor
       $auto_undraft = false;
     }
 
-    if ($object->isDraft() && $auto_undraft) {
+    $can_promote = false;
+    $can_demote = false;
+
+    // "Draft" revisions can promote to "Review Requested" after builds pass,
+    // or demote to "Changes Planned" after builds fail.
+    if ($object->isDraft()) {
+      $can_promote = true;
+      $can_demote = true;
+    }
+
+    // See PHI584. "Changes Planned" revisions which are not yet broadcasting
+    // can promote to "Review Requested" if builds pass.
+
+    // This pass is presumably the result of someone restarting the builds and
+    // having them work this time, perhaps because the builds are not perfectly
+    // reliable or perhaps because someone fixed some issue with build hardware
+    // or some other dependency.
+
+    // Currently, there's no legitimate way to end up in this state except
+    // through automatic demotion, so this behavior should not generate an
+    // undue level of confusion or ambiguity. Also note that these changes can
+    // not demote again since they've already been demoted once.
+    if ($object->isChangePlanned()) {
+      if (!$object->getShouldBroadcast()) {
+        $can_promote = true;
+      }
+    }
+
+    if (($can_promote || $can_demote) && $auto_undraft) {
       $status = $this->loadCompletedBuildableStatus($object);
 
       $is_passed = ($status === HarbormasterBuildableStatus::STATUS_PASSED);
       $is_failed = ($status === HarbormasterBuildableStatus::STATUS_FAILED);
 
-      if ($is_passed) {
+      if ($is_passed && $can_promote) {
         // When Harbormaster moves a revision out of the draft state, we
         // attribute the action to the revision author since this is more
         // natural and more useful.
@@ -1593,7 +1621,7 @@ final class DifferentialTransactionEditor
         // batch of transactions finishes so that Herald can fire on the new
         // revision state. See T13027 for discussion.
         $this->queueTransaction($xaction);
-      } else if ($is_failed) {
+      } else if ($is_failed && $can_demote) {
         // When demoting a revision, we act as "Harbormaster" instead of
         // the author since this feels a little more natural.
         $harbormaster_phid = id(new PhabricatorHarbormasterApplication())
@@ -1607,8 +1635,6 @@ final class DifferentialTransactionEditor
           ->setNewValue(true);
 
         $this->queueTransaction($xaction);
-
-        // TODO: Notify the author (only) that we did this.
       }
     }
 

From 95e179d9a4f31b8c5c2cbb4db8f7fa9f2d3867d6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 20 Apr 2018 12:51:31 -0700
Subject: [PATCH 17/19] Fix a fatal in the document engine blame view with
 files that blame to the initial commit

Summary:
Ref T13126. When you view a file using the new document engine view and some lines were introduced in the initial commit to the repository, Git renders "^abc123" in the blame output.

We currently don't do anything about this, and later fail to look it up and fatal.

It's also unlikely-but-conceivably-possible to end up here if a commit has not imported yet or has been nuked with `bin/remove destroy`.

Let the whole thing run without fataling even if a `$commit` is missing. Future refinements could improve this behavior.

Test Plan: Viewed a file with lines introduced in the initial commit, got empty blame instead of a fatal.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19391
---
 .../controller/DiffusionBlameController.php   | 50 +++++++++++++------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionBlameController.php b/src/applications/diffusion/controller/DiffusionBlameController.php
index 66403a8af9..85ab393ef6 100644
--- a/src/applications/diffusion/controller/DiffusionBlameController.php
+++ b/src/applications/diffusion/controller/DiffusionBlameController.php
@@ -80,7 +80,6 @@ final class DiffusionBlameController extends DiffusionController {
 
     $handles = $viewer->loadHandles($handle_phids);
 
-
     $map = array();
     $epochs = array();
     foreach ($identifiers as $identifier) {
@@ -106,9 +105,21 @@ final class DiffusionBlameController extends DiffusionController {
         ),
         $skip_icon);
 
-      $commit = $commits[$identifier];
+      // We may not have a commit object for a given identifier if the commit
+      // has not imported yet.
+
+      // At time of writing, this can also happen if a line was part of the
+      // initial import: blame produces a "^abc123" identifier in Git, which
+      // doesn't correspond to a real commit.
+
+      $commit = idx($commits, $identifier);
+
+      $author_phid = null;
+
+      if ($commit) {
+        $author_phid = $commit->getAuthorPHID();
+      }
 
-      $author_phid = $commit->getAuthorPHID();
       if (!$author_phid && $revision) {
         $author_phid = $revision->getAuthorPHID();
       }
@@ -141,18 +152,22 @@ final class DiffusionBlameController extends DiffusionController {
           'meta' => $author_meta,
         ));
 
-      $commit_link = javelin_tag(
-        'a',
-        array(
-          'href' => $commit->getURI(),
-          'sigil' => 'has-tooltip',
-          'meta'  => array(
-            'tip' => $this->renderCommitTooltip($commit, $handles),
-            'align' => 'E',
-            'size' => 600,
+      if ($commit) {
+        $commit_link = javelin_tag(
+          'a',
+          array(
+            'href' => $commit->getURI(),
+            'sigil' => 'has-tooltip',
+            'meta'  => array(
+              'tip' => $this->renderCommitTooltip($commit, $handles),
+              'align' => 'E',
+              'size' => 600,
+            ),
           ),
-        ),
-        $commit->getLocalName());
+          $commit->getLocalName());
+      } else {
+        $commit_link = null;
+      }
 
       $info = array(
         $author_link,
@@ -180,7 +195,12 @@ final class DiffusionBlameController extends DiffusionController {
         );
       }
 
-      $epoch = $commit->getEpoch();
+      if ($commit) {
+        $epoch = $commit->getEpoch();
+      } else {
+        $epoch = 0;
+      }
+
       $epochs[] = $epoch;
 
       $data = array(

From 8c78cde32f5e3c4db2fcd62c50ffae974c108e9a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 20 Apr 2018 13:09:47 -0700
Subject: [PATCH 18/19] Stop "git blame" from printing "^" markers on root
 repository commits

Summary: Depends on D19391. Ref T13126. See that task for some details on what's going on here.

Test Plan:
  - Viewed a file which includes lines that were added during the first commit to the repository.
  - Before D19391: fatal.
  - After D19391: blank.
  - After this patch: accurate blame information.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19392
---
 .../diffusion/query/blame/DiffusionGitBlameQuery.php        | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
index f98cc645a7..0eb15cd018 100644
--- a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
+++ b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php
@@ -7,8 +7,12 @@ final class DiffusionGitBlameQuery extends DiffusionBlameQuery {
 
     $commit = $request->getCommit();
 
+    // NOTE: The "--root" flag suppresses the addition of the "^" boundary
+    // commit marker. Without it, root commits render with a "^" before them,
+    // and one fewer character of the commit hash.
+
     return $repository->getLocalCommandFuture(
-      '--no-pager blame -s -l %s -- %s',
+      '--no-pager blame --root -s -l %s -- %s',
       $commit,
       $path);
   }

From 33da9f833fdd6bbf2d1ade40acd091f4a9f0ac76 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 20 Apr 2018 13:14:44 -0700
Subject: [PATCH 19/19] Fix odd line number line wrapping on embedded pastes
 (`{Pxxx}`)

Summary: Ref T13126. After SourceView changes, embedded pastes with the `{Pxxx}` syntax are line-wrapping line numbers in Safari, at least. Put a stop to this.

Test Plan: Viewed a `{Pxxx}` with more than 10 lines. Before: weird line wrapping; after: nice consistent display.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19393
---
 resources/celerity/map.php                               | 4 ++--
 webroot/rsrc/css/layout/phabricator-source-code-view.css | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index e41220b8b5..dd4d978fed 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -119,7 +119,7 @@ return array(
     'rsrc/css/font/font-lato.css' => 'c7ccd872',
     'rsrc/css/font/phui-font-icon-base.css' => '870a7360',
     'rsrc/css/layout/phabricator-filetree-view.css' => 'b912ad97',
-    'rsrc/css/layout/phabricator-source-code-view.css' => 'fdbefca0',
+    'rsrc/css/layout/phabricator-source-code-view.css' => '2ab25dfa',
     'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494',
     'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68',
     'rsrc/css/phui/button/phui-button.css' => '1863cc6e',
@@ -776,7 +776,7 @@ return array(
     'phabricator-search-results-css' => '505dd8cf',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-slowvote-css' => 'a94b7230',
-    'phabricator-source-code-view-css' => 'fdbefca0',
+    'phabricator-source-code-view-css' => '2ab25dfa',
     'phabricator-standard-page-view' => '34ee718b',
     'phabricator-textareautils' => '320810c8',
     'phabricator-title' => '485aaa6c',
diff --git a/webroot/rsrc/css/layout/phabricator-source-code-view.css b/webroot/rsrc/css/layout/phabricator-source-code-view.css
index b0c334da26..9b61425d63 100644
--- a/webroot/rsrc/css/layout/phabricator-source-code-view.css
+++ b/webroot/rsrc/css/layout/phabricator-source-code-view.css
@@ -24,6 +24,7 @@
   text-align: right;
   border-right: 1px solid {$paste.border};
   color: {$sh-yellowtext};
+  white-space: nowrap;
 }
 
 .phabricator-source-line > a::before {