From 584495215301bf9fa94cc09b3f53469efff8c979 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 2 Mar 2018 17:26:11 -0800
Subject: [PATCH 01/37] Show lint messages in deleted files on the left-hand
 side of the change

Summary:
See PHI416. If you raise a lint message in a deleted file, we don't render any text on the right hand side so the message never displays.

This is occasionally still legitimate/useful, e.g. to display a "don't delete this file" message. At least for now, show these messages on the left.

Test Plan: Posted a lint message on a deleted file via `harbormaster.sendmessage`, viewed revision, saw file expand with synthetic inline for lint.

Differential Revision: https://secure.phabricator.com/D19171
---
 .../DifferentialChangesetViewController.php      | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php
index c2bd4bf8de..9d02202f9f 100644
--- a/src/applications/differential/controller/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -390,10 +390,20 @@ final class DifferentialChangesetViewController extends DifferentialController {
       return array();
     }
 
+    $change_type = $changeset->getChangeType();
+    if (DifferentialChangeType::isDeleteChangeType($change_type)) {
+      // If this is a lint message on a deleted file, show it on the left
+      // side of the UI because there are no source code lines on the right
+      // side of the UI so inlines don't have anywhere to render. See PHI416.
+      $is_new = 0;
+    } else {
+      $is_new = 1;
+    }
+
     $template = id(new DifferentialInlineComment())
-        ->setChangesetID($changeset->getID())
-        ->setIsNewFile(1)
-        ->setLineLength(0);
+      ->setChangesetID($changeset->getID())
+      ->setIsNewFile($is_new)
+      ->setLineLength(0);
 
     $inlines = array();
     foreach ($messages as $message) {

From f31975f7a3be3790b7ecfe6f3b64227dc96002ff Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Mar 2018 06:49:30 -0800
Subject: [PATCH 02/37] Don't emit Content-Security-Policy when returning a
 response during preflight setup checks

Summary:
Ref T4340. See <https://discourse.phabricator-community.org/t/core-exception-during-installation/1193/8>.

If we return a response very early during setup, we may not be able to read from the environment yet. Just decline to build a "Content-Security-Policy" header in these cases.

Test Plan:
  - Faked a preflight error (e.g., safe_mode enabled), restarted apache.
    - Before patch: environment error while generating CSP.
    - After patch: no error.
  - Loaded a normal page, observed an normal CSP header.

Maniphest Tasks: T4340

Differential Revision: https://secure.phabricator.com/D19172
---
 src/aphront/response/AphrontResponse.php | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php
index 892417fcb1..2ee222d61c 100644
--- a/src/aphront/response/AphrontResponse.php
+++ b/src/aphront/response/AphrontResponse.php
@@ -103,9 +103,20 @@ abstract class AphrontResponse extends Phobject {
       return null;
     }
 
-    $csp = array();
+    // NOTE: We may return a response during preflight checks (for example,
+    // if a user has a bad version of PHP).
 
-    $cdn = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+    // In this case, setup isn't complete yet and we can't access environmental
+    // configuration. If we aren't able to read the environment, just decline
+    // to emit a Content-Security-Policy header.
+
+    try {
+      $cdn = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+    } catch (Exception $ex) {
+      return null;
+    }
+
+    $csp = array();
     if ($cdn) {
       $default = $this->newContentSecurityPolicySource($cdn);
     } else {

From fd367adbaf85f0fcb4b80c830f829ae61d30de68 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Mar 2018 14:07:47 -0800
Subject: [PATCH 03/37] Parameterize PhabricatorGlobalLock

Summary:
Ref T13096. Currently, we do a fair amount of clever digesting and string manipulation to build lock names which are less than 64 characters long while still being reasonably readable.

Instead, do more of this automatically. This will let lock acquisition become simpler and make it more possible to build a useful lock log.

Test Plan: Ran `bin/repository update`, saw a reasonable lock acquire and release.

Maniphest Tasks: T13096

Differential Revision: https://secure.phabricator.com/D19173
---
 .../engine/PhabricatorRepositoryEngine.php    | 11 +++---
 .../util/PhabricatorGlobalLock.php            | 38 +++++++++++++------
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php
index 244ed0cd45..fed5b09521 100644
--- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php
+++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php
@@ -56,19 +56,18 @@ abstract class PhabricatorRepositoryEngine extends Phobject {
     $lock_key,
     $lock_device_only) {
 
-    $lock_parts = array();
-    $lock_parts[] = $lock_key;
-    $lock_parts[] = $repository->getID();
+    $lock_parts = array(
+      'repositoryPHID' => $repository->getPHID(),
+    );
 
     if ($lock_device_only) {
       $device = AlmanacKeys::getLiveDevice();
       if ($device) {
-        $lock_parts[] = $device->getID();
+        $lock_parts['devicePHID'] = $device->getPHID();
       }
     }
 
-    $lock_name = implode(':', $lock_parts);
-    return PhabricatorGlobalLock::newLock($lock_name);
+    return PhabricatorGlobalLock::newLock($lock_key, $lock_parts);
   }
 
 
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index 26e11d1899..8aecb40873 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -28,6 +28,7 @@
  */
 final class PhabricatorGlobalLock extends PhutilLock {
 
+  private $parameters;
   private $conn;
   private $isExternalConnection = false;
 
@@ -37,27 +38,42 @@ final class PhabricatorGlobalLock extends PhutilLock {
 /* -(  Constructing Locks  )------------------------------------------------- */
 
 
-  public static function newLock($name) {
+  public static function newLock($name, $parameters = array()) {
     $namespace = PhabricatorLiskDAO::getStorageNamespace();
     $namespace = PhabricatorHash::digestToLength($namespace, 20);
 
-    $full_name = 'ph:'.$namespace.':'.$name;
+    $parts = array();
+    ksort($parameters);
+    foreach ($parameters as $key => $parameter) {
+      if (!preg_match('/^[a-zA-Z0-9]+\z/', $key)) {
+        throw new Exception(
+          pht(
+            'Lock parameter key "%s" must be alphanumeric.',
+            $key));
+      }
 
-    $length_limit = 64;
-    if (strlen($full_name) > $length_limit) {
-      throw new Exception(
-        pht(
-          'Lock name "%s" is too long (full lock name is "%s"). The '.
-          'full lock name must not be longer than %s bytes.',
-          $name,
-          $full_name,
-          new PhutilNumber($length_limit)));
+      if (!is_scalar($parameter) && !is_null($parameter)) {
+        throw new Exception(
+          pht(
+            'Lock parameter for key "%s" must be a scalar.',
+            $key));
+      }
+
+      $value = phutil_json_encode($parameter);
+      $parts[] = "{$key}={$value}";
     }
+    $parts = implode(', ', $parts);
 
+    $local = "{$name}({$parts})";
+    $local = PhabricatorHash::digestToLength($local, 20);
+
+    $full_name = "ph:{$namespace}:{$local}";
     $lock = self::getLock($full_name);
     if (!$lock) {
       $lock = new PhabricatorGlobalLock($full_name);
       self::registerLock($lock);
+
+      $lock->parameters = $parameters;
     }
 
     return $lock;

From 44f0664d2c6f1700881c8da9d00d0f1813c4f34e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 5 Mar 2018 14:22:13 -0800
Subject: [PATCH 04/37] Add a "lock log" for debugging where locks are being
 held

Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.

Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.

Maniphest Tasks: T13096

Differential Revision: https://secure.phabricator.com/D19174
---
 bin/lock                                      |   1 +
 .../autopatches/20180305.lock.01.locklog.sql  |   9 +
 scripts/setup/manage_lock.php                 |  21 ++
 src/__phutil_library_map__.php                |   8 +
 ...abricatorDaemonLockLogGarbageCollector.php |  29 +++
 .../PhabricatorLockLogManagementWorkflow.php  | 222 ++++++++++++++++++
 .../PhabricatorLockManagementWorkflow.php     |   4 +
 .../storage/PhabricatorDaemonLockLog.php      |  32 +++
 .../PhabricatorGarbageCollector.php           |   6 +-
 .../PhabricatorStorageManagementWorkflow.php  |  12 +-
 .../util/PhabricatorGlobalLock.php            |  51 ++++
 11 files changed, 389 insertions(+), 6 deletions(-)
 create mode 120000 bin/lock
 create mode 100644 resources/sql/autopatches/20180305.lock.01.locklog.sql
 create mode 100755 scripts/setup/manage_lock.php
 create mode 100644 src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php
 create mode 100644 src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php
 create mode 100644 src/applications/daemon/management/PhabricatorLockManagementWorkflow.php
 create mode 100644 src/applications/daemon/storage/PhabricatorDaemonLockLog.php

diff --git a/bin/lock b/bin/lock
new file mode 120000
index 0000000000..686fa38798
--- /dev/null
+++ b/bin/lock
@@ -0,0 +1 @@
+../scripts/setup/manage_lock.php
\ No newline at end of file
diff --git a/resources/sql/autopatches/20180305.lock.01.locklog.sql b/resources/sql/autopatches/20180305.lock.01.locklog.sql
new file mode 100644
index 0000000000..fa10c21c07
--- /dev/null
+++ b/resources/sql/autopatches/20180305.lock.01.locklog.sql
@@ -0,0 +1,9 @@
+CREATE TABLE {$NAMESPACE}_daemon.daemon_locklog (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  lockName VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT},
+  lockReleased INT UNSIGNED,
+  lockParameters LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
+  lockContext LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
diff --git a/scripts/setup/manage_lock.php b/scripts/setup/manage_lock.php
new file mode 100755
index 0000000000..ec5405ec01
--- /dev/null
+++ b/scripts/setup/manage_lock.php
@@ -0,0 +1,21 @@
+#!/usr/bin/env php
+<?php
+
+$root = dirname(dirname(dirname(__FILE__)));
+require_once $root.'/scripts/init/init-script.php';
+
+$args = new PhutilArgumentParser($argv);
+$args->setTagline(pht('manage locks'));
+$args->setSynopsis(<<<EOSYNOPSIS
+**lock** __command__ [__options__]
+  Manage locks.
+
+EOSYNOPSIS
+  );
+$args->parseStandardArguments();
+
+$workflows = id(new PhutilClassMapQuery())
+  ->setAncestorClass('PhabricatorLockManagementWorkflow')
+  ->execute();
+$workflows[] = new PhutilHelpArgumentWorkflow();
+$args->parseWorkflows($workflows);
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index bb08685634..c65ff759f6 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2670,6 +2670,8 @@ phutil_register_library_map(array(
     'PhabricatorDaemonController' => 'applications/daemon/controller/PhabricatorDaemonController.php',
     'PhabricatorDaemonDAO' => 'applications/daemon/storage/PhabricatorDaemonDAO.php',
     'PhabricatorDaemonEventListener' => 'applications/daemon/event/PhabricatorDaemonEventListener.php',
+    'PhabricatorDaemonLockLog' => 'applications/daemon/storage/PhabricatorDaemonLockLog.php',
+    'PhabricatorDaemonLockLogGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php',
     'PhabricatorDaemonLog' => 'applications/daemon/storage/PhabricatorDaemonLog.php',
     'PhabricatorDaemonLogEvent' => 'applications/daemon/storage/PhabricatorDaemonLogEvent.php',
     'PhabricatorDaemonLogEventGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLogEventGarbageCollector.php',
@@ -3204,6 +3206,8 @@ phutil_register_library_map(array(
     'PhabricatorLocalTimeTestCase' => 'view/__tests__/PhabricatorLocalTimeTestCase.php',
     'PhabricatorLocaleScopeGuard' => 'infrastructure/internationalization/scope/PhabricatorLocaleScopeGuard.php',
     'PhabricatorLocaleScopeGuardTestCase' => 'infrastructure/internationalization/scope/__tests__/PhabricatorLocaleScopeGuardTestCase.php',
+    'PhabricatorLockLogManagementWorkflow' => 'applications/daemon/management/PhabricatorLockLogManagementWorkflow.php',
+    'PhabricatorLockManagementWorkflow' => 'applications/daemon/management/PhabricatorLockManagementWorkflow.php',
     'PhabricatorLogTriggerAction' => 'infrastructure/daemon/workers/action/PhabricatorLogTriggerAction.php',
     'PhabricatorLogoutController' => 'applications/auth/controller/PhabricatorLogoutController.php',
     'PhabricatorLunarPhasePolicyRule' => 'applications/policy/rule/PhabricatorLunarPhasePolicyRule.php',
@@ -8194,6 +8198,8 @@ phutil_register_library_map(array(
     'PhabricatorDaemonController' => 'PhabricatorController',
     'PhabricatorDaemonDAO' => 'PhabricatorLiskDAO',
     'PhabricatorDaemonEventListener' => 'PhabricatorEventListener',
+    'PhabricatorDaemonLockLog' => 'PhabricatorDaemonDAO',
+    'PhabricatorDaemonLockLogGarbageCollector' => 'PhabricatorGarbageCollector',
     'PhabricatorDaemonLog' => array(
       'PhabricatorDaemonDAO',
       'PhabricatorPolicyInterface',
@@ -8794,6 +8800,8 @@ phutil_register_library_map(array(
     'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase',
     'PhabricatorLocaleScopeGuard' => 'Phobject',
     'PhabricatorLocaleScopeGuardTestCase' => 'PhabricatorTestCase',
+    'PhabricatorLockLogManagementWorkflow' => 'PhabricatorLockManagementWorkflow',
+    'PhabricatorLockManagementWorkflow' => 'PhabricatorManagementWorkflow',
     'PhabricatorLogTriggerAction' => 'PhabricatorTriggerAction',
     'PhabricatorLogoutController' => 'PhabricatorAuthController',
     'PhabricatorLunarPhasePolicyRule' => 'PhabricatorPolicyRule',
diff --git a/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php b/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php
new file mode 100644
index 0000000000..1fc62b9862
--- /dev/null
+++ b/src/applications/daemon/garbagecollector/PhabricatorDaemonLockLogGarbageCollector.php
@@ -0,0 +1,29 @@
+<?php
+
+final class PhabricatorDaemonLockLogGarbageCollector
+  extends PhabricatorGarbageCollector {
+
+  const COLLECTORCONST = 'daemon.lock-log';
+
+  public function getCollectorName() {
+    return pht('Lock Logs');
+  }
+
+  public function getDefaultRetentionPolicy() {
+    return 0;
+  }
+
+  protected function collectGarbage() {
+    $table = new PhabricatorDaemonLockLog();
+    $conn = $table->establishConnection('w');
+
+    queryfx(
+      $conn,
+      'DELETE FROM %T WHERE dateCreated < %d LIMIT 100',
+      $table->getTableName(),
+      $this->getGarbageEpoch());
+
+    return ($conn->getAffectedRows() == 100);
+  }
+
+}
diff --git a/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php b/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php
new file mode 100644
index 0000000000..5602bda309
--- /dev/null
+++ b/src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php
@@ -0,0 +1,222 @@
+<?php
+
+final class PhabricatorLockLogManagementWorkflow
+  extends PhabricatorLockManagementWorkflow {
+
+  protected function didConstruct() {
+    $this
+      ->setName('log')
+      ->setSynopsis(pht('Enable, disable, or show the lock log.'))
+      ->setArguments(
+        array(
+          array(
+            'name' => 'enable',
+            'help' => pht('Enable the lock log.'),
+          ),
+          array(
+            'name' => 'disable',
+            'help' => pht('Disable the lock log.'),
+          ),
+          array(
+            'name' => 'name',
+            'param' => 'name',
+            'help' => pht('Review logs for a specific lock.'),
+          ),
+        ));
+  }
+
+  public function execute(PhutilArgumentParser $args) {
+    $is_enable = $args->getArg('enable');
+    $is_disable = $args->getArg('disable');
+
+    if ($is_enable && $is_disable) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'You can not both "--enable" and "--disable" the lock log.'));
+    }
+
+    $with_name = $args->getArg('name');
+
+    if ($is_enable || $is_disable) {
+      if (strlen($with_name)) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'You can not both "--enable" or "--disable" with search '.
+            'parameters like "--name".'));
+      }
+
+      $gc = new PhabricatorDaemonLockLogGarbageCollector();
+      $is_enabled = (bool)$gc->getRetentionPolicy();
+
+      $config_key = 'phd.garbage-collection';
+      $const = $gc->getCollectorConstant();
+      $value = PhabricatorEnv::getEnvConfig($config_key);
+
+      if ($is_disable) {
+        if (!$is_enabled) {
+          echo tsprintf(
+            "%s\n",
+            pht('Lock log is already disabled.'));
+          return 0;
+        }
+        echo tsprintf(
+          "%s\n",
+          pht('Disabling the lock log.'));
+
+        unset($value[$const]);
+      } else {
+        if ($is_enabled) {
+          echo tsprintf(
+            "%s\n",
+            pht('Lock log is already enabled.'));
+          return 0;
+        }
+        echo tsprintf(
+          "%s\n",
+          pht('Enabling the lock log.'));
+
+        $value[$const] = phutil_units('24 hours in seconds');
+      }
+
+      id(new PhabricatorConfigLocalSource())
+        ->setKeys(
+          array(
+            $config_key => $value,
+          ));
+
+      echo tsprintf(
+        "%s\n",
+        pht('Done.'));
+
+      echo tsprintf(
+        "%s\n",
+        pht('Restart daemons to apply changes.'));
+
+      return 0;
+    }
+
+    $table = new PhabricatorDaemonLockLog();
+    $conn = $table->establishConnection('r');
+
+    $parts = array();
+    if (strlen($with_name)) {
+      $parts[] = qsprintf(
+        $conn,
+        'lockName = %s',
+        $with_name);
+    }
+
+    if (!$parts) {
+      $constraint = '1 = 1';
+    } else {
+      $constraint = '('.implode(') AND (', $parts).')';
+    }
+
+    $logs = $table->loadAllWhere(
+      '%Q ORDER BY id DESC LIMIT 100',
+      $constraint);
+    $logs = array_reverse($logs);
+
+    if (!$logs) {
+      echo tsprintf(
+        "%s\n",
+        pht('No matching lock logs.'));
+      return 0;
+    }
+
+    $table = id(new PhutilConsoleTable())
+      ->setBorders(true)
+      ->addColumn(
+        'id',
+        array(
+          'title' => pht('Lock'),
+        ))
+      ->addColumn(
+        'name',
+        array(
+          'title' => pht('Name'),
+        ))
+      ->addColumn(
+        'acquired',
+        array(
+          'title' => pht('Acquired'),
+        ))
+      ->addColumn(
+        'released',
+        array(
+          'title' => pht('Released'),
+        ))
+      ->addColumn(
+        'held',
+        array(
+          'title' => pht('Held'),
+        ))
+      ->addColumn(
+        'parameters',
+        array(
+          'title' => pht('Parameters'),
+        ))
+      ->addColumn(
+        'context',
+        array(
+          'title' => pht('Context'),
+        ));
+
+    $viewer = $this->getViewer();
+
+    foreach ($logs as $log) {
+      $created = $log->getDateCreated();
+      $released = $log->getLockReleased();
+
+      if ($released) {
+        $held = '+'.($released - $created);
+      } else {
+        $held = null;
+      }
+
+      $created = phabricator_datetime($created, $viewer);
+      $released = phabricator_datetime($released, $viewer);
+
+      $parameters = $log->getLockParameters();
+      $context = $log->getLockContext();
+
+      $table->addRow(
+        array(
+          'id' => $log->getID(),
+          'name' => $log->getLockName(),
+          'acquired' => $created,
+          'released' => $released,
+          'held' => $held,
+          'parameters' => $this->flattenParameters($parameters),
+          'context' => $this->flattenParameters($context),
+        ));
+    }
+
+    $table->draw();
+
+    return 0;
+  }
+
+  private function flattenParameters(array $params, $keys = true) {
+    $flat = array();
+    foreach ($params as $key => $value) {
+      if (is_array($value)) {
+        $value = $this->flattenParameters($value, false);
+      }
+      if ($keys) {
+        $flat[] = "{$key}={$value}";
+      } else {
+        $flat[] = "{$value}";
+      }
+    }
+
+    if ($keys) {
+      $flat = implode(', ', $flat);
+    } else {
+      $flat = implode(' ', $flat);
+    }
+
+    return $flat;
+  }
+
+}
diff --git a/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php b/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php
new file mode 100644
index 0000000000..e791f51090
--- /dev/null
+++ b/src/applications/daemon/management/PhabricatorLockManagementWorkflow.php
@@ -0,0 +1,4 @@
+<?php
+
+abstract class PhabricatorLockManagementWorkflow
+  extends PhabricatorManagementWorkflow {}
diff --git a/src/applications/daemon/storage/PhabricatorDaemonLockLog.php b/src/applications/daemon/storage/PhabricatorDaemonLockLog.php
new file mode 100644
index 0000000000..f9d80138cd
--- /dev/null
+++ b/src/applications/daemon/storage/PhabricatorDaemonLockLog.php
@@ -0,0 +1,32 @@
+<?php
+
+final class PhabricatorDaemonLockLog
+  extends PhabricatorDaemonDAO {
+
+  protected $lockName;
+  protected $lockReleased;
+  protected $lockParameters = array();
+  protected $lockContext = array();
+
+  protected function getConfiguration() {
+    return array(
+      self::CONFIG_SERIALIZATION => array(
+        'lockParameters' => self::SERIALIZATION_JSON,
+        'lockContext' => self::SERIALIZATION_JSON,
+      ),
+      self::CONFIG_COLUMN_SCHEMA => array(
+        'lockName' => 'text64',
+        'lockReleased' => 'epoch?',
+      ),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_lock' => array(
+          'columns' => array('lockName'),
+        ),
+        'key_created' => array(
+          'columns' => array('dateCreated'),
+        ),
+      ),
+    ) + parent::getConfiguration();
+  }
+
+}
diff --git a/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php b/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php
index f5960e9504..9cf1c22527 100644
--- a/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php
+++ b/src/infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php
@@ -100,8 +100,10 @@ abstract class PhabricatorGarbageCollector extends Phobject {
 
     // Hold a lock while performing collection to avoid racing other daemons
     // running the same collectors.
-    $lock_name = 'gc:'.$this->getCollectorConstant();
-    $lock = PhabricatorGlobalLock::newLock($lock_name);
+    $params = array(
+      'collector' => $this->getCollectorConstant(),
+    );
+    $lock = PhabricatorGlobalLock::newLock('gc', $params);
 
     try {
       $lock->lock(5);
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
index f6b120ba83..5b9459cfb8 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
@@ -1163,12 +1163,16 @@ abstract class PhabricatorStorageManagementWorkflow
     // Although we're holding this lock on different databases so it could
     // have the same name on each as far as the database is concerned, the
     // locks would be the same within this process.
-    $ref_key = $api->getRef()->getRefKey();
-    $ref_hash = PhabricatorHash::digestForIndex($ref_key);
-    $lock_name = 'adjust('.$ref_hash.')';
+    $parameters = array(
+      'refKey' => $api->getRef()->getRefKey(),
+    );
 
-    return PhabricatorGlobalLock::newLock($lock_name)
+    // We disable logging for this lock because we may not have created the
+    // log table yet, or may need to adjust it.
+
+    return PhabricatorGlobalLock::newLock('adjust', $parameters)
       ->useSpecificConnection($api->getConn(null))
+      ->setDisableLogging(true)
       ->lock();
   }
 
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index 8aecb40873..827d7e0e3d 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -31,6 +31,8 @@ final class PhabricatorGlobalLock extends PhutilLock {
   private $parameters;
   private $conn;
   private $isExternalConnection = false;
+  private $log;
+  private $disableLogging;
 
   private static $pool = array();
 
@@ -95,6 +97,11 @@ final class PhabricatorGlobalLock extends PhutilLock {
     return $this;
   }
 
+  public function setDisableLogging($disable) {
+    $this->disableLogging = $disable;
+    return $this;
+  }
+
 
 /* -(  Implementation  )----------------------------------------------------- */
 
@@ -143,6 +150,24 @@ final class PhabricatorGlobalLock extends PhutilLock {
     $conn->rememberLock($lock_name);
 
     $this->conn = $conn;
+
+    if ($this->shouldLogLock()) {
+      global $argv;
+
+      $lock_context = array(
+        'pid' => getmypid(),
+        'host' => php_uname('n'),
+        'argv' => $argv,
+      );
+
+      $log = id(new PhabricatorDaemonLockLog())
+        ->setLockName($lock_name)
+        ->setLockParameters($this->parameters)
+        ->setLockContext($lock_context)
+        ->save();
+
+      $this->log = $log;
+    }
   }
 
   protected function doUnlock() {
@@ -175,6 +200,32 @@ final class PhabricatorGlobalLock extends PhutilLock {
       $conn->close();
       self::$pool[] = $conn;
     }
+
+    if ($this->log) {
+      $log = $this->log;
+      $this->log = null;
+
+      $conn = $log->establishConnection('w');
+      queryfx(
+        $conn,
+        'UPDATE %T SET lockReleased = UNIX_TIMESTAMP() WHERE id = %d',
+        $log->getTableName(),
+        $log->getID());
+    }
+  }
+
+  private function shouldLogLock() {
+    if ($this->disableLogging) {
+      return false;
+    }
+
+    $policy = id(new PhabricatorDaemonLockLogGarbageCollector())
+      ->getRetentionPolicy();
+    if (!$policy) {
+      return false;
+    }
+
+    return true;
   }
 
 }

From 743d1ac426fbb82ac9ca5736cbca26d3a84e7bbc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 08:12:04 -0800
Subject: [PATCH 05/37] Mostly modularize the Differential "update" transaction

Summary: Ref T13099. Move most of the "Update" logic to modular transactions

Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19175
---
 .../autopatches/20140212.dx.1.armageddon.php  |   2 +-
 src/__phutil_library_map__.php                |   2 +
 .../conduit/DifferentialConduitAPIMethod.php  |   2 +-
 .../editor/DifferentialRevisionEditEngine.php |   7 +-
 .../editor/DifferentialTransactionEditor.php  | 148 +-------------
 .../DifferentialDiffExtractionEngine.php      |   4 +-
 ...rDifferentialRevisionTestDataGenerator.php |   4 +-
 .../storage/DifferentialTransaction.php       |  60 +-----
 .../DifferentialRevisionUpdateTransaction.php | 185 ++++++++++++++++++
 .../ReleephDiffChurnFieldSpecification.php    |   2 +-
 10 files changed, 208 insertions(+), 208 deletions(-)
 create mode 100644 src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php

diff --git a/resources/sql/autopatches/20140212.dx.1.armageddon.php b/resources/sql/autopatches/20140212.dx.1.armageddon.php
index 021e886629..a7e978b559 100644
--- a/resources/sql/autopatches/20140212.dx.1.armageddon.php
+++ b/resources/sql/autopatches/20140212.dx.1.armageddon.php
@@ -75,7 +75,7 @@ foreach ($rows as $row) {
 
   if ($diff_id || $row['action'] == DifferentialAction::ACTION_UPDATE) {
     $xactions[] = array(
-      'type' => DifferentialTransaction::TYPE_UPDATE,
+      'type' => DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE,
       'old' => null,
       'new' => $diff_id,
     );
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index c65ff759f6..6cc7450820 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -598,6 +598,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionTitleTransaction' => 'applications/differential/xaction/DifferentialRevisionTitleTransaction.php',
     'DifferentialRevisionTransactionType' => 'applications/differential/xaction/DifferentialRevisionTransactionType.php',
     'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php',
+    'DifferentialRevisionUpdateTransaction' => 'applications/differential/xaction/DifferentialRevisionUpdateTransaction.php',
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
     'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php',
     'DifferentialRevisionWrongStateTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php',
@@ -5822,6 +5823,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionTitleTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionTransactionType' => 'PhabricatorModularTransactionType',
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',
+    'DifferentialRevisionUpdateTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionViewController' => 'DifferentialController',
     'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType',
diff --git a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
index 34843b5609..13ed71c96e 100644
--- a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
+++ b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php
@@ -58,7 +58,7 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod {
 
     $xactions = array();
     $xactions[] = array(
-      'type' => DifferentialRevisionEditEngine::KEY_UPDATE,
+      'type' => DifferentialRevisionUpdateTransaction::EDITKEY,
       'value' => $diff->getPHID(),
     );
 
diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index 2bd3a5cdc3..fe1b983f1a 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -7,8 +7,6 @@ final class DifferentialRevisionEditEngine
 
   const ENGINECONST = 'differential.revision';
 
-  const KEY_UPDATE = 'update';
-
   const ACTIONGROUP_REVIEW = 'review';
   const ACTIONGROUP_REVISION = 'revision';
 
@@ -123,12 +121,13 @@ final class DifferentialRevisionEditEngine
     $fields = array();
 
     $fields[] = id(new PhabricatorHandlesEditField())
-      ->setKey(self::KEY_UPDATE)
+      ->setKey(DifferentialRevisionUpdateTransaction::EDITKEY)
       ->setLabel(pht('Update Diff'))
       ->setDescription(pht('New diff to create or update the revision with.'))
       ->setConduitDescription(pht('Create or update a revision with a diff.'))
       ->setConduitTypeDescription(pht('PHID of the diff.'))
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setTransactionType(
+        DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE)
       ->setHandleParameterType(new AphrontPHIDListHTTPParameterType())
       ->setSingleValue($diff_phid)
       ->setIsConduitOnly(!$diff)
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index a092bf2693..03af04dd0f 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -33,7 +33,7 @@ final class DifferentialTransactionEditor
   }
 
   public function getDiffUpdateTransaction(array $xactions) {
-    $type_update = DifferentialTransaction::TYPE_UPDATE;
+    $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
 
     foreach ($xactions as $xaction) {
       if ($xaction->getTransactionType() == $type_update) {
@@ -76,7 +76,6 @@ final class DifferentialTransactionEditor
     $types[] = PhabricatorTransactions::TYPE_INLINESTATE;
 
     $types[] = DifferentialTransaction::TYPE_INLINE;
-    $types[] = DifferentialTransaction::TYPE_UPDATE;
 
     return $types;
   }
@@ -88,12 +87,6 @@ final class DifferentialTransactionEditor
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_INLINE:
         return null;
-      case DifferentialTransaction::TYPE_UPDATE:
-        if ($this->getIsNewObject()) {
-          return null;
-        } else {
-          return $object->getActiveDiff()->getPHID();
-        }
     }
 
     return parent::getCustomTransactionOldValue($object, $xaction);
@@ -104,8 +97,6 @@ final class DifferentialTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case DifferentialTransaction::TYPE_UPDATE:
-        return $xaction->getNewValue();
       case DifferentialTransaction::TYPE_INLINE:
         return null;
     }
@@ -120,29 +111,6 @@ final class DifferentialTransactionEditor
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_INLINE:
         return;
-      case DifferentialTransaction::TYPE_UPDATE:
-        if (!$this->getIsCloseByCommit()) {
-          if ($object->isNeedsRevision() ||
-              $object->isChangePlanned() ||
-              $object->isAbandoned()) {
-            $object->setModernRevisionStatus(
-              DifferentialRevisionStatus::NEEDS_REVIEW);
-          }
-        }
-
-        $diff = $this->requireDiff($xaction->getNewValue());
-
-        $this->updateRevisionLineCounts($object, $diff);
-
-        if ($this->repositoryPHIDOverride !== false) {
-          $object->setRepositoryPHID($this->repositoryPHIDOverride);
-        } else {
-          $object->setRepositoryPHID($diff->getRepositoryPHID());
-        }
-
-        $object->attachActiveDiff($diff);
-        $object->setActiveDiffPHID($diff->getPHID());
-        return;
     }
 
     return parent::applyCustomInternalTransaction($object, $xaction);
@@ -196,7 +164,7 @@ final class DifferentialTransactionEditor
       // commit.
     } else {
       switch ($xaction->getTransactionType()) {
-        case DifferentialTransaction::TYPE_UPDATE:
+        case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
           $downgrade_rejects = true;
           if (!$is_sticky_accept) {
             // If "sticky accept" is disabled, also downgrade the accepts.
@@ -243,7 +211,7 @@ final class DifferentialTransactionEditor
 
     $is_commandeer = false;
     switch ($xaction->getTransactionType()) {
-      case DifferentialTransaction::TYPE_UPDATE:
+      case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
         if ($this->getIsCloseByCommit()) {
           // Don't bother with any of this if this update is a side effect of
           // commit detection.
@@ -293,7 +261,7 @@ final class DifferentialTransactionEditor
     if (!$this->didExpandInlineState) {
       switch ($xaction->getTransactionType()) {
         case PhabricatorTransactions::TYPE_COMMENT:
-        case DifferentialTransaction::TYPE_UPDATE:
+        case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
         case DifferentialTransaction::TYPE_INLINE:
           $this->didExpandInlineState = true;
 
@@ -343,45 +311,6 @@ final class DifferentialTransactionEditor
         if ($reply && !$reply->getHasReplies()) {
           $reply->setHasReplies(1)->save();
         }
-        return;
-      case DifferentialTransaction::TYPE_UPDATE:
-        // Now that we're inside the transaction, do a final check.
-        $diff = $this->requireDiff($xaction->getNewValue());
-
-        // TODO: It would be slightly cleaner to just revalidate this
-        // transaction somehow using the same validation code, but that's
-        // not easy to do at the moment.
-
-        $revision_id = $diff->getRevisionID();
-        if ($revision_id && ($revision_id != $object->getID())) {
-          throw new Exception(
-            pht(
-              'Diff is already attached to another revision. You lost '.
-              'a race?'));
-        }
-
-        // TODO: This can race with diff updates, particularly those from
-        // Harbormaster. See discussion in T8650.
-        $diff->setRevisionID($object->getID());
-        $diff->save();
-
-        // If there are any outstanding buildables for this diff, tell
-        // Harbormaster that their containers need to be updated. This is
-        // common, because `arc` creates buildables so it can upload lint
-        // and unit results.
-
-        $buildables = id(new HarbormasterBuildableQuery())
-          ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->withManualBuildables(false)
-          ->withBuildablePHIDs(array($diff->getPHID()))
-          ->execute();
-        foreach ($buildables as $buildable) {
-          $buildable->sendMessage(
-            $this->getActor(),
-            HarbormasterMessageType::BUILDABLE_CONTAINER,
-            true);
-        }
-
         return;
     }
 
@@ -437,7 +366,7 @@ final class DifferentialTransactionEditor
 
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
-        case DifferentialTransaction::TYPE_UPDATE:
+        case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
           $diff = $this->requireDiff($xaction->getNewValue(), true);
 
           // Update these denormalized index tables when we attach a new
@@ -554,44 +483,6 @@ final class DifferentialTransactionEditor
     return $xactions;
   }
 
-
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    $config_self_accept_key = 'differential.allow-self-accept';
-    $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
-
-    foreach ($xactions as $xaction) {
-      switch ($type) {
-        case DifferentialTransaction::TYPE_UPDATE:
-          $diff = $this->loadDiff($xaction->getNewValue());
-          if (!$diff) {
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht('The specified diff does not exist.'),
-              $xaction);
-          } else if (($diff->getRevisionID()) &&
-            ($diff->getRevisionID() != $object->getID())) {
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht(
-                'You can not update this revision to the specified diff, '.
-                'because the diff is already attached to another revision.'),
-              $xaction);
-          }
-          break;
-      }
-    }
-
-    return $errors;
-  }
-
   protected function sortTransactions(array $xactions) {
     $xactions = parent::sortTransactions($xactions);
 
@@ -674,7 +565,7 @@ final class DifferentialTransactionEditor
       $action = parent::getMailAction($object, $xactions);
 
       $strongest = $this->getStrongestAction($object, $xactions);
-      $type_update = DifferentialTransaction::TYPE_UPDATE;
+      $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
       if ($strongest->getTransactionType() == $type_update) {
         $show_lines = true;
       }
@@ -772,7 +663,7 @@ final class DifferentialTransactionEditor
     $update_xaction = null;
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
-        case DifferentialTransaction::TYPE_UPDATE:
+        case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
           $update_xaction = $xaction;
           break;
       }
@@ -1053,7 +944,7 @@ final class DifferentialTransactionEditor
     return $query->executeOne();
   }
 
-  private function requireDiff($phid, $need_changesets = false) {
+  public function requireDiff($phid, $need_changesets = false) {
     $diff = $this->loadDiff($phid, $need_changesets);
     if (!$diff) {
       throw new Exception(pht('Diff "%s" does not exist!', $phid));
@@ -1274,7 +1165,7 @@ final class DifferentialTransactionEditor
     $has_update = false;
     $has_commit = false;
 
-    $type_update = DifferentialTransaction::TYPE_UPDATE;
+    $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
     foreach ($xactions as $xaction) {
       if ($xaction->getTransactionType() != $type_update) {
         continue;
@@ -1721,27 +1612,6 @@ final class DifferentialTransactionEditor
     return true;
   }
 
-  private function updateRevisionLineCounts(
-    DifferentialRevision $revision,
-    DifferentialDiff $diff) {
-
-    $revision->setLineCount($diff->getLineCount());
-
-    $conn = $revision->establishConnection('r');
-
-    $row = queryfx_one(
-      $conn,
-      'SELECT SUM(addLines) A, SUM(delLines) D FROM %T
-        WHERE diffID = %d',
-      id(new DifferentialChangeset())->getTableName(),
-      $diff->getID());
-
-    if ($row) {
-      $revision->setAddedLineCount((int)$row['A']);
-      $revision->setRemovedLineCount((int)$row['D']);
-    }
-  }
-
   private function requireReviewers(DifferentialRevision $revision) {
     if ($revision->hasAttachedReviewers()) {
       return;
diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
index 6fb5e70de5..a6487c8108 100644
--- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
+++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
@@ -278,8 +278,10 @@ final class DifferentialDiffExtractionEngine extends Phobject {
         ->setNewValue($revision->getModernRevisionStatus());
     }
 
+    $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
+
     $xactions[] = id(new DifferentialTransaction())
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setTransactionType($type_update)
       ->setIgnoreOnNoEffect(true)
       ->setNewValue($new_diff->getPHID())
       ->setMetadataValue('isCommitUpdate', true);
diff --git a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php
index c37974fa2e..2443fe9651 100644
--- a/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php
+++ b/src/applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php
@@ -22,14 +22,14 @@ final class PhabricatorDifferentialRevisionTestDataGenerator
     $revision->setTestPlan($this->generateDescription());
 
     $diff = $this->generateDiff($author);
+    $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
 
     $xactions = array();
 
     $xactions[] = id(new DifferentialTransaction())
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setTransactionType($type_update)
       ->setNewValue($diff->getPHID());
 
-
     id(new DifferentialTransactionEditor())
       ->setActor($author)
       ->setContentSource($this->getLipsumContentSource())
diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php
index f7dcf29860..53fdc71a15 100644
--- a/src/applications/differential/storage/DifferentialTransaction.php
+++ b/src/applications/differential/storage/DifferentialTransaction.php
@@ -6,7 +6,6 @@ final class DifferentialTransaction
   private $isCommandeerSideEffect;
 
   const TYPE_INLINE  = 'differential:inline';
-  const TYPE_UPDATE  = 'differential:update';
   const TYPE_ACTION  = 'differential:action';
 
   const MAILTAG_REVIEWERS      = 'differential-reviewers';
@@ -75,18 +74,6 @@ final class DifferentialTransaction
     $new = $this->getNewValue();
 
     switch ($this->getTransactionType()) {
-      case self::TYPE_UPDATE:
-        // Older versions of this transaction have an ID for the new value,
-        // and/or do not record the old value. Only hide the transaction if
-        // the new value is a PHID, indicating that this is a newer style
-        // transaction.
-        if ($old === null) {
-          if (phid_get_type($new) == DifferentialDiffPHIDType::TYPECONST) {
-            return true;
-          }
-        }
-        break;
-
       case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE:
         // Don't hide the initial "X requested review: ..." transaction from
         // mail or feed even when it occurs during creation. We need this
@@ -139,11 +126,6 @@ final class DifferentialTransaction
           }
         }
         break;
-      case self::TYPE_UPDATE:
-        if ($new) {
-          $phids[] = $new;
-        }
-        break;
     }
 
     return $phids;
@@ -153,8 +135,6 @@ final class DifferentialTransaction
     switch ($this->getTransactionType()) {
       case self::TYPE_ACTION:
         return 3;
-      case self::TYPE_UPDATE:
-        return 2;
     }
 
     return parent::getActionStrength();
@@ -165,13 +145,6 @@ final class DifferentialTransaction
     switch ($this->getTransactionType()) {
       case self::TYPE_INLINE:
         return pht('Commented On');
-      case self::TYPE_UPDATE:
-        $old = $this->getOldValue();
-        if ($old === null) {
-          return pht('Request');
-        } else {
-          return pht('Updated');
-        }
       case self::TYPE_ACTION:
         $map = array(
           DifferentialAction::ACTION_ACCEPT => pht('Accepted'),
@@ -209,7 +182,7 @@ final class DifferentialTransaction
             break;
         }
         break;
-      case self::TYPE_UPDATE:
+      case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
         $old = $this->getOldValue();
         if ($old === null) {
           $tags[] = self::MAILTAG_REVIEW_REQUEST;
@@ -248,28 +221,6 @@ final class DifferentialTransaction
         return pht(
           '%s added inline comments.',
           $author_handle);
-      case self::TYPE_UPDATE:
-        if ($this->getMetadataValue('isCommitUpdate')) {
-          return pht(
-            'This revision was automatically updated to reflect the '.
-            'committed changes.');
-        } else if ($new) {
-          // TODO: Migrate to PHIDs and use handles here?
-          if (phid_get_type($new) == DifferentialDiffPHIDType::TYPECONST) {
-            return pht(
-              '%s updated this revision to %s.',
-              $author_handle,
-              $this->renderHandleLink($new));
-          } else {
-            return pht(
-              '%s updated this revision.',
-              $author_handle);
-          }
-        } else {
-          return pht(
-            '%s updated this revision.',
-            $author_handle);
-        }
       case self::TYPE_ACTION:
         switch ($new) {
           case DifferentialAction::ACTION_CLOSE:
@@ -347,11 +298,6 @@ final class DifferentialTransaction
           '%s added inline comments to %s.',
           $author_link,
           $object_link);
-      case self::TYPE_UPDATE:
-        return pht(
-          '%s updated the diff for %s.',
-          $author_link,
-          $object_link);
       case self::TYPE_ACTION:
         switch ($new) {
           case DifferentialAction::ACTION_ACCEPT:
@@ -462,8 +408,6 @@ final class DifferentialTransaction
     switch ($this->getTransactionType()) {
       case self::TYPE_INLINE:
         return 'fa-comment';
-      case self::TYPE_UPDATE:
-        return 'fa-refresh';
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:
@@ -526,8 +470,6 @@ final class DifferentialTransaction
 
   public function getColor() {
     switch ($this->getTransactionType()) {
-      case self::TYPE_UPDATE:
-        return PhabricatorTransactions::COLOR_SKY;
       case self::TYPE_ACTION:
         switch ($this->getNewValue()) {
           case DifferentialAction::ACTION_CLOSE:
diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
new file mode 100644
index 0000000000..8b53bced88
--- /dev/null
+++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
@@ -0,0 +1,185 @@
+<?php
+
+final class DifferentialRevisionUpdateTransaction
+  extends DifferentialRevisionTransactionType {
+
+  const TRANSACTIONTYPE = 'differential:update';
+  const EDITKEY = 'update';
+
+  public function generateOldValue($object) {
+    return $object->getActiveDiffPHID();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $should_review = $this->shouldRequestReviewAfterUpdate($object);
+    if ($should_review) {
+      $object->setModernRevisionStatus(
+        DifferentialRevisionStatus::NEEDS_REVIEW);
+    }
+
+    $editor = $this->getEditor();
+    $diff = $editor->requireDiff($value);
+
+    $this->updateRevisionLineCounts($object, $diff);
+
+    $object->setRepositoryPHID($diff->getRepositoryPHID());
+    $object->setActiveDiffPHID($diff->getPHID());
+    $object->attachActiveDiff($diff);
+  }
+
+  private function shouldRequestReviewAfterUpdate($object) {
+    if ($this->isCommitUpdate()) {
+      return false;
+    }
+
+    $should_update =
+      $object->isNeedsRevision() ||
+      $object->isChangePlanned() ||
+      $object->isAbandoned();
+    if ($should_update) {
+      return true;
+    }
+
+    return false;
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $editor = $this->getEditor();
+    $diff = $editor->requireDiff($value);
+
+    // TODO: This can race with diff updates, particularly those from
+    // Harbormaster. See discussion in T8650.
+    $diff->setRevisionID($object->getID());
+    $diff->save();
+
+    // If there are any outstanding buildables for this diff, tell
+    // Harbormaster that their containers need to be updated. This is
+    // common, because `arc` creates buildables so it can upload lint
+    // and unit results.
+
+    $buildables = id(new HarbormasterBuildableQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withManualBuildables(false)
+      ->withBuildablePHIDs(array($diff->getPHID()))
+      ->execute();
+    foreach ($buildables as $buildable) {
+      $buildable->sendMessage(
+        $this->getActor(),
+        HarbormasterMessageType::BUILDABLE_CONTAINER,
+        true);
+    }
+  }
+
+  public function getColor() {
+    return 'sky';
+  }
+
+  public function getIcon() {
+    return 'fa-refresh';
+  }
+
+  public function getActionName() {
+    if ($this->isCreateTransaction()) {
+      return pht('Request');
+    } else {
+      return pht('Updated');
+    }
+  }
+
+  public function getActionStrength() {
+    return 2;
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    if ($this->isCommitUpdate()) {
+      return pht(
+        'This revision was automatically updated to reflect the '.
+        'committed changes.');
+    }
+
+    // NOTE: Very, very old update transactions did not have a new value or
+    // did not use a diff PHID as a new value. This was changed years ago,
+    // but wasn't migrated. We might consider migrating if this causes issues.
+
+    return pht(
+      '%s updated this revision to %s.',
+      $this->renderAuthor(),
+      $this->renderNewHandle());
+  }
+
+  public function getTitleForFeed() {
+    return pht(
+      '%s updated the diff for %s.',
+      $this->renderAuthor(),
+      $this->renderObject());
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $diff_phid = null;
+    foreach ($xactions as $xaction) {
+      $diff_phid = $xaction->getNewValue();
+
+      $diff = id(new DifferentialDiffQuery())
+        ->withPHIDs(array($diff_phid))
+        ->setViewer($this->getActor())
+        ->executeOne();
+      if (!$diff) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Specified diff ("%s") does not exist.',
+            $diff_phid),
+          $xaction);
+        continue;
+      }
+
+      if ($diff->getRevisionID()) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'You can not update this revision with the specified diff ("%s") '.
+            'because the diff is already attached to another revision.',
+            $diff_phid),
+          $xaction);
+        continue;
+      }
+    }
+
+    if (!$diff_phid && !$object->getActiveDiffPHID()) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'You must specify an initial diff when creating a revision.'));
+    }
+
+    return $errors;
+  }
+
+  public function isCommitUpdate() {
+    return (bool)$this->getMetadataValue('isCommitUpdate');
+  }
+
+  private function updateRevisionLineCounts(
+    DifferentialRevision $revision,
+    DifferentialDiff $diff) {
+
+    $revision->setLineCount($diff->getLineCount());
+
+    $conn = $revision->establishConnection('r');
+
+    $row = queryfx_one(
+      $conn,
+      'SELECT SUM(addLines) A, SUM(delLines) D FROM %T
+        WHERE diffID = %d',
+      id(new DifferentialChangeset())->getTableName(),
+      $diff->getID());
+
+    if ($row) {
+      $revision->setAddedLineCount((int)$row['A']);
+      $revision->setRemovedLineCount((int)$row['D']);
+    }
+  }
+
+}
diff --git a/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php b/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php
index 112ef09885..3b472bbae7 100644
--- a/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php
+++ b/src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php
@@ -37,7 +37,7 @@ final class ReleephDiffChurnFieldSpecification
         case PhabricatorTransactions::TYPE_COMMENT:
           $comments++;
           break;
-        case DifferentialTransaction::TYPE_UPDATE:
+        case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE:
           $updates++;
           break;
         case DifferentialTransaction::TYPE_ACTION:

From f3928962096c1dc4472b19bd123812d88a1b9a23 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 08:40:34 -0800
Subject: [PATCH 06/37] Return commit information for Revision "close" and
 "update" transactions over the Conduit API

Summary: Depends on D19175. Ref T13099. This fills in "close" and "update" transactions so that they show which commit(s) caused the action.

Test Plan: Used `transaction.search` to query some revisions, saw commit PHID information.

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19176
---
 .../DifferentialDiffExtractionEngine.php      |  3 ++-
 .../DifferentialRevisionCloseTransaction.php  | 19 +++++++++++++++++++
 .../DifferentialRevisionUpdateTransaction.php | 14 ++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
index a6487c8108..d7d7c767e1 100644
--- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
+++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
@@ -284,7 +284,8 @@ final class DifferentialDiffExtractionEngine extends Phobject {
       ->setTransactionType($type_update)
       ->setIgnoreOnNoEffect(true)
       ->setNewValue($new_diff->getPHID())
-      ->setMetadataValue('isCommitUpdate', true);
+      ->setMetadataValue('isCommitUpdate', true)
+      ->setMetadataValue('commitPHIDs', array($commit->getPHID()));
 
     foreach ($more_xactions as $more_xaction) {
       $xactions[] = $more_xaction;
diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
index b1c796dca4..7ef0f2ba60 100644
--- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
@@ -136,4 +136,23 @@ final class DifferentialRevisionCloseTransaction
       $this->renderObject());
   }
 
+  public function getTransactionTypeForConduit($xaction) {
+    return 'close';
+  }
+
+  public function getFieldValuesForConduit($object, $data) {
+    $commit_phid = $object->getMetadataValue('commitPHID');
+
+    if ($commit_phid) {
+      $commit_phids = array($commit_phid);
+    } else {
+      $commit_phids = array();
+    }
+
+    return array(
+      'commitPHIDs' => $commit_phids,
+    );
+  }
+
+
 }
diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
index 8b53bced88..c89c3c79e1 100644
--- a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
@@ -182,4 +182,18 @@ final class DifferentialRevisionUpdateTransaction
     }
   }
 
+  public function getTransactionTypeForConduit($xaction) {
+    return 'update';
+  }
+
+  public function getFieldValuesForConduit($object, $data) {
+    $commit_phids = $object->getMetadataValue('commitPHIDs', array());
+
+    return array(
+      'old' => $object->getOldValue(),
+      'new' => $object->getNewValue(),
+      'commitPHIDs' => $commit_phids,
+    );
+  }
+
 }

From dbccfb234f8de6cf08e7946ea3b2204d2555ea0a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 10:25:05 -0800
Subject: [PATCH 07/37] Perform a client-side redirect after OAuth server
 authorization

Summary:
Ref T13099. See that task for discussion. Chrome is unhappy with an MFA form submitting to an endpoint which redirects you to an OAuth URI.

Instead, do the redirect entirely on the client.

Chrome's rationale here isn't obvious, so we may be able to revert this at some point.

Test Plan: Went through the OAuth flow locally, was redirected on the client. Will verify in production.

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19177
---
 resources/celerity/map.php                    |  6 ++++
 .../PhabricatorOAuthServerAuthController.php  | 30 +++++++++++++++++--
 webroot/rsrc/js/core/behavior-redirect.js     |  9 ++++++
 3 files changed, 42 insertions(+), 3 deletions(-)
 create mode 100644 webroot/rsrc/js/core/behavior-redirect.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index f601b5cec3..f90e3bd511 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -502,6 +502,7 @@ return array(
     'rsrc/js/core/behavior-phabricator-nav.js' => '836f966d',
     'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'acd29eee',
     'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207',
+    'rsrc/js/core/behavior-redirect.js' => '0213259f',
     'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b',
     'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e',
     'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e',
@@ -686,6 +687,7 @@ return array(
     'javelin-behavior-project-create' => '065227cc',
     'javelin-behavior-quicksand-blacklist' => '7927a7d3',
     'javelin-behavior-read-only-warning' => 'ba158207',
+    'javelin-behavior-redirect' => '0213259f',
     'javelin-behavior-refresh-csrf' => 'ab2f381b',
     'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf',
     'javelin-behavior-releeph-request-state-change' => 'a0b57eb8',
@@ -934,6 +936,10 @@ return array(
       'javelin-dom',
       'phabricator-keyboard-shortcut',
     ),
+    '0213259f' => array(
+      'javelin-behavior',
+      'javelin-uri',
+    ),
     '04b2ae03' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
index b9d916e82b..8fdd6fdb75 100644
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@@ -172,9 +172,33 @@ final class PhabricatorOAuthServerAuthController
         ));
 
       if ($client->getIsTrusted()) {
-        return id(new AphrontRedirectResponse())
-          ->setIsExternal(true)
-          ->setURI((string)$full_uri);
+        // NOTE: See T13099. We currently emit a "Content-Security-Policy"
+        // which includes a narrow "form-action". At the time of writing,
+        // Chrome applies "form-action" to redirects following form submission.
+
+        // This can lead to a situation where a user enters the OAuth workflow
+        // and is prompted for MFA. When they submit an MFA response, the form
+        // can redirect here, and Chrome will block the "Location" redirect.
+
+        // To avoid this, render an interstitial. We only actually need to do
+        // this in Chrome (but do it everywhere for consistency) and only need
+        // to do it if the request is a redirect after a form submission (but
+        // we can't tell if it is or not).
+
+        Javelin::initBehavior(
+          'redirect',
+          array(
+            'uri' => (string)$full_uri,
+          ));
+
+        return $this->newDialog()
+          ->setTitle(pht('Authenticate: %s', $name))
+          ->setRedirect(true)
+          ->appendParagraph(
+            pht(
+              'Authorization for "%s" confirmed, redirecting...',
+              phutil_tag('strong', array(), $name)))
+          ->addCancelButton((string)$full_uri, pht('Continue'));
       }
 
       // TODO: It would be nice to give the user more options here, like
diff --git a/webroot/rsrc/js/core/behavior-redirect.js b/webroot/rsrc/js/core/behavior-redirect.js
new file mode 100644
index 0000000000..4f6e234c0a
--- /dev/null
+++ b/webroot/rsrc/js/core/behavior-redirect.js
@@ -0,0 +1,9 @@
+/**
+ * @provides javelin-behavior-redirect
+ * @requires javelin-behavior
+ *           javelin-uri
+ */
+
+JX.behavior('redirect', function(config) {
+  JX.$U(config.uri).go();
+});

From 573bf15124bb9b204e72bd32ca75f0ef109188ac Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 12:12:53 -0800
Subject: [PATCH 08/37] Provide a more tailored error message when a Herald
 rule fails because of PCRE limits

Summary: Ref T13100. Since rules may begin failing for PRCE configuration reasons soon, provide a more complete explanation of possible causes in the UI.

Test Plan: Faked this, hit it via test console, saw explanation in web UI.

Maniphest Tasks: T13100

Differential Revision: https://secure.phabricator.com/D19178
---
 src/applications/herald/adapter/HeraldAdapter.php | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php
index 08a0ca7a52..a266e21f39 100644
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -490,7 +490,13 @@ abstract class HeraldAdapter extends Phobject {
           $result = @preg_match($condition_value.'S', $value);
           if ($result === false) {
             throw new HeraldInvalidConditionException(
-              pht('Regular expression is not valid!'));
+              pht(
+                'Regular expression "%s" in Herald rule "%s" is not valid, '.
+                'or exceeded backtracking or recursion limits while '.
+                'executing. Verify the expression and correct it or rewrite '.
+                'it with less backtracking.',
+                $condition_value,
+                $rule->getMonogram()));
           }
           if ($result) {
             return $result_if_match;

From e57dbcda336ac7c8e2667fa72abccc31289a8244 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 17:36:14 -0800
Subject: [PATCH 09/37] Hide "abraham landed Dxyz irresponsibly" stories from
 feed

Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.

These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.

Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.

Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".

Maniphest Tasks: T13099, T12787

Differential Revision: https://secure.phabricator.com/D19179
---
 ...ferentialRevisionWrongStateTransaction.php |  5 +--
 .../storage/ManiphestTransaction.php          | 16 ---------
 .../ManiphestTaskUnblockTransaction.php       | 10 ++++++
 .../storage/PhrictionTransaction.php          | 12 -------
 .../PhrictionDocumentMoveAwayTransaction.php  |  4 +++
 .../PhrictionDocumentMoveToTransaction.php    |  4 +++
 ...ricatorApplicationTransactionFeedStory.php | 33 ++++++++++++++++++-
 .../storage/PhabricatorModularTransaction.php |  8 +++++
 .../PhabricatorModularTransactionType.php     |  4 +++
 9 files changed, 65 insertions(+), 31 deletions(-)

diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php
index e19e54199c..47678e886f 100644
--- a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php
@@ -35,7 +35,8 @@ final class DifferentialRevisionWrongStateTransaction
       $this->renderValue($status->getDisplayName()));
   }
 
-  public function getTitleForFeed() {
-    return null;
+  public function shouldHideForFeed() {
+    return true;
   }
+
 }
diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php
index 6301241050..e2739c1d09 100644
--- a/src/applications/maniphest/storage/ManiphestTransaction.php
+++ b/src/applications/maniphest/storage/ManiphestTransaction.php
@@ -40,22 +40,6 @@ final class ManiphestTransaction
     return parent::shouldGenerateOldValue();
   }
 
-  public function shouldHideForFeed() {
-    // NOTE: Modular transactions don't currently support this, and it has
-    // very few callsites, and it's publish-time rather than display-time.
-    // This should probably become a supported, display-time behavior. For
-    // discussion, see T12787.
-
-    // Hide "alice created X, a task blocking Y." from feed because it
-    // will almost always appear adjacent to "alice created Y".
-    $is_new = $this->getMetadataValue('blocker.new');
-    if ($is_new) {
-      return true;
-    }
-
-    return parent::shouldHideForFeed();
-  }
-
   public function getRequiredHandlePHIDs() {
     $phids = parent::getRequiredHandlePHIDs();
 
diff --git a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php
index 1c7a88dec7..8833e62b79 100644
--- a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php
+++ b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php
@@ -112,5 +112,15 @@ final class ManiphestTaskUnblockTransaction
     return 'fa-shield';
   }
 
+  public function shouldHideForFeed() {
+    // Hide "alice created X, a task blocking Y." from feed because it
+    // will almost always appear adjacent to "alice created Y".
+    $is_new = $this->getMetadataValue('blocker.new');
+    if ($is_new) {
+      return true;
+    }
+
+    return parent::shouldHideForFeed();
+  }
 
 }
diff --git a/src/applications/phriction/storage/PhrictionTransaction.php b/src/applications/phriction/storage/PhrictionTransaction.php
index 2ce9d38a08..860a86be35 100644
--- a/src/applications/phriction/storage/PhrictionTransaction.php
+++ b/src/applications/phriction/storage/PhrictionTransaction.php
@@ -54,18 +54,6 @@ final class PhrictionTransaction
     return parent::shouldHideForMail($xactions);
   }
 
-  public function shouldHideForFeed() {
-    switch ($this->getTransactionType()) {
-      case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE:
-      case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE:
-        return true;
-      case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE:
-        return $this->getMetadataValue('stub:create:phid', false);
-    }
-    return parent::shouldHideForFeed();
-  }
-
-
   public function getMailTags() {
     $tags = array();
     switch ($this->getTransactionType()) {
diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php
index c827f7337d..3cd45f73b7 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php
@@ -59,4 +59,8 @@ final class PhrictionDocumentMoveAwayTransaction
     return 'fa-arrows';
   }
 
+  public function shouldHideForFeed() {
+    return true;
+  }
+
 }
diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php
index a95e70ebdb..bdf2c5d8fc 100644
--- a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php
+++ b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php
@@ -102,4 +102,8 @@ final class PhrictionDocumentMoveToTransaction
     return 'fa-arrows';
   }
 
+  public function shouldHideForFeed() {
+    return true;
+  }
+
 }
diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
index 6cc18d5181..305e38ab09 100644
--- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
+++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php
@@ -6,6 +6,8 @@
 class PhabricatorApplicationTransactionFeedStory
   extends PhabricatorFeedStory {
 
+  private $primaryTransactionPHID;
+
   public function getPrimaryObjectPHID() {
     return $this->getValue('objectPHID');
   }
@@ -27,7 +29,36 @@ class PhabricatorApplicationTransactionFeedStory
   }
 
   protected function getPrimaryTransactionPHID() {
-    return head($this->getValue('transactionPHIDs'));
+    if ($this->primaryTransactionPHID === null) {
+      // Transactions are filtered and sorted before they're stored, but the
+      // rendering logic can change between the time an edit occurs and when
+      // we actually render the story. Recalculate the filtering at display
+      // time because it's cheap and gets us better results when things change
+      // by letting the changes apply retroactively.
+
+      $xaction_phids = $this->getValue('transactionPHIDs');
+
+      $xactions = array();
+      foreach ($xaction_phids as $xaction_phid) {
+        $xactions[] = $this->getObject($xaction_phid);
+      }
+
+      foreach ($xactions as $key => $xaction) {
+        if ($xaction->shouldHideForFeed()) {
+          unset($xactions[$key]);
+        }
+      }
+
+      if ($xactions) {
+        $primary_phid = head($xactions)->getPHID();
+      } else {
+        $primary_phid = head($xaction_phids);
+      }
+
+      $this->primaryTransactionPHID = $primary_phid;
+    }
+
+    return $this->primaryTransactionPHID;
   }
 
   public function getPrimaryTransaction() {
diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php
index 9f3c24b946..fff2b842d1 100644
--- a/src/applications/transactions/storage/PhabricatorModularTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php
@@ -92,6 +92,14 @@ abstract class PhabricatorModularTransaction
     return parent::shouldHide();
   }
 
+  final public function shouldHideForFeed() {
+    if ($this->getTransactionImplementation()->shouldHideForFeed()) {
+      return true;
+    }
+
+    return parent::shouldHideForFeed();
+  }
+
   /* final */ public function getIcon() {
     $icon = $this->getTransactionImplementation()->getIcon();
     if ($icon !== null) {
diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
index f2b59c8a2b..d93831affc 100644
--- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
@@ -47,6 +47,10 @@ abstract class PhabricatorModularTransactionType
     return false;
   }
 
+  public function shouldHideForFeed() {
+    return false;
+  }
+
   public function getIcon() {
     return null;
   }

From d14a0f4787b5bc563b129b08947f964001dbecfe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 18:38:23 -0800
Subject: [PATCH 10/37] Add "All" and "With Non-Owner Author" options for all
 Owners Package autoreview rules

Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.

Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.

Maniphest Tasks: T13099, T11664

Differential Revision: https://secure.phabricator.com/D19180
---
 .../editor/DifferentialTransactionEditor.php  | 55 +++++++++++++------
 .../storage/PhabricatorOwnersPackage.php      | 25 +++++++--
 src/docs/user/userguide/owners.diviner        | 23 +++++---
 3 files changed, 72 insertions(+), 31 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 03af04dd0f..d04b0d684a 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -982,25 +982,43 @@ final class DifferentialTransactionEditor
       return array();
     }
 
-    // Remove packages that the revision author is an owner of. If you own
-    // code, you don't need another owner to review it.
-    $authority = id(new PhabricatorOwnersPackageQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs(mpull($packages, 'getPHID'))
-      ->withAuthorityPHIDs(array($object->getAuthorPHID()))
-      ->execute();
-    $authority = mpull($authority, null, 'getPHID');
+    // Identify the packages with "Non-Owner Author" review rules and remove
+    // them if the author has authority over the package.
 
-    foreach ($packages as $key => $package) {
-      $package_phid = $package->getPHID();
-      if (isset($authority[$package_phid])) {
-        unset($packages[$key]);
+    $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    $need_authority = array();
+    foreach ($packages as $package) {
+      $autoreview_setting = $package->getAutoReview();
+
+      $spec = idx($autoreview_map, $autoreview_setting);
+      if (!$spec) {
         continue;
       }
+
+      if (idx($spec, 'authority')) {
+        $need_authority[$package->getPHID()] = $package->getPHID();
+      }
     }
 
-    if (!$packages) {
-      return array();
+    if ($need_authority) {
+      $authority = id(new PhabricatorOwnersPackageQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs($need_authority)
+        ->withAuthorityPHIDs(array($object->getAuthorPHID()))
+        ->execute();
+      $authority = mpull($authority, null, 'getPHID');
+
+      foreach ($packages as $key => $package) {
+        $package_phid = $package->getPHID();
+        if (isset($authority[$package_phid])) {
+          unset($packages[$key]);
+          continue;
+        }
+      }
+
+      if (!$packages) {
+        return array();
+      }
     }
 
     $auto_subscribe = array();
@@ -1009,15 +1027,18 @@ final class DifferentialTransactionEditor
 
     foreach ($packages as $package) {
       switch ($package->getAutoReview()) {
-        case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE:
-          $auto_subscribe[] = $package;
-          break;
         case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW:
+        case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW_ALWAYS:
           $auto_review[] = $package;
           break;
         case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK:
+        case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK_ALWAYS:
           $auto_block[] = $package;
           break;
+        case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE:
+        case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE_ALWAYS:
+          $auto_subscribe[] = $package;
+          break;
         case PhabricatorOwnersPackage::AUTOREVIEW_NONE:
         default:
           break;
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index a09137c4df..60d244052f 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -33,8 +33,11 @@ final class PhabricatorOwnersPackage
 
   const AUTOREVIEW_NONE = 'none';
   const AUTOREVIEW_SUBSCRIBE = 'subscribe';
+  const AUTOREVIEW_SUBSCRIBE_ALWAYS = 'subscribe-always';
   const AUTOREVIEW_REVIEW = 'review';
+  const AUTOREVIEW_REVIEW_ALWAYS = 'review-always';
   const AUTOREVIEW_BLOCK = 'block';
+  const AUTOREVIEW_BLOCK_ALWAYS = 'block-always';
 
   const DOMINION_STRONG = 'strong';
   const DOMINION_WEAK = 'weak';
@@ -74,14 +77,26 @@ final class PhabricatorOwnersPackage
       self::AUTOREVIEW_NONE => array(
         'name' => pht('No Autoreview'),
       ),
-      self::AUTOREVIEW_SUBSCRIBE => array(
-        'name' => pht('Subscribe to Changes'),
-      ),
       self::AUTOREVIEW_REVIEW => array(
-        'name' => pht('Review Changes'),
+        'name' => pht('Review Changes With Non-Owner Author'),
+        'authority' => true,
       ),
       self::AUTOREVIEW_BLOCK => array(
-        'name' => pht('Review Changes (Blocking)'),
+        'name' => pht('Review Changes With Non-Owner Author (Blocking)'),
+        'authority' => true,
+      ),
+      self::AUTOREVIEW_SUBSCRIBE => array(
+        'name' => pht('Subscribe to Changes With Non-Owner Author'),
+        'authority' => true,
+      ),
+      self::AUTOREVIEW_REVIEW_ALWAYS => array(
+        'name' => pht('Review All Changes'),
+      ),
+      self::AUTOREVIEW_BLOCK_ALWAYS => array(
+        'name' => pht('Review All Changes (Blocking)'),
+      ),
+      self::AUTOREVIEW_SUBSCRIBE_ALWAYS => array(
+        'name' => pht('Subscribe to All Changes'),
       ),
     );
   }
diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner
index f30fe5023c..35260b08af 100644
--- a/src/docs/user/userguide/owners.diviner
+++ b/src/docs/user/userguide/owners.diviner
@@ -84,21 +84,26 @@ You can configure **Auto Review** for packages. When a new code review is
 created in Differential which affects code in a package, the package can
 automatically be added as a subscriber or reviewer.
 
-The available settings are:
+The available settings allow you to take these actions:
 
-  - **No Autoreview**: This package will not be added to new reviews.
-  - **Subscribe to Changes**: This package will be added to reviews as a
-    subscriber. Owners will be notified of changes, but not required to act.
   - **Review Changes**: This package will be added to reviews as a reviewer.
     Reviews will appear on the dashboards of package owners.
-  - **Review Changes (Blocking)** This package will be added to reviews
-    as a blocking reviewer. A package owner will be required to accept changes
+  - **Review Changes (Blocking)** This package will be added to reviews as a
+    blocking reviewer. A package owner will be required to accept changes
     before they may land.
+  - **Subscribe to Changes**: This package will be added to reviews as a
+    subscriber. Owners will be notified of changes, but not required to act.
 
-NOTE: These rules **do not trigger** if the change author is a package owner.
-They only apply to changes made by users who aren't already owners.
+If you select the **With Non-Owner Author** option for these actions, the
+action will not trigger if the author of the revision is a package owner. This
+mode may be helpful if you are using Owners mostly to make sure that someone
+who is qualified is involved in each change to a piece of code.
 
-These rules also do not trigger if the package has been archived.
+If you select the **All** option for these actions, the action will always
+trigger even if the author is a package owner. This mode may be helpful if you
+are using Owners mostly to suggest reviewers.
+
+These rules do not trigger if the package has been archived.
 
 The intent of this feature is to make it easy to configure simple, reasonable
 behaviors. If you want more tailored or specific triggers, you can write more

From 1bf4422c749c8a6777e4e77eb7806c8dcad6e2b8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 19:33:49 -0800
Subject: [PATCH 11/37] Add and populate a `pathIndex` column for OwnersPath

Summary: Ref T11015. This supports making path names arbitrarily long and putting a proper unique key on the table.

Test Plan:
  - Migrated, checked database, saw nice digested indexes.
  - Edited a package, saw new rows update with digested indexes.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19181
---
 .../autopatches/20180306.opath.01.digest.sql  |  2 ++
 .../20180306.opath.02.digestpopulate.php      | 19 +++++++++++++++++++
 .../owners/storage/PhabricatorOwnersPath.php  | 11 +++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20180306.opath.01.digest.sql
 create mode 100644 resources/sql/autopatches/20180306.opath.02.digestpopulate.php

diff --git a/resources/sql/autopatches/20180306.opath.01.digest.sql b/resources/sql/autopatches/20180306.opath.01.digest.sql
new file mode 100644
index 0000000000..2418366fc5
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.01.digest.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_path
+  ADD pathIndex BINARY(12) NOT NULL;
diff --git a/resources/sql/autopatches/20180306.opath.02.digestpopulate.php b/resources/sql/autopatches/20180306.opath.02.digestpopulate.php
new file mode 100644
index 0000000000..a6b817fc46
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.02.digestpopulate.php
@@ -0,0 +1,19 @@
+<?php
+
+$table = new PhabricatorOwnersPath();
+$conn = $table->establishConnection('w');
+
+foreach (new LiskMigrationIterator($table) as $path) {
+  $index = PhabricatorHash::digestForIndex($path->getPath());
+
+  if ($index === $path->getPathIndex()) {
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'UPDATE %T SET pathIndex = %s WHERE id = %d',
+    $table->getTableName(),
+    $index,
+    $path->getID());
+}
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index f240db2851..47f7faa2b3 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -4,6 +4,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
 
   protected $packageID;
   protected $repositoryPHID;
+  protected $pathIndex;
   protected $path;
   protected $excluded;
 
@@ -15,6 +16,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_COLUMN_SCHEMA => array(
         'path' => 'text255',
+        'pathIndex' => 'bytes12',
         'excluded' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
@@ -25,12 +27,17 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
     ) + parent::getConfiguration();
   }
 
-
   public static function newFromRef(array $ref) {
     $path = new PhabricatorOwnersPath();
     $path->repositoryPHID = $ref['repositoryPHID'];
-    $path->path = $ref['path'];
+
+    $raw_path = $ref['path'];
+
+    $path->pathIndex = PhabricatorHash::digestForIndex($raw_path);
+    $path->path = $raw_path;
+
     $path->excluded = $ref['excluded'];
+
     return $path;
   }
 

From 8cb273a0532b128b75b3a30aefe3a2a2c1ea36e2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 19:42:02 -0800
Subject: [PATCH 12/37] Add a unique key to OwnersPath on "<packageID,
 repositoryPHID, pathIndex>"

Summary:
Depends on D19181. Ref T11015. This nukes duplicates from the table if they exist, then adds a unique key.

(Duplicates should not exist and can not be added with any recent version of the web UI.)

Test Plan:
  - Tried to add duplicates with web UI, didn't have any luck.
  - Explicitly added duplicates with manual `INSERT`s.
  - Viewed packages in web UI and saw duplicates.
  - Ran migrations, got a clean purge and a nice unique key.
  - There's still no way to actually hit a duplicate key error in the UI (unless you can collide hashes, I suppose), this is purely a correctness/robustness change.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19182
---
 .../autopatches/20180306.opath.03.purge.php   | 22 +++++++++++++++++++
 .../autopatches/20180306.opath.04.unique.sql  |  2 ++
 .../owners/storage/PhabricatorOwnersPath.php  |  5 +++--
 3 files changed, 27 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20180306.opath.03.purge.php
 create mode 100644 resources/sql/autopatches/20180306.opath.04.unique.sql

diff --git a/resources/sql/autopatches/20180306.opath.03.purge.php b/resources/sql/autopatches/20180306.opath.03.purge.php
new file mode 100644
index 0000000000..91a15e2a58
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.03.purge.php
@@ -0,0 +1,22 @@
+<?php
+
+$table = new PhabricatorOwnersPath();
+$conn = $table->establishConnection('w');
+
+$seen = array();
+foreach (new LiskMigrationIterator($table) as $path) {
+  $package_id = $path->getPackageID();
+  $repository_phid = $path->getRepositoryPHID();
+  $path_index = $path->getPathIndex();
+
+  if (!isset($seen[$package_id][$repository_phid][$path_index])) {
+    $seen[$package_id][$repository_phid][$path_index] = true;
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'DELETE FROM %T WHERE id = %d',
+    $table->getTableName(),
+    $path->getID());
+}
diff --git a/resources/sql/autopatches/20180306.opath.04.unique.sql b/resources/sql/autopatches/20180306.opath.04.unique.sql
new file mode 100644
index 0000000000..2349533b1f
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.04.unique.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_path
+  ADD UNIQUE KEY `key_path` (packageID, repositoryPHID, pathIndex);
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index 47f7faa2b3..7e7822df1a 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -20,8 +20,9 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
         'excluded' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
-        'packageID' => array(
-          'columns' => array('packageID'),
+        'key_path' => array(
+          'columns' => array('packageID', 'repositoryPHID', 'pathIndex'),
+          'unique' => true,
         ),
       ),
     ) + parent::getConfiguration();

From adde4089b49b71f7761a02c66b43eb807f86747f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 19:50:09 -0800
Subject: [PATCH 13/37] Allow owners paths to be arbitrarily long and add
 storage for display paths

Summary:
Depends on D19182. Ref T11015. This changes `path` from `text255` to `longtext` because paths may be arbitrarily long.

It adds `pathDisplay` to prepare for display paths and storage paths having different values. For now, `pathDisplay` is copied from `path` and always has the same value.

Test Plan:
  - Ran migration, checked database for sanity (all `pathDisplay` and `path` values identical).
  - Added new paths, saw `pathDisplay` and `path` get the same values.
  - Added an unreasonably enormous path with far more than 255 characters.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19183
---
 resources/sql/autopatches/20180306.opath.05.longpath.sql    | 2 ++
 resources/sql/autopatches/20180306.opath.06.pathdisplay.sql | 2 ++
 resources/sql/autopatches/20180306.opath.07.copypaths.sql   | 2 ++
 src/applications/owners/storage/PhabricatorOwnersPath.php   | 5 ++++-
 4 files changed, 10 insertions(+), 1 deletion(-)
 create mode 100644 resources/sql/autopatches/20180306.opath.05.longpath.sql
 create mode 100644 resources/sql/autopatches/20180306.opath.06.pathdisplay.sql
 create mode 100644 resources/sql/autopatches/20180306.opath.07.copypaths.sql

diff --git a/resources/sql/autopatches/20180306.opath.05.longpath.sql b/resources/sql/autopatches/20180306.opath.05.longpath.sql
new file mode 100644
index 0000000000..79ff2f7a7f
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.05.longpath.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_path
+  CHANGE path path LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT};
diff --git a/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql b/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql
new file mode 100644
index 0000000000..b9b336ecd7
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.06.pathdisplay.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_path
+  ADD pathDisplay LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT};
diff --git a/resources/sql/autopatches/20180306.opath.07.copypaths.sql b/resources/sql/autopatches/20180306.opath.07.copypaths.sql
new file mode 100644
index 0000000000..74ebecfa9a
--- /dev/null
+++ b/resources/sql/autopatches/20180306.opath.07.copypaths.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_owners.owners_path
+  SET pathDisplay = path WHERE pathDisplay = '';
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index 7e7822df1a..6c49023e69 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -6,6 +6,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
   protected $repositoryPHID;
   protected $pathIndex;
   protected $path;
+  protected $pathDisplay;
   protected $excluded;
 
   private $fragments;
@@ -15,7 +16,8 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
     return array(
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_COLUMN_SCHEMA => array(
-        'path' => 'text255',
+        'path' => 'text',
+        'pathDisplay' => 'text',
         'pathIndex' => 'bytes12',
         'excluded' => 'bool',
       ),
@@ -36,6 +38,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
 
     $path->pathIndex = PhabricatorHash::digestForIndex($raw_path);
     $path->path = $raw_path;
+    $path->pathDisplay = $raw_path;
 
     $path->excluded = $ref['excluded'];
 

From df1e9ce646c03a9e23be5b1f254a1b67a0b24c0e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 20:07:32 -0800
Subject: [PATCH 14/37] Treat Owners paths like "/src/backend" and
 "/src/backend/" identically

Summary:
Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work.

Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed.

Test Plan:
  - Entered "/src/main.c", "/src/main.c/", saw them de-duplicate.
  - Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally.
  - Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it.
  - Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19184
---
 resources/celerity/map.php                    | 20 ++++++------
 .../PhabricatorOwnersDetailController.php     |  4 +--
 ...catorOwnersPathsSearchEngineAttachment.php |  2 +-
 .../owners/storage/PhabricatorOwnersPath.php  |  1 +
 ...abricatorOwnersPackagePathsTransaction.php | 32 +++++++++++++++++++
 .../js/application/owners/OwnersPathEditor.js |  2 +-
 6 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index f90e3bd511..a6435d6afd 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -424,7 +424,7 @@ return array(
     'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876',
     'rsrc/js/application/maniphest/behavior-list-edit.js' => 'a9f88de2',
     'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '71237763',
-    'rsrc/js/application/owners/OwnersPathEditor.js' => 'aa1733d0',
+    'rsrc/js/application/owners/OwnersPathEditor.js' => '996d62b9',
     'rsrc/js/application/owners/owners-path-editor.js' => '7a68dda3',
     'rsrc/js/application/passphrase/passphrase-credential-control.js' => '3cb0b2fc',
     'rsrc/js/application/pholio/behavior-pholio-mock-edit.js' => 'bee502c8',
@@ -764,7 +764,7 @@ return array(
     'maniphest-task-edit-css' => 'fda62a9b',
     'maniphest-task-summary-css' => '11cc5344',
     'multirow-row-manager' => 'b5d57730',
-    'owners-path-editor' => 'aa1733d0',
+    'owners-path-editor' => '996d62b9',
     'owners-path-editor-css' => '2f00933b',
     'paste-css' => '9fcc9773',
     'path-typeahead' => 'f7fc67ec',
@@ -1676,6 +1676,14 @@ return array(
       'javelin-mask',
       'phabricator-drag-and-drop-file-upload',
     ),
+    '996d62b9' => array(
+      'multirow-row-manager',
+      'javelin-install',
+      'path-typeahead',
+      'javelin-dom',
+      'javelin-util',
+      'phabricator-prefab',
+    ),
     '9a6dd75c' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1766,14 +1774,6 @@ return array(
       'javelin-fx',
       'javelin-util',
     ),
-    'aa1733d0' => array(
-      'multirow-row-manager',
-      'javelin-install',
-      'path-typeahead',
-      'javelin-dom',
-      'javelin-util',
-      'phabricator-prefab',
-    ),
     'ab2f381b' => array(
       'javelin-request',
       'javelin-behavior',
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index 3c6ae3836c..c7e96594f3 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -279,7 +279,7 @@ final class PhabricatorOwnersDetailController
       $href = $repo->generateURI(
         array(
           'branch'   => $repo->getDefaultBranch(),
-          'path'     => $path->getPath(),
+          'path'     => $path->getPathDisplay(),
           'action'   => 'browse',
         ));
 
@@ -288,7 +288,7 @@ final class PhabricatorOwnersDetailController
         array(
           'href' => (string)$href,
         ),
-        $path->getPath());
+        $path->getPathDisplay());
 
       $rows[] = array(
         ($path->getExcluded() ? '-' : '+'),
diff --git a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php
index ad5415ef69..19e699e8ff 100644
--- a/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php
+++ b/src/applications/owners/engineextension/PhabricatorOwnersPathsSearchEngineAttachment.php
@@ -22,7 +22,7 @@ final class PhabricatorOwnersPathsSearchEngineAttachment
     foreach ($paths as $path) {
       $list[] = array(
         'repositoryPHID' => $path->getRepositoryPHID(),
-        'path' => $path->getPath(),
+        'path' => $path->getPathDisplay(),
         'excluded' => (bool)$path->getExcluded(),
       );
     }
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index 6c49023e69..0ef7fce59b 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -49,6 +49,7 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
     return array(
       'repositoryPHID' => $this->getRepositoryPHID(),
       'path' => $this->getPath(),
+      'display' => $this->getPathDisplay(),
       'excluded' => (int)$this->getExcluded(),
     );
   }
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
index 5952b78aed..984b26b8ac 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php
@@ -103,6 +103,26 @@ final class PhabricatorOwnersPackagePathsTransaction
 
     $paths = $object->getPaths();
 
+    // We store paths in a normalized format with a trailing slash, regardless
+    // of whether the user enters "path/to/file.c" or "src/backend/". Normalize
+    // paths now.
+
+    $display_map = array();
+    foreach ($new as $key => $spec) {
+      $display_path = $spec['path'];
+      $raw_path = rtrim($display_path, '/').'/';
+
+      // If the user entered two paths which normalize to the same value
+      // (like "src/main.c" and "src/main.c/"), discard the duplicates.
+      if (isset($display_map[$raw_path])) {
+        unset($new[$key]);
+        continue;
+      }
+
+      $new[$key]['path'] = $raw_path;
+      $display_map[$raw_path] = $display_path;
+    }
+
     $diffs = PhabricatorOwnersPath::getTransactionValueChanges($old, $new);
     list($rem, $add) = $diffs;
 
@@ -111,12 +131,24 @@ final class PhabricatorOwnersPackagePathsTransaction
       $ref = $path->getRef();
       if (PhabricatorOwnersPath::isRefInSet($ref, $set)) {
         $path->delete();
+        continue;
+      }
+
+      // If the user has changed the display value for a path but the raw
+      // storage value hasn't changed, update the display value.
+
+      if (isset($display_map[$path->getPath()])) {
+        $path
+          ->setPathDisplay($display_map[$path->getPath()])
+          ->save();
+        continue;
       }
     }
 
     foreach ($add as $ref) {
       $path = PhabricatorOwnersPath::newFromRef($ref)
         ->setPackageID($object->getID())
+        ->setPathDisplay($display_map[$ref['path']])
         ->save();
     }
   }
diff --git a/webroot/rsrc/js/application/owners/OwnersPathEditor.js b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
index 3199ac4e2a..f1d47a7908 100644
--- a/webroot/rsrc/js/application/owners/OwnersPathEditor.js
+++ b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
@@ -115,7 +115,7 @@ JX.install('OwnersPathEditor', {
       JX.copy(
         path_input,
         {
-          value : path_ref.path || '',
+          value : path_ref.display || '',
           name : 'path[' + this._count + ']'
         });
 

From 516aaad3410c242b47ee270b4cdca8e56f6b379b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 20:25:17 -0800
Subject: [PATCH 15/37] Use "pathIndex" in some owners package queries to
 improve query plans

Summary: Depends on D19184. Ref T11015. Now that we have a digest index column, we can improve some of the queries a bit.

Test Plan:
  - Ran queries from revision pages before and after with and without EXPLAIN.
  - Saw the same results with much better EXPLAIN plans.
  - Fragment size is now fixed at 12 bytes per fragment, so we can shove more of them in a single query.

Maniphest Tasks: T11015

Differential Revision: https://secure.phabricator.com/D19185
---
 .../query/PhabricatorOwnersPackageQuery.php   | 20 ++++++++++++++-----
 .../storage/PhabricatorOwnersPackage.php      | 11 +++++++---
 .../owners/storage/PhabricatorOwnersPath.php  |  3 +++
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
index ce411aad35..ff0665b344 100644
--- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
+++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
@@ -206,8 +206,8 @@ final class PhabricatorOwnersPackageQuery
     if ($this->paths !== null) {
       $where[] = qsprintf(
         $conn,
-        'rpath.path IN (%Ls)',
-        $this->getFragmentsForPaths($this->paths));
+        'rpath.pathIndex IN (%Ls)',
+        $this->getFragmentIndexesForPaths($this->paths));
     }
 
     if ($this->statuses !== null) {
@@ -220,13 +220,13 @@ final class PhabricatorOwnersPackageQuery
     if ($this->controlMap) {
       $clauses = array();
       foreach ($this->controlMap as $repository_phid => $paths) {
-        $fragments = $this->getFragmentsForPaths($paths);
+        $indexes = $this->getFragmentIndexesForPaths($paths);
 
         $clauses[] = qsprintf(
           $conn,
-          '(rpath.repositoryPHID = %s AND rpath.path IN (%Ls))',
+          '(rpath.repositoryPHID = %s AND rpath.pathIndex IN (%Ls))',
           $repository_phid,
-          $fragments);
+          $indexes);
       }
       $where[] = implode(' OR ', $clauses);
     }
@@ -333,6 +333,16 @@ final class PhabricatorOwnersPackageQuery
     return $fragments;
   }
 
+  private function getFragmentIndexesForPaths(array $paths) {
+    $indexes = array();
+
+    foreach ($this->getFragmentsForPaths($paths) as $fragment) {
+      $indexes[] = PhabricatorHash::digestForIndex($fragment);
+    }
+
+    return $indexes;
+  }
+
 
 /* -(  Path Control  )------------------------------------------------------- */
 
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 60d244052f..7f155d7c89 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -218,15 +218,20 @@ final class PhabricatorOwnersPackage
     // and then merge results in PHP.
 
     $rows = array();
-    foreach (array_chunk(array_keys($fragments), 128) as $chunk) {
+    foreach (array_chunk(array_keys($fragments), 1024) as $chunk) {
+      $indexes = array();
+      foreach ($chunk as $fragment) {
+        $indexes[] = PhabricatorHash::digestForIndex($fragment);
+      }
+
       $rows[] = queryfx_all(
         $conn,
         'SELECT pkg.id, pkg.dominion, p.excluded, p.path
           FROM %T pkg JOIN %T p ON p.packageID = pkg.id
-          WHERE p.path IN (%Ls) AND pkg.status IN (%Ls) %Q',
+          WHERE p.pathIndex IN (%Ls) AND pkg.status IN (%Ls) %Q',
         $package->getTableName(),
         $path->getTableName(),
-        $chunk,
+        $indexes,
         array(
           self::STATUS_ACTIVE,
         ),
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index 0ef7fce59b..7022cb887c 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -26,6 +26,9 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
           'columns' => array('packageID', 'repositoryPHID', 'pathIndex'),
           'unique' => true,
         ),
+        'key_repository' => array(
+          'columns' => array('repositoryPHID', 'pathIndex'),
+        ),
       ),
     ) + parent::getConfiguration();
   }

From 9462f8aa8908c3e81352dd149c362bfeed9e2204 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 6 Mar 2018 20:40:16 -0800
Subject: [PATCH 16/37] Remove client OAuth redirect code which was only
 partially cleaned up

See T13099. I took a different approach here but didn't fully clean up
the old one.
---
 .../controller/PhabricatorOAuthServerAuthController.php          | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
index 8fdd6fdb75..745be3e820 100644
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@@ -193,7 +193,6 @@ final class PhabricatorOAuthServerAuthController
 
         return $this->newDialog()
           ->setTitle(pht('Authenticate: %s', $name))
-          ->setRedirect(true)
           ->appendParagraph(
             pht(
               'Authorization for "%s" confirmed, redirecting...',

From 28854ae8121b02ab0d78997cf416ec562afb01c3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 06:20:18 -0800
Subject: [PATCH 17/37] Return a integer JSON type from "*.edit" endpoints for
 the object ID

Summary: See PHI425. See T12678. This should be an integer, but may be a string.

Test Plan: Called `differential.revision.edit`, observed integer in result instead of string.

Differential Revision: https://secure.phabricator.com/D19186
---
 .../transactions/editengine/PhabricatorEditEngine.php           | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
index ea171a421c..de8139b145 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -2092,7 +2092,7 @@ abstract class PhabricatorEditEngine
 
     return array(
       'object' => array(
-        'id' => $object->getID(),
+        'id' => (int)$object->getID(),
         'phid' => $object->getPHID(),
       ),
       'transactions' => $xactions_struct,

From c6a042b59acfb0a5dd3c2b8432a2eea6aa0e7eec Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 07:02:34 -0800
Subject: [PATCH 18/37] Correct line highlighting behavior in Diffusion

Summary: See <https://discourse.phabricator-community.org/t/line-highlighting-in-diffusion-breaks-url/1207>. Ref T13088. This was disrupted by changes for the new Harbormaster build logs and now needs an explicit base URI.

Test Plan: Clicked lines and dragged across line ranges in Diffusion, observed correct URI behavior.

Maniphest Tasks: T13088

Differential Revision: https://secure.phabricator.com/D19187
---
 .../controller/DiffusionBrowseController.php   | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 8df5579a62..ada75f688a 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -593,6 +593,9 @@ final class DiffusionBrowseController extends DiffusionController {
       array(
         'class' => 'diffusion-source remarkup-code PhabricatorMonospaced',
         'sigil' => 'phabricator-source',
+        'meta' => array(
+          'uri' => $this->getLineNumberBaseURI(),
+        ),
       ),
       $rows);
 
@@ -1126,11 +1129,7 @@ final class DiffusionBrowseController extends DiffusionController {
 
     // NOTE: We're doing this manually because rendering is otherwise
     // dominated by URI generation for very large files.
-    $line_base = (string)$drequest->generateURI(
-      array(
-        'action'  => 'browse',
-        'stable'  => true,
-      ));
+    $line_base = $this->getLineNumberBaseURI();
 
     require_celerity_resource('aphront-tooltip-css');
     Javelin::initBehavior('phabricator-oncopy');
@@ -2039,4 +2038,13 @@ final class DiffusionBrowseController extends DiffusionController {
       ->setTable($history_table);
   }
 
+  private function getLineNumberBaseURI() {
+    $drequest = $this->getDiffusionRequest();
+
+    return (string)$drequest->generateURI(
+      array(
+        'action'  => 'browse',
+        'stable'  => true,
+      ));
+  }
 }

From 229d4677701aa50fa6feebea2e3d791771f86482 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 07:23:47 -0800
Subject: [PATCH 19/37] Restore lightbox behavior for thumbnailed images

Summary: Ref T13099. See <https://discourse.phabricator-community.org/t/lightbox-not-working/1201>. The Content-Security-Policy changes rewrote some of this code and the handling for "Download" links is incorrectly catching clicks on thumbnailed images.

Test Plan: Clicked a thumbnailed image, got a lightbox. Command-clicked a download link, still got link behavior instead of a lightbox.

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19188
---
 resources/celerity/map.php                    | 24 +++++++++----------
 .../js/core/behavior-lightbox-attachments.js  |  5 ++--
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index a6435d6afd..f4a855da99 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,7 +10,7 @@ return array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
     'core.pkg.css' => '2fa91e14',
-    'core.pkg.js' => 'a3ceffdb',
+    'core.pkg.js' => 'e4d73c62',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '113e692c',
     'differential.pkg.js' => 'f6d809c0',
@@ -494,7 +494,7 @@ return array(
     'rsrc/js/core/behavior-hovercard.js' => 'bcaccd64',
     'rsrc/js/core/behavior-keyboard-pager.js' => 'a8da01f0',
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0',
-    'rsrc/js/core/behavior-lightbox-attachments.js' => 'e31fad01',
+    'rsrc/js/core/behavior-lightbox-attachments.js' => '562bcce0',
     'rsrc/js/core/behavior-line-linker.js' => 'a9b946f8',
     'rsrc/js/core/behavior-more.js' => 'a80d0378',
     'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0',
@@ -643,7 +643,7 @@ return array(
     'javelin-behavior-history-install' => '7ee2b591',
     'javelin-behavior-icon-composer' => '8499b6ab',
     'javelin-behavior-launch-icon-composer' => '48086888',
-    'javelin-behavior-lightbox-attachments' => 'e31fad01',
+    'javelin-behavior-lightbox-attachments' => '562bcce0',
     'javelin-behavior-line-chart' => 'e4232876',
     'javelin-behavior-load-blame' => '42126667',
     'javelin-behavior-maniphest-batch-selector' => 'ad54037e',
@@ -1362,6 +1362,15 @@ return array(
       'javelin-vector',
       'javelin-dom',
     ),
+    '562bcce0' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-mask',
+      'javelin-util',
+      'phuix-icon-view',
+      'phabricator-busy',
+    ),
     '58dea2fa' => array(
       'javelin-install',
       'javelin-util',
@@ -2067,15 +2076,6 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
-    'e31fad01' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-mask',
-      'javelin-util',
-      'phuix-icon-view',
-      'phabricator-busy',
-    ),
     'e379b58e' => array(
       'javelin-behavior',
       'javelin-stratcom',
diff --git a/webroot/rsrc/js/core/behavior-lightbox-attachments.js b/webroot/rsrc/js/core/behavior-lightbox-attachments.js
index eb31d22c60..cacec257b3 100644
--- a/webroot/rsrc/js/core/behavior-lightbox-attachments.js
+++ b/webroot/rsrc/js/core/behavior-lightbox-attachments.js
@@ -48,8 +48,9 @@ JX.behavior('lightbox-attachments', function() {
     }
 
     // If you click the "Download" link inside an embedded file element,
-    // don't lightbox the file.
-    if (e.getNode('tag:a')) {
+    // don't lightbox the file. But do lightbox when the user clicks an
+    // "<img />" inside an "<a />".
+    if (e.getNode('tag:a') && !e.getNode('tag:img')) {
       return;
     }
 

From ab0ac7f61bab621daee1067a837d06166c628ace Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 16:41:35 -0800
Subject: [PATCH 20/37] Remove very old "owners-default-path" code from Owners

Summary: Ref T12590. This is ancient code which was used to prefill `/trunk/tfb/www/` or similar at Facebook. I don't think it ever had a UI and no install has asked for this feature since 2011.

Test Plan: Grepped for affected symbols, edited paths in Owners.

Maniphest Tasks: T12590

Differential Revision: https://secure.phabricator.com/D19189
---
 resources/celerity/map.php                    | 40 +++++++++----------
 .../PhabricatorOwnersPathsController.php      | 11 -----
 .../js/application/herald/PathTypeahead.js    |  7 +---
 .../js/application/owners/OwnersPathEditor.js |  7 +---
 4 files changed, 23 insertions(+), 42 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index f4a855da99..8bccfbb406 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -418,13 +418,13 @@ return array(
     'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888',
     'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909',
     'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e',
-    'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec',
+    'rsrc/js/application/herald/PathTypeahead.js' => '78039abe',
     'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3',
     'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'ad54037e',
     'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876',
     'rsrc/js/application/maniphest/behavior-list-edit.js' => 'a9f88de2',
     'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '71237763',
-    'rsrc/js/application/owners/OwnersPathEditor.js' => '996d62b9',
+    'rsrc/js/application/owners/OwnersPathEditor.js' => '52b9cbc4',
     'rsrc/js/application/owners/owners-path-editor.js' => '7a68dda3',
     'rsrc/js/application/passphrase/passphrase-credential-control.js' => '3cb0b2fc',
     'rsrc/js/application/pholio/behavior-pholio-mock-edit.js' => 'bee502c8',
@@ -764,10 +764,10 @@ return array(
     'maniphest-task-edit-css' => 'fda62a9b',
     'maniphest-task-summary-css' => '11cc5344',
     'multirow-row-manager' => 'b5d57730',
-    'owners-path-editor' => '996d62b9',
+    'owners-path-editor' => '52b9cbc4',
     'owners-path-editor-css' => '2f00933b',
     'paste-css' => '9fcc9773',
-    'path-typeahead' => 'f7fc67ec',
+    'path-typeahead' => '78039abe',
     'people-picture-menu-item-css' => 'a06f7f34',
     'people-profile-css' => '4df76faf',
     'phabricator-action-list-view-css' => '0bcd9a45',
@@ -1337,6 +1337,14 @@ return array(
       'javelin-vector',
       'javelin-typeahead-static-source',
     ),
+    '52b9cbc4' => array(
+      'multirow-row-manager',
+      'javelin-install',
+      'path-typeahead',
+      'javelin-dom',
+      'javelin-util',
+      'phabricator-prefab',
+    ),
     '54b612ba' => array(
       'javelin-color',
       'javelin-install',
@@ -1534,6 +1542,14 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
+    '78039abe' => array(
+      'javelin-install',
+      'javelin-typeahead',
+      'javelin-dom',
+      'javelin-request',
+      'javelin-typeahead-ondemand-source',
+      'javelin-util',
+    ),
     '7927a7d3' => array(
       'javelin-behavior',
       'javelin-quicksand',
@@ -1685,14 +1701,6 @@ return array(
       'javelin-mask',
       'phabricator-drag-and-drop-file-upload',
     ),
-    '996d62b9' => array(
-      'multirow-row-manager',
-      'javelin-install',
-      'path-typeahead',
-      'javelin-dom',
-      'javelin-util',
-      'phabricator-prefab',
-    ),
     '9a6dd75c' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2163,14 +2171,6 @@ return array(
       'javelin-util',
       'javelin-reactor',
     ),
-    'f7fc67ec' => array(
-      'javelin-install',
-      'javelin-typeahead',
-      'javelin-dom',
-      'javelin-request',
-      'javelin-typeahead-ondemand-source',
-      'javelin-util',
-    ),
     'f829edb3' => array(
       'javelin-view',
       'javelin-install',
diff --git a/src/applications/owners/controller/PhabricatorOwnersPathsController.php b/src/applications/owners/controller/PhabricatorOwnersPathsController.php
index e5463b4cb4..ec7322a7c0 100644
--- a/src/applications/owners/controller/PhabricatorOwnersPathsController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersPathsController.php
@@ -74,15 +74,6 @@ final class PhabricatorOwnersPathsController
       ->setViewer($viewer)
       ->execute();
 
-    $default_paths = array();
-    foreach ($repos as $repo) {
-      $default_path = $repo->getDetail('default-owners-path');
-      if ($default_path) {
-        $default_paths[$repo->getPHID()] = $default_path;
-      }
-    }
-
-
     $repo_map = array();
     foreach ($repos as $key => $repo) {
       $monogram = $repo->getMonogram();
@@ -106,8 +97,6 @@ final class PhabricatorOwnersPathsController
 
         'completeURI'         => '/diffusion/services/path/complete/',
         'validateURI'         => '/diffusion/services/path/validate/',
-
-        'repositoryDefaultPaths' => $default_paths,
       ));
 
     require_celerity_resource('owners-path-editor-css');
diff --git a/webroot/rsrc/js/application/herald/PathTypeahead.js b/webroot/rsrc/js/application/herald/PathTypeahead.js
index 988d728ced..5df0263086 100644
--- a/webroot/rsrc/js/application/herald/PathTypeahead.js
+++ b/webroot/rsrc/js/application/herald/PathTypeahead.js
@@ -17,12 +17,7 @@ JX.install('PathTypeahead', {
     this._completeURI = config.completeURI;
     this._validateURI = config.validateURI;
     this._errorDisplay = config.error_display;
-
-    /*
-     * Default values to preload the typeahead with, for extremely common
-     * cases.
-     */
-    this._textInputValues = config.repositoryDefaultPaths;
+    this._textInputValues = {};
 
     this._initializeDatasource();
     this._initializeTypeahead(this._input);
diff --git a/webroot/rsrc/js/application/owners/OwnersPathEditor.js b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
index f1d47a7908..75dff4c71f 100644
--- a/webroot/rsrc/js/application/owners/OwnersPathEditor.js
+++ b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
@@ -28,7 +28,6 @@ JX.install('OwnersPathEditor', {
 
     this._completeURI = config.completeURI;
     this._validateURI = config.validateURI;
-    this._repositoryDefaultPaths = config.repositoryDefaultPaths;
 
     this._initializePaths(config.pathRefs);
   },
@@ -67,8 +66,6 @@ JX.install('OwnersPathEditor', {
      */
     _lastRepositoryChoice : null,
 
-    _repositoryDefaultPaths : null,
-
     /*
      * Initialize with 0 or more rows.
      * Adds one initial row if none are given.
@@ -144,13 +141,13 @@ JX.install('OwnersPathEditor', {
         [exclude_cell, repo_cell, typeahead_cell, error_display_cell]);
 
       new JX.PathTypeahead({
-        repositoryDefaultPaths : this._repositoryDefaultPaths,
         repo_select : repo_select,
         path_input : path_input,
         hardpoint : hardpoint,
         error_display : error_display,
         completeURI : this._completeURI,
-        validateURI : this._validateURI}).start();
+        validateURI : this._validateURI
+      }).start();
 
       this._count++;
       return row;

From b41a0e6ddda3495eed2904e355862e39721ae64e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 17:31:39 -0800
Subject: [PATCH 21/37] Fix broken suggestion/validation for Owners paths in
 repositories with short names

Summary:
Depends on D19189. Ref T12590. The "validate" and "complete" endpoints for this UI could incorrectly return redirect responses. These aren't critical to the behavior of Owners, but they're nice to have, and shouldn't redirect.

Instead, skip the canonicalizing redirect for AJAX requests.

Test Plan: Edited Owners paths in a repository with a short name, got completion/validation again.

Maniphest Tasks: T12590

Differential Revision: https://secure.phabricator.com/D19190
---
 .../controller/DiffusionController.php          | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php
index 7ae67c2b5d..a220ac05e0 100644
--- a/src/applications/diffusion/controller/DiffusionController.php
+++ b/src/applications/diffusion/controller/DiffusionController.php
@@ -69,13 +69,18 @@ abstract class DiffusionController extends PhabricatorController {
     // repository has a different canonical path like "/diffusion/XYZ/...",
     // redirect them to the canonical path.
 
-    $request_path = $request->getPath();
-    $repository = $drequest->getRepository();
+    // Skip this redirect if the request is an AJAX request, like the requests
+    // that Owners makes to complete and validate paths.
 
-    $canonical_path = $repository->getCanonicalPath($request_path);
-    if ($canonical_path !== null) {
-      if ($canonical_path != $request_path) {
-        return id(new AphrontRedirectResponse())->setURI($canonical_path);
+    if (!$request->isAjax()) {
+      $request_path = $request->getPath();
+      $repository = $drequest->getRepository();
+
+      $canonical_path = $repository->getCanonicalPath($request_path);
+      if ($canonical_path !== null) {
+        if ($canonical_path != $request_path) {
+          return id(new AphrontRedirectResponse())->setURI($canonical_path);
+        }
       }
     }
 

From a4cc1373d3a2f4c589bf772088e111301850ba77 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Mar 2018 17:29:06 -0800
Subject: [PATCH 22/37] Use a tokenizer, not a gigantic poorly-ordered "<select
 />", to choose repositories in Owners

Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.

Test Plan:
  - Edited paths: include/exclude paths, from different repositories, different actual paths.
  - Used "Add New Path" to add rows, got repository selector prepopulated with last value.
  - Used "remove".
  - Used validation typeahead, got reasonable behaviors?

The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.

Maniphest Tasks: T13099, T12590

Differential Revision: https://secure.phabricator.com/D19191
---
 resources/celerity/map.php                    |  75 +++----
 .../DiffusionPathValidateController.php       |  13 --
 .../PhabricatorOwnersPathsController.php      |  68 ++++--
 .../application/owners/owners-path-editor.css |  40 ++--
 .../typeahead/source/TypeaheadSource.js       |   9 +-
 .../js/application/herald/PathTypeahead.js    |  90 ++++----
 .../js/application/owners/OwnersPathEditor.js | 204 +++++++++++-------
 webroot/rsrc/js/phuix/PHUIXFormControl.js     |   9 +-
 8 files changed, 310 insertions(+), 198 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 8bccfbb406..699d3288e5 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,7 +10,7 @@ return array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
     'core.pkg.css' => '2fa91e14',
-    'core.pkg.js' => 'e4d73c62',
+    'core.pkg.js' => '32bb68e9',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '113e692c',
     'differential.pkg.js' => 'f6d809c0',
@@ -85,7 +85,7 @@ return array(
     'rsrc/css/application/maniphest/task-edit.css' => 'fda62a9b',
     'rsrc/css/application/maniphest/task-summary.css' => '11cc5344',
     'rsrc/css/application/objectselector/object-selector.css' => '85ee8ce6',
-    'rsrc/css/application/owners/owners-path-editor.css' => '2f00933b',
+    'rsrc/css/application/owners/owners-path-editor.css' => '9c136c29',
     'rsrc/css/application/paste/paste.css' => '9fcc9773',
     'rsrc/css/application/people/people-picture-menu-item.css' => 'a06f7f34',
     'rsrc/css/application/people/people-profile.css' => '4df76faf',
@@ -268,7 +268,7 @@ return array(
     'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadCompositeSource.js' => '503e17fd',
     'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadOnDemandSource.js' => '013ffff9',
     'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadPreloadedSource.js' => '54f314a0',
-    'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js' => '0fcf201c',
+    'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js' => 'ab9e0a82',
     'rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadStaticSource.js' => '6c0e62fa',
     'rsrc/favicons/apple-touch-icon-114x114.png' => '12a24178',
     'rsrc/favicons/apple-touch-icon-120x120.png' => '0d1543c7',
@@ -418,13 +418,13 @@ return array(
     'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888',
     'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909',
     'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e',
-    'rsrc/js/application/herald/PathTypeahead.js' => '78039abe',
+    'rsrc/js/application/herald/PathTypeahead.js' => '662e9cea',
     'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3',
     'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'ad54037e',
     'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876',
     'rsrc/js/application/maniphest/behavior-list-edit.js' => 'a9f88de2',
     'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '71237763',
-    'rsrc/js/application/owners/OwnersPathEditor.js' => '52b9cbc4',
+    'rsrc/js/application/owners/OwnersPathEditor.js' => 'c96502cf',
     'rsrc/js/application/owners/owners-path-editor.js' => '7a68dda3',
     'rsrc/js/application/passphrase/passphrase-credential-control.js' => '3cb0b2fc',
     'rsrc/js/application/pholio/behavior-pholio-mock-edit.js' => 'bee502c8',
@@ -534,7 +534,7 @@ return array(
     'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac',
     'rsrc/js/phuix/PHUIXDropdownMenu.js' => '04b2ae03',
     'rsrc/js/phuix/PHUIXExample.js' => '68af71ca',
-    'rsrc/js/phuix/PHUIXFormControl.js' => '16ad6224',
+    'rsrc/js/phuix/PHUIXFormControl.js' => '210a16c1',
     'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b',
   ),
   'symbols' => array(
@@ -744,7 +744,7 @@ return array(
     'javelin-typeahead-normalizer' => '185bbd53',
     'javelin-typeahead-ondemand-source' => '013ffff9',
     'javelin-typeahead-preloaded-source' => '54f314a0',
-    'javelin-typeahead-source' => '0fcf201c',
+    'javelin-typeahead-source' => 'ab9e0a82',
     'javelin-typeahead-static-source' => '6c0e62fa',
     'javelin-uri' => 'c989ade3',
     'javelin-util' => '93cc50d6',
@@ -764,10 +764,10 @@ return array(
     'maniphest-task-edit-css' => 'fda62a9b',
     'maniphest-task-summary-css' => '11cc5344',
     'multirow-row-manager' => 'b5d57730',
-    'owners-path-editor' => '52b9cbc4',
-    'owners-path-editor-css' => '2f00933b',
+    'owners-path-editor' => 'c96502cf',
+    'owners-path-editor-css' => '9c136c29',
     'paste-css' => '9fcc9773',
-    'path-typeahead' => '78039abe',
+    'path-typeahead' => '662e9cea',
     'people-picture-menu-item-css' => 'a06f7f34',
     'people-profile-css' => '4df76faf',
     'phabricator-action-list-view-css' => '0bcd9a45',
@@ -888,7 +888,7 @@ return array(
     'phuix-autocomplete' => '7fa5c915',
     'phuix-button-view' => '8a91e1ac',
     'phuix-dropdown-menu' => '04b2ae03',
-    'phuix-form-control-view' => '16ad6224',
+    'phuix-form-control-view' => '210a16c1',
     'phuix-icon-view' => 'bff6884b',
     'policy-css' => '957ea14c',
     'policy-edit-css' => '815c66f7',
@@ -998,20 +998,10 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    '0fcf201c' => array(
-      'javelin-install',
-      'javelin-util',
-      'javelin-dom',
-      'javelin-typeahead-normalizer',
-    ),
     '15d5ff71' => array(
       'aphront-typeahead-control-css',
       'phui-tag-view-css',
     ),
-    '16ad6224' => array(
-      'javelin-install',
-      'javelin-dom',
-    ),
     '17bb8539' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1061,6 +1051,10 @@ return array(
       'javelin-install',
       'javelin-dom',
     ),
+    '210a16c1' => array(
+      'javelin-install',
+      'javelin-dom',
+    ),
     '2290aeef' => array(
       'javelin-install',
       'javelin-dom',
@@ -1337,14 +1331,6 @@ return array(
       'javelin-vector',
       'javelin-typeahead-static-source',
     ),
-    '52b9cbc4' => array(
-      'multirow-row-manager',
-      'javelin-install',
-      'path-typeahead',
-      'javelin-dom',
-      'javelin-util',
-      'phabricator-prefab',
-    ),
     '54b612ba' => array(
       'javelin-color',
       'javelin-install',
@@ -1439,6 +1425,14 @@ return array(
       'javelin-workflow',
       'javelin-dom',
     ),
+    '662e9cea' => array(
+      'javelin-install',
+      'javelin-typeahead',
+      'javelin-dom',
+      'javelin-request',
+      'javelin-typeahead-ondemand-source',
+      'javelin-util',
+    ),
     '66a6def1' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1542,14 +1536,6 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
-    '78039abe' => array(
-      'javelin-install',
-      'javelin-typeahead',
-      'javelin-dom',
-      'javelin-request',
-      'javelin-typeahead-ondemand-source',
-      'javelin-util',
-    ),
     '7927a7d3' => array(
       'javelin-behavior',
       'javelin-quicksand',
@@ -1799,6 +1785,12 @@ return array(
       'javelin-util',
       'phabricator-busy',
     ),
+    'ab9e0a82' => array(
+      'javelin-install',
+      'javelin-util',
+      'javelin-dom',
+      'javelin-typeahead-normalizer',
+    ),
     'acd29eee' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1974,6 +1966,15 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
+    'c96502cf' => array(
+      'multirow-row-manager',
+      'javelin-install',
+      'path-typeahead',
+      'javelin-dom',
+      'javelin-util',
+      'phabricator-prefab',
+      'phuix-form-control-view',
+    ),
     'c989ade3' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/applications/diffusion/controller/DiffusionPathValidateController.php b/src/applications/diffusion/controller/DiffusionPathValidateController.php
index e6fbc41132..62e9e04796 100644
--- a/src/applications/diffusion/controller/DiffusionPathValidateController.php
+++ b/src/applications/diffusion/controller/DiffusionPathValidateController.php
@@ -45,19 +45,6 @@ final class DiffusionPathValidateController extends DiffusionController {
       'valid' => (bool)$valid,
     );
 
-    if (!$valid) {
-      $branch = $drequest->getBranch();
-      if ($branch) {
-        $message = pht('Not found in %s', $branch);
-      } else {
-        $message = pht('Not found at %s', 'HEAD');
-      }
-    } else {
-      $message = pht('OK');
-    }
-
-    $output['message'] = $message;
-
     return id(new AphrontAjaxResponse())->setContent($output);
   }
 }
diff --git a/src/applications/owners/controller/PhabricatorOwnersPathsController.php b/src/applications/owners/controller/PhabricatorOwnersPathsController.php
index ec7322a7c0..d4d97290f9 100644
--- a/src/applications/owners/controller/PhabricatorOwnersPathsController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersPathsController.php
@@ -27,7 +27,7 @@ final class PhabricatorOwnersPathsController
 
       $path_refs = array();
       foreach ($paths as $key => $path) {
-        if (!isset($repos[$key])) {
+        if (!isset($repos[$key]) || !strlen($repos[$key])) {
           throw new Exception(
             pht(
               'No repository PHID for path "%s"!',
@@ -70,17 +70,39 @@ final class PhabricatorOwnersPathsController
       $path_refs = mpull($paths, 'getRef');
     }
 
-    $repos = id(new PhabricatorRepositoryQuery())
-      ->setViewer($viewer)
-      ->execute();
+    $template = new AphrontTokenizerTemplateView();
 
-    $repo_map = array();
-    foreach ($repos as $key => $repo) {
-      $monogram = $repo->getMonogram();
-      $name = $repo->getName();
-      $repo_map[$repo->getPHID()] = "{$monogram} {$name}";
+    $datasource = id(new DiffusionRepositoryDatasource())
+      ->setViewer($viewer);
+
+    $tokenizer_spec = array(
+      'markup' => $template->render(),
+      'config' => array(
+        'src' => $datasource->getDatasourceURI(),
+        'browseURI' => $datasource->getBrowseURI(),
+        'placeholder' => $datasource->getPlaceholderText(),
+        'limit' => 1,
+      ),
+    );
+
+    foreach ($path_refs as $key => $path_ref) {
+      $path_refs[$key]['repositoryValue'] = $datasource->getWireTokens(
+        array(
+          $path_ref['repositoryPHID'],
+        ));
     }
-    asort($repos);
+
+    $icon_test = id(new PHUIIconView())
+      ->setIcon('fa-spinner grey')
+      ->setTooltip(pht('Validating...'));
+
+    $icon_okay = id(new PHUIIconView())
+      ->setIcon('fa-check-circle green')
+      ->setTooltip(pht('Path Exists in Repository'));
+
+    $icon_fail = id(new PHUIIconView())
+      ->setIcon('fa-question-circle-o red')
+      ->setTooltip(pht('Path Not Found On Default Branch'));
 
     $template = new AphrontTypeaheadTemplateView();
     $template = $template->render();
@@ -88,15 +110,23 @@ final class PhabricatorOwnersPathsController
     Javelin::initBehavior(
       'owners-path-editor',
       array(
-        'root'                => 'path-editor',
-        'table'               => 'paths',
-        'add_button'          => 'addpath',
-        'repositories'        => $repo_map,
-        'input_template'      => $template,
-        'pathRefs'            => $path_refs,
-
-        'completeURI'         => '/diffusion/services/path/complete/',
-        'validateURI'         => '/diffusion/services/path/validate/',
+        'root' => 'path-editor',
+        'table' => 'paths',
+        'add_button' => 'addpath',
+        'input_template' => $template,
+        'pathRefs' => $path_refs,
+        'completeURI' => '/diffusion/services/path/complete/',
+        'validateURI' => '/diffusion/services/path/validate/',
+        'repositoryTokenizerSpec' => $tokenizer_spec,
+        'icons' => array(
+          'test' => hsprintf('%s', $icon_test),
+          'okay' => hsprintf('%s', $icon_okay),
+          'fail' => hsprintf('%s', $icon_fail),
+        ),
+        'modeOptions' => array(
+          0 => pht('Include'),
+          1 => pht('Exclude'),
+        ),
       ));
 
     require_celerity_resource('owners-path-editor-css');
diff --git a/webroot/rsrc/css/application/owners/owners-path-editor.css b/webroot/rsrc/css/application/owners/owners-path-editor.css
index 026420123b..b703656369 100644
--- a/webroot/rsrc/css/application/owners/owners-path-editor.css
+++ b/webroot/rsrc/css/application/owners/owners-path-editor.css
@@ -3,7 +3,8 @@
  */
 
 .owners-path-editor-table {
-  margin: 10px;
+  margin: 10px 0;
+  width: 100%;
 }
 
 .owners-path-editor-table td {
@@ -11,27 +12,38 @@
   vertical-align: middle;
 }
 
-.owners-path-editor-table select.owners-repo {
-  width: 150px;
+.owners-path-editor-table td.owners-path-mode-control {
+  width: 180px;
 }
 
-.owners-path-editor-table input {
-  width: 400px;
+.owners-path-editor-table td.owners-path-mode-control select {
+  width: 100%;
 }
 
-.owners-path-editor-table div.error-display {
-  padding: 4px 12px 0;
+.owners-path-editor-table td.owners-path-repo-control {
+  width: 280px;
 }
 
-.owners-path-editor-table div.validating {
-  color: {$greytext};
+.owners-path-editor-table td.owners-path-path-control {
+  width: auto;
 }
 
-.owners-path-editor-table div.invalid {
-  color: #aa0000;
+.owners-path-editor-table td.owners-path-path-control input {
+  width: 100%;
 }
 
-.owners-path-editor-table div.valid {
-  color: #00aa00;
-  font-weight: bold;
+.owners-path-editor-table td.owners-path-path-control .jx-typeahead-results a {
+  padding: 4px;
+}
+
+.owners-path-editor-table td.owners-path-icon-control {
+  width: 18px;
+}
+
+.owners-path-editor-table td.remove-column {
+  width: 100px;
+}
+
+.owners-path-editor-table td.remove-column a {
+  display: block;
 }
diff --git a/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js b/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js
index 91eec515f5..0dedc08666 100644
--- a/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js
+++ b/webroot/rsrc/externals/javelin/lib/control/typeahead/source/TypeaheadSource.js
@@ -9,8 +9,7 @@
 
 JX.install('TypeaheadSource', {
   construct : function() {
-    this._raw = {};
-    this._lookup = {};
+    this.resetResults();
     this.setNormalizer(JX.TypeaheadNormalizer.normalize);
     this._excludeIDs = {};
   },
@@ -359,6 +358,12 @@ JX.install('TypeaheadSource', {
       }
       return str.split(/\s+/g);
     },
+
+    resetResults: function() {
+      this._raw = {};
+      this._lookup = {};
+    },
+
     _defaultTransformer : function(object) {
       return {
         name : object[0],
diff --git a/webroot/rsrc/js/application/herald/PathTypeahead.js b/webroot/rsrc/js/application/herald/PathTypeahead.js
index 5df0263086..0ce9823d13 100644
--- a/webroot/rsrc/js/application/herald/PathTypeahead.js
+++ b/webroot/rsrc/js/application/herald/PathTypeahead.js
@@ -11,7 +11,7 @@
 
 JX.install('PathTypeahead', {
   construct : function(config) {
-    this._repositorySelect = config.repo_select;
+    this._repositoryTokenizer = config.repositoryTokenizer;
     this._hardpoint = config.hardpoint;
     this._input = config.path_input;
     this._completeURI = config.completeURI;
@@ -19,14 +19,14 @@ JX.install('PathTypeahead', {
     this._errorDisplay = config.error_display;
     this._textInputValues = {};
 
+    this._icons = config.icons;
+
     this._initializeDatasource();
     this._initializeTypeahead(this._input);
   },
   members : {
-    /*
-     * DOM <select> elem for choosing the repository of a path.
-     */
-    _repositorySelect : null,
+    _repositoryTokenizer : null,
+
     /*
      * DOM parent div "hardpoint" to be passed to the JX.Typeahead.
      */
@@ -79,31 +79,28 @@ JX.install('PathTypeahead', {
      */
     start : function() {
       if (this._typeahead.getValue()) {
-        this._textInputValues[this._repositorySelect.value] =
-          this._typeahead.getValue();
+        var phid = this._getRepositoryPHID();
+        if (phid) {
+          this._textInputValues[phid] = this._typeahead.getValue();
+        }
       }
 
       this._typeahead.listen(
         'change',
         JX.bind(this, function(value) {
-          this._textInputValues[this._repositorySelect.value] = value;
-          this._validate();
-        }));
+          var phid = this._getRepositoryPHID();
+          if (phid) {
+            this._textInputValues[phid] = value;
+          }
 
-      this._typeahead.listen(
-        'choose',
-        JX.bind(this, function() {
-          setTimeout(JX.bind(this._typeahead, this._typeahead.refresh), 0);
+          this._validate();
         }));
 
       var repo_set_input = JX.bind(this, this._onrepochange);
 
       this._typeahead.listen('start', repo_set_input);
-      JX.DOM.listen(
-        this._repositorySelect,
-        'change',
-        null,
-        repo_set_input);
+
+      this._repositoryTokenizer.listen('change', repo_set_input);
 
       this._typeahead.start();
       this._validate();
@@ -115,13 +112,18 @@ JX.install('PathTypeahead', {
         this._textInputValues);
 
       this._datasource.setAuxiliaryData(
-        {repositoryPHID : this._repositorySelect.value}
-      );
+        {
+          repositoryPHID: this._getRepositoryPHID()
+        });
+
+      // Since we've changed the repository, reset the results.
+      this._datasource.resetResults();
     },
 
     _setPathInputBasedOnRepository : function(typeahead, lookup) {
-      if (lookup[this._repositorySelect.value]) {
-        typeahead.setValue(lookup[this._repositorySelect.value]);
+      var phid = this._getRepositoryPHID();
+      if (phid && lookup[phid]) {
+        typeahead.setValue(lookup[phid]);
       } else {
         typeahead.setValue('/');
       }
@@ -147,9 +149,24 @@ JX.install('PathTypeahead', {
       return ('' + str).replace(/[\/]+/g, '\/');
     },
 
+    _getRepositoryPHID: function() {
+      var tokens = this._repositoryTokenizer.getTokens();
+      var keys = JX.keys(tokens);
+
+      if (keys.length) {
+        return keys[0];
+      }
+
+      return null;
+    },
+
     _validate : function() {
+      var repo_phid = this._getRepositoryPHID();
+      if (!repo_phid) {
+        return;
+      }
+
       var input = this._input;
-      var repo_id = this._repositorySelect.value;
       var input_value = input.value;
       var error_display = this._errorDisplay;
 
@@ -165,33 +182,32 @@ JX.install('PathTypeahead', {
 
       var validation_request = new JX.Request(
         this._validateURI,
-        function(payload) {
+        JX.bind(this, function(payload) {
           // Don't change validation display state if the input has been
           // changed since we started validation
-          if (input.value === input_value) {
-            if (payload.valid) {
-              JX.DOM.alterClass(error_display, 'invalid', false);
-              JX.DOM.alterClass(error_display, 'valid', true);
-            } else {
-              JX.DOM.alterClass(error_display, 'invalid', true);
-              JX.DOM.alterClass(error_display, 'valid', false);
-            }
-            JX.DOM.setContent(error_display, payload.message);
+          if (input.value !== input_value) {
+            return;
           }
-        });
+
+          if (payload.valid) {
+            JX.DOM.setContent(error_display, JX.$H(this._icons.okay));
+          } else {
+            JX.DOM.setContent(error_display, JX.$H(this._icons.fail));
+          }
+        }));
 
       validation_request.listen('finally', function() {
-        JX.DOM.alterClass(error_display, 'validating', false);
         this._validationInflight = null;
       });
 
       validation_request.setData(
         {
-          repositoryPHID : repo_id,
+          repositoryPHID : repo_phid,
           path : input_value
         });
 
       this._validationInflight = validation_request;
+      JX.DOM.setContent(error_display, JX.$H(this._icons.test));
 
       validation_request.setTimeout(750);
       validation_request.send();
diff --git a/webroot/rsrc/js/application/owners/OwnersPathEditor.js b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
index 75dff4c71f..51780d8a9c 100644
--- a/webroot/rsrc/js/application/owners/OwnersPathEditor.js
+++ b/webroot/rsrc/js/application/owners/OwnersPathEditor.js
@@ -5,6 +5,7 @@
  *           javelin-dom
  *           javelin-util
  *           phabricator-prefab
+ *           phuix-form-control-view
  * @provides owners-path-editor
  * @javelin
  */
@@ -23,11 +24,13 @@ JX.install('OwnersPathEditor', {
       JX.bind(this, this._onaddpath));
 
     this._count = 0;
-    this._repositories = config.repositories;
     this._inputTemplate = config.input_template;
+    this._repositoryTokenizerSpec = config.repositoryTokenizerSpec;
 
     this._completeURI = config.completeURI;
     this._validateURI = config.validateURI;
+    this._icons = config.icons;
+    this._modeOptions = config.modeOptions;
 
     this._initializePaths(config.pathRefs);
   },
@@ -37,12 +40,6 @@ JX.install('OwnersPathEditor', {
      */
     _rowManager : null,
 
-    /*
-     * Array of objects with 'name' and 'repo_id' keys for
-     * selecting the repository of a path.
-     */
-    _repositories : null,
-
     /*
      * How many rows have been created, for form name generation.
      */
@@ -65,6 +62,8 @@ JX.install('OwnersPathEditor', {
      * default for future rows.
      */
     _lastRepositoryChoice : null,
+    _icons: null,
+    _modeOptions: null,
 
     /*
      * Initialize with 0 or more rows.
@@ -85,68 +84,34 @@ JX.install('OwnersPathEditor', {
     addPath : function(path_ref) {
       // Smart default repository. See _lastRepositoryChoice.
       if (path_ref) {
-        this._lastRepositoryChoice = path_ref.repositoryPHID;
+        this._lastRepositoryChoice = path_ref.repositoryValue;
+      } else {
+        path_ref = {
+          repositoryValue: this._lastRepositoryChoice || {}
+        };
       }
-      path_ref = path_ref || {};
 
-      var selected_repository = path_ref.repositoryPHID ||
-        this._lastRepositoryChoice;
-      var options = this._buildRepositoryOptions(selected_repository);
-      var attrs = {
-        name : 'repo[' + this._count + ']',
-        className : 'owners-repo'
-      };
-      var repo_select = JX.$N('select', attrs, options);
-
-      JX.DOM.listen(repo_select, 'change', null, JX.bind(this, function() {
-        this._lastRepositoryChoice = repo_select.value;
-      }));
-
-      var repo_cell = JX.$N('td', {}, repo_select);
-      var typeahead_cell = JX.$N(
-        'td',
-        JX.$H(this._inputTemplate));
-
-      // Text input for path.
-      var path_input = JX.DOM.find(typeahead_cell, 'input');
-      JX.copy(
-        path_input,
-        {
-          value : path_ref.display || '',
-          name : 'path[' + this._count + ']'
-        });
-
-      // The Typeahead requires a display div called hardpoint.
-      var hardpoint = JX.DOM.find(
-        typeahead_cell,
-        'div',
-        'typeahead-hardpoint');
-
-      var error_display = JX.$N(
-        'div',
-        {
-          className : 'error-display validating'
-        },
-        'Validating...');
-
-      var error_display_cell = JX.$N('td', {}, error_display);
-
-      var exclude = JX.Prefab.renderSelect(
-        {'0' : 'Include', '1' : 'Exclude'},
-        path_ref.excluded,
-        {name : 'exclude[' + this._count + ']'});
-      var exclude_cell = JX.$N('td', {}, exclude);
+      var repo = this._newRepoCell(path_ref.repositoryValue);
+      var path = this._newPathCell(path_ref.display);
+      var icon = this._newIconCell();
+      var mode_cell = this._newModeCell(path_ref.excluded);
 
       var row = this._rowManager.addRow(
-        [exclude_cell, repo_cell, typeahead_cell, error_display_cell]);
+        [
+          mode_cell,
+          repo.cell,
+          path.cell,
+          icon.cell
+        ]);
 
       new JX.PathTypeahead({
-        repo_select : repo_select,
-        path_input : path_input,
-        hardpoint : hardpoint,
-        error_display : error_display,
+        repositoryTokenizer: repo.tokenizer,
+        path_input : path.input,
+        hardpoint : path.hardpoint,
+        error_display : icon.cell,
         completeURI : this._completeURI,
-        validateURI : this._validateURI
+        validateURI : this._validateURI,
+        icons: this._icons
       }).start();
 
       this._count++;
@@ -158,20 +123,109 @@ JX.install('OwnersPathEditor', {
       this.addPath();
     },
 
-    /**
-     * Helper to build the options for the repository choice dropdown.
-     */
-    _buildRepositoryOptions : function(selected) {
-      var repos = this._repositories;
-      var result = [];
-      for (var k in repos) {
-        var attr = {
-          value : k,
-          selected : (selected == k)
-        };
-        result.push(JX.$N('option', attr, repos[k]));
+    _newModeCell: function(value) {
+      var options = this._modeOptions;
+
+      var name = 'exclude[' + this._count + ']';
+
+      var control = JX.Prefab.renderSelect(options, value, {name: name});
+
+      return JX.$N(
+        'td',
+        {
+          className: 'owners-path-mode-control'
+        },
+        control);
+    },
+
+    _newRepoCell: function(value) {
+      var repo_control = new JX.PHUIXFormControl()
+        .setControl('tokenizer', this._repositoryTokenizerSpec)
+        .setValue(value);
+
+      var repo_tokenizer = repo_control.getTokenizer();
+      var name = 'repo[' + this._count + ']';
+
+      function get_phid() {
+        var phids = repo_control.getValue();
+        if (!phids.length) {
+          return null;
+        }
+
+        return phids[0];
       }
-      return result;
+
+      var input = JX.$N(
+        'input',
+        {
+          type: 'hidden',
+          name: name,
+          value: get_phid()
+        });
+
+      repo_tokenizer.listen('change', JX.bind(this, function() {
+        this._lastRepositoryChoice = repo_tokenizer.getTokens();
+
+        input.value = get_phid();
+      }));
+
+      var cell = JX.$N(
+        'td',
+        {
+          className: 'owners-path-repo-control'
+        },
+        [
+          repo_control.getRawInputNode(),
+          input
+        ]);
+
+      return {
+        cell: cell,
+        tokenizer: repo_tokenizer
+      };
+    },
+
+    _newPathCell: function(value) {
+      var path_cell = JX.$N(
+        'td',
+        {
+          className: 'owners-path-path-control'
+        },
+        JX.$H(this._inputTemplate));
+
+      var path_input = JX.DOM.find(path_cell, 'input');
+
+      JX.copy(
+        path_input,
+        {
+          value: value || '',
+          name: 'path[' + this._count + ']'
+        });
+
+      var hardpoint = JX.DOM.find(
+        path_cell,
+        'div',
+        'typeahead-hardpoint');
+
+      return {
+        cell: path_cell,
+        input: path_input,
+        hardpoint: hardpoint
+      };
+    },
+
+    _newIconCell: function() {
+      var cell = JX.$N(
+        'td',
+        {
+          className: 'owners-path-icon-control'
+        });
+
+      return {
+        cell: cell
+      };
     }
+
   }
+
 });
diff --git a/webroot/rsrc/js/phuix/PHUIXFormControl.js b/webroot/rsrc/js/phuix/PHUIXFormControl.js
index eb5f32963d..88a1cd3f3d 100644
--- a/webroot/rsrc/js/phuix/PHUIXFormControl.js
+++ b/webroot/rsrc/js/phuix/PHUIXFormControl.js
@@ -15,6 +15,7 @@ JX.install('PHUIXFormControl', {
     _valueSetCallback: null,
     _valueGetCallback: null,
     _rawInputNode: null,
+    _tokenizer: null,
 
     setLabel: function(label) {
       JX.DOM.setContent(this._getLabelNode(), label);
@@ -70,6 +71,7 @@ JX.install('PHUIXFormControl', {
       this._valueGetCallback = input.get;
       this._valueSetCallback = input.set;
       this._rawInputNode = input.node;
+      this._tokenizer = input.tokenizer || null;
 
       return this;
     },
@@ -87,6 +89,10 @@ JX.install('PHUIXFormControl', {
       return this._rawInputNode;
     },
 
+    getTokenizer: function() {
+      return this._tokenizer;
+    },
+
     getNode: function() {
       if (!this._node) {
 
@@ -168,7 +174,8 @@ JX.install('PHUIXFormControl', {
       return {
         node: build.node,
         get: get_value,
-        set: set_value
+        set: set_value,
+        tokenizer: build.tokenizer
       };
     },
 

From 01bbd71b9683a393a6376b82d481e39884d42158 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 05:00:07 -0800
Subject: [PATCH 23/37] Separate the "{img ...}" remarkup rule into separate
 parse and markup phases

Summary:
Ref T13101. Ref T4190. This rule is currently single-phase but I'd like to check for a valid proxied image in cache already and just emit an `<img ... />` tag pointing at it if we have one.

To support batching these lookups, split the rule into a parse phase (where we extract URIs) and a markup phase (where we build tags).

Test Plan: Used `{img ...}` in Remarkup with no apparent behavioral changes. (This change should do nothing on its own.)

Maniphest Tasks: T13101, T4190

Differential Revision: https://secure.phabricator.com/D19192
---
 .../markup/PhabricatorImageRemarkupRule.php   | 67 +++++++++++++++++--
 1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
index 36089dc57e..69e9d6bcf1 100644
--- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
@@ -1,6 +1,9 @@
 <?php
 
 final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
+
+  const KEY_RULE_EXTERNAL_IMAGE = 'rule.external-image';
+
   public function getPriority() {
     return 200.0;
   }
@@ -16,6 +19,7 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
     if (!$this->isFlatText($matches[0])) {
       return $matches[0];
     }
+
     $args = array();
     $defaults = array(
       'uri' => null,
@@ -23,9 +27,10 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
       'width' => null,
       'height' => null,
     );
+
     $trimmed_match = trim($matches[2]);
     if ($this->isURI($trimmed_match)) {
-      $args['uri'] = new PhutilURI($trimmed_match);
+      $args['uri'] = $trimmed_match;
     } else {
       $parser = new PhutilSimpleOptions();
       $keys = $parser->parse($trimmed_match);
@@ -37,16 +42,64 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
         }
       }
       if ($uri_key) {
-        $args['uri'] = new PhutilURI($keys[$uri_key]);
+        $args['uri'] = $keys[$uri_key];
       }
       $args += $keys;
     }
 
     $args += $defaults;
 
-    if ($args['uri']) {
+    if (!strlen($args['uri'])) {
+      return $matches[0];
+    }
+
+    // Make sure this is something that looks roughly like a real URI. We'll
+    // validate it more carefully before proxying it, but if whatever the user
+    // has typed isn't even close, just decline to activate the rule behavior.
+    try {
+      $uri = new PhutilURI($args['uri']);
+
+      if (!strlen($uri->getProtocol())) {
+        return $matches[0];
+      }
+
+      $args['uri'] = (string)$uri;
+    } catch (Exception $ex) {
+      return $matches[0];
+    }
+
+    $engine = $this->getEngine();
+    $metadata_key = self::KEY_RULE_EXTERNAL_IMAGE;
+    $metadata = $engine->getTextMetadata($metadata_key, array());
+
+    $token = $engine->storeText('<img>');
+
+    $metadata[] = array(
+      'token' => $token,
+      'args' => $args,
+    );
+
+    $engine->setTextMetadata($metadata_key, $metadata);
+
+    return $token;
+  }
+
+  public function didMarkupText() {
+    $engine = $this->getEngine();
+    $metadata_key = self::KEY_RULE_EXTERNAL_IMAGE;
+    $images = $engine->getTextMetadata($metadata_key, array());
+    $engine->setTextMetadata($metadata_key, array());
+
+    if (!$images) {
+      return;
+    }
+
+    foreach ($images as $image) {
+      $args = $image['args'];
+
       $src_uri = id(new PhutilURI('/file/imageproxy/'))
-        ->setQueryParam('uri', (string)$args['uri']);
+        ->setQueryParam('uri', $args['uri']);
+
       $img = $this->newTag(
         'img',
         array(
@@ -55,9 +108,8 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
           'width' => $args['width'],
           'height' => $args['height'],
         ));
-      return $this->getEngine()->storeText($img);
-    } else {
-      return $matches[0];
+
+      $engine->overwriteStoredText($image['token'], $img);
     }
   }
 
@@ -66,4 +118,5 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
     // If it does, we'll try to treat it like a valid URI
     return preg_match('~^https?\:\/\/.*\z~i', $uri_string);
   }
+
 }

From 9d3a722eb1fedeb448f567a0c33e2f224bd3fbbe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 05:29:24 -0800
Subject: [PATCH 24/37] When proxying an "{image ...}" image fails, show the
 user an error message

Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.

This isn't the most polished implementation imaginable, but it's much less mysterious about errors.

Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.

Maniphest Tasks: T13101, T4190

Differential Revision: https://secure.phabricator.com/D19193
---
 resources/celerity/map.php                    | 12 ++-
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorFileImageProxyController.php   | 85 +++++++++++++------
 .../markup/PhabricatorImageRemarkupRule.php   | 13 ++-
 .../markup/view/PHUIRemarkupImageView.php     | 67 +++++++++++++++
 webroot/rsrc/css/core/remarkup.css            |  7 ++
 .../js/core/behavior-remarkup-load-image.js   | 45 ++++++++++
 7 files changed, 193 insertions(+), 38 deletions(-)
 create mode 100644 src/infrastructure/markup/view/PHUIRemarkupImageView.php
 create mode 100644 webroot/rsrc/js/core/behavior-remarkup-load-image.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 699d3288e5..48e8362194 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' => '2fa91e14',
+    'core.pkg.css' => 'c218ed53',
     'core.pkg.js' => '32bb68e9',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '113e692c',
@@ -114,7 +114,7 @@ return array(
     'rsrc/css/application/tokens/tokens.css' => '3d0f239e',
     'rsrc/css/application/uiexample/example.css' => '528b19de',
     'rsrc/css/core/core.css' => '62fa3ace',
-    'rsrc/css/core/remarkup.css' => 'cad18339',
+    'rsrc/css/core/remarkup.css' => '97dc3523',
     'rsrc/css/core/syntax.css' => 'cae95e89',
     'rsrc/css/core/z-index.css' => '9d8f7c4b',
     'rsrc/css/diviner/diviner-shared.css' => '896f1d43',
@@ -504,6 +504,7 @@ return array(
     'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207',
     'rsrc/js/core/behavior-redirect.js' => '0213259f',
     'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b',
+    'rsrc/js/core/behavior-remarkup-load-image.js' => '040fce04',
     'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e',
     'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e',
     'rsrc/js/core/behavior-reveal-content.js' => '60821bc7',
@@ -692,6 +693,7 @@ return array(
     'javelin-behavior-releeph-preview-branch' => 'b2b4fbaf',
     'javelin-behavior-releeph-request-state-change' => 'a0b57eb8',
     'javelin-behavior-releeph-request-typeahead' => 'de2e896f',
+    'javelin-behavior-remarkup-load-image' => '040fce04',
     'javelin-behavior-remarkup-preview' => '4b700e9e',
     'javelin-behavior-reorder-applications' => '76b9fc3e',
     'javelin-behavior-reorder-columns' => 'e1d25dfb',
@@ -800,7 +802,7 @@ return array(
     'phabricator-object-selector-css' => '85ee8ce6',
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => '77b0ae28',
-    'phabricator-remarkup-css' => 'cad18339',
+    'phabricator-remarkup-css' => '97dc3523',
     'phabricator-search-results-css' => '505dd8cf',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-slowvote-css' => 'a94b7230',
@@ -940,6 +942,10 @@ return array(
       'javelin-behavior',
       'javelin-uri',
     ),
+    '040fce04' => array(
+      'javelin-behavior',
+      'javelin-request',
+    ),
     '04b2ae03' => array(
       'javelin-install',
       'javelin-util',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 6cc7450820..69c0801f9a 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1883,6 +1883,7 @@ phutil_register_library_map(array(
     'PHUIPropertyGroupView' => 'view/phui/PHUIPropertyGroupView.php',
     'PHUIPropertyListExample' => 'applications/uiexample/examples/PHUIPropertyListExample.php',
     'PHUIPropertyListView' => 'view/phui/PHUIPropertyListView.php',
+    'PHUIRemarkupImageView' => 'infrastructure/markup/view/PHUIRemarkupImageView.php',
     'PHUIRemarkupPreviewPanel' => 'view/phui/PHUIRemarkupPreviewPanel.php',
     'PHUIRemarkupView' => 'infrastructure/markup/view/PHUIRemarkupView.php',
     'PHUISegmentBarSegmentView' => 'view/phui/PHUISegmentBarSegmentView.php',
@@ -7287,6 +7288,7 @@ phutil_register_library_map(array(
     'PHUIPropertyGroupView' => 'AphrontTagView',
     'PHUIPropertyListExample' => 'PhabricatorUIExample',
     'PHUIPropertyListView' => 'AphrontView',
+    'PHUIRemarkupImageView' => 'AphrontView',
     'PHUIRemarkupPreviewPanel' => 'AphrontTagView',
     'PHUIRemarkupView' => 'AphrontView',
     'PHUISegmentBarSegmentView' => 'AphrontTagView',
diff --git a/src/applications/files/controller/PhabricatorFileImageProxyController.php b/src/applications/files/controller/PhabricatorFileImageProxyController.php
index 44573aea46..b1d94c0801 100644
--- a/src/applications/files/controller/PhabricatorFileImageProxyController.php
+++ b/src/applications/files/controller/PhabricatorFileImageProxyController.php
@@ -44,6 +44,7 @@ final class PhabricatorFileImageProxyController
       ->setTTL($ttl);
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+    $save_request = false;
     // Cache missed so we'll need to validate and download the image
     try {
       // Rate limit outbound fetches to make this mechanism less useful for
@@ -75,44 +76,74 @@ final class PhabricatorFileImageProxyController
         $file->save();
       }
 
-      $external_request->setIsSuccessful(true)
-        ->setFilePHID($file->getPHID())
-        ->save();
-      unset($unguarded);
-      return $this->getExternalResponse($external_request);
+      $external_request
+        ->setIsSuccessful(1)
+        ->setFilePHID($file->getPHID());
+
+      $save_request = true;
     } catch (HTTPFutureHTTPResponseStatus $status) {
-      $external_request->setIsSuccessful(false)
+      $external_request
+        ->setIsSuccessful(0)
         ->setResponseMessage($status->getMessage())
         ->save();
-      return $this->getExternalResponse($external_request);
+
+      $save_request = true;
     } catch (Exception $ex) {
       // Not actually saving the request in this case
       $external_request->setResponseMessage($ex->getMessage());
-      return $this->getExternalResponse($external_request);
     }
+
+    if ($save_request) {
+      try {
+        $external_request->save();
+      } catch (AphrontDuplicateKeyQueryException $ex) {
+        // We may have raced against another identical request. If we did,
+        // just throw our result away and use the winner's result.
+        $external_request = $external_request->loadOneWhere(
+          'uriIndex = %s',
+          PhabricatorHash::digestForIndex($img_uri));
+        if (!$external_request) {
+          throw new Exception(
+            pht(
+              'Hit duplicate key collision when saving proxied image, but '.
+              'failed to load duplicate row (for URI "%s").',
+              $img_uri));
+        }
+      }
+    }
+
+    unset($unguarded);
+
+
+    return $this->getExternalResponse($external_request);
   }
 
   private function getExternalResponse(
     PhabricatorFileExternalRequest $request) {
-    if ($request->getIsSuccessful()) {
-      $file = id(new PhabricatorFileQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withPHIDs(array($request->getFilePHID()))
-        ->executeOne();
-      if (!$file) {
-        throw new Exception(pht(
-          'The underlying file does not exist, but the cached request was '.
-          'successful. This likely means the file record was manually deleted '.
-          'by an administrator.'));
-      }
-      return id(new AphrontRedirectResponse())
-        ->setIsExternal(true)
-        ->setURI($file->getViewURI());
-    } else {
-      throw new Exception(pht(
-        "The request to get the external file from '%s' was unsuccessful:\n %s",
-        $request->getURI(),
-        $request->getResponseMessage()));
+    if (!$request->getIsSuccessful()) {
+      throw new Exception(
+        pht(
+          'Request to "%s" failed: %s',
+          $request->getURI(),
+          $request->getResponseMessage()));
     }
+
+    $file = id(new PhabricatorFileQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs(array($request->getFilePHID()))
+      ->executeOne();
+    if (!$file) {
+      throw new Exception(
+        pht(
+          'The underlying file does not exist, but the cached request was '.
+          'successful. This likely means the file record was manually '.
+          'deleted by an administrator.'));
+    }
+
+    return id(new AphrontAjaxResponse())
+      ->setContent(
+        array(
+          'imageURI' => $file->getViewURI(),
+        ));
   }
 }
diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
index 69e9d6bcf1..c07ab9d496 100644
--- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
@@ -100,14 +100,11 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
       $src_uri = id(new PhutilURI('/file/imageproxy/'))
         ->setQueryParam('uri', $args['uri']);
 
-      $img = $this->newTag(
-        'img',
-        array(
-          'src' => $src_uri,
-          'alt' => $args['alt'],
-          'width' => $args['width'],
-          'height' => $args['height'],
-        ));
+      $img = id(new PHUIRemarkupImageView())
+        ->setURI($src_uri)
+        ->setAlt($args['alt'])
+        ->setWidth($args['width'])
+        ->setHeight($args['height']);
 
       $engine->overwriteStoredText($image['token'], $img);
     }
diff --git a/src/infrastructure/markup/view/PHUIRemarkupImageView.php b/src/infrastructure/markup/view/PHUIRemarkupImageView.php
new file mode 100644
index 0000000000..0a73efb83b
--- /dev/null
+++ b/src/infrastructure/markup/view/PHUIRemarkupImageView.php
@@ -0,0 +1,67 @@
+<?php
+
+final class PHUIRemarkupImageView
+  extends AphrontView {
+
+  private $uri;
+  private $width;
+  private $height;
+  private $alt;
+
+  public function setURI($uri) {
+    $this->uri = $uri;
+    return $this;
+  }
+
+  public function getURI() {
+    return $this->uri;
+  }
+
+  public function setWidth($width) {
+    $this->width = $width;
+    return $this;
+  }
+
+  public function getWidth() {
+    return $this->width;
+  }
+
+  public function setHeight($height) {
+    $this->height = $height;
+    return $this;
+  }
+
+  public function getHeight() {
+    return $this->height;
+  }
+
+  public function setAlt($alt) {
+    $this->alt = $alt;
+    return $this;
+  }
+
+  public function getAlt() {
+    return $this->alt;
+  }
+
+  public function render() {
+    $id = celerity_generate_unique_node_id();
+
+    Javelin::initBehavior(
+      'remarkup-load-image',
+      array(
+        'uri' => (string)$this->uri,
+        'imageID' => $id,
+      ));
+
+    return phutil_tag(
+      'img',
+      array(
+        'id' => $id,
+        'width' => $this->getWidth(),
+        'height' => $this->getHeight(),
+        'alt' => $this->getAlt(),
+      ));
+  }
+
+}
diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css
index 8750598780..a51c1ce1d9 100644
--- a/webroot/rsrc/css/core/remarkup.css
+++ b/webroot/rsrc/css/core/remarkup.css
@@ -472,6 +472,13 @@ video.phabricator-media {
   margin: .5em 1em 0;
 }
 
+.phabricator-remarkup-image-error {
+  border: 1px solid {$redborder};
+  background: {$sh-redbackground};
+  padding: 8px 12px;
+  color: {$darkgreytext};
+}
+
 .phabricator-remarkup-embed-image {
   display: inline-block;
   border: 3px solid white;
diff --git a/webroot/rsrc/js/core/behavior-remarkup-load-image.js b/webroot/rsrc/js/core/behavior-remarkup-load-image.js
new file mode 100644
index 0000000000..a13f3b0d78
--- /dev/null
+++ b/webroot/rsrc/js/core/behavior-remarkup-load-image.js
@@ -0,0 +1,45 @@
+/**
+ * @provides javelin-behavior-remarkup-load-image
+ * @requires javelin-behavior
+ *           javelin-request
+ */
+
+JX.behavior('remarkup-load-image', function(config) {
+
+  function get_node() {
+    try {
+      return JX.$(config.imageID);
+    } catch (ex) {
+      return null;
+    }
+  }
+
+  function onload(r) {
+    var node = get_node();
+    if (!node) {
+      return;
+    }
+
+    node.src = r.imageURI;
+  }
+
+  function onerror(r) {
+    var node = get_node();
+    if (!node) {
+      return;
+    }
+
+    var error = JX.$N(
+      'div',
+      {
+        className: 'phabricator-remarkup-image-error'
+      },
+      r.info);
+
+    JX.DOM.replace(node, error);
+  }
+
+  var request = new JX.Request(config.uri, onload);
+  request.listen('error', onerror);
+  request.send();
+});

From b30535a36f5f9ad0180c78c66e4d6272de8e3ef4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 06:44:18 -0800
Subject: [PATCH 25/37] When rendering "{image ...}" images, check the cache
 and just render a direct "<img />" tag if possible

Summary: Depends on D19193. Ref T13101. Fixes T4190. Before we render a fancy AJAX placeholder, check if we already have a valid cache for the image. If we do, render a direct `<img />` tag. This is a little cleaner and, e.g., avoids flicker in Safari, at least.

Test Plan: Rendered `{image ...}` rules in remarkup with new and existing URIs.

Maniphest Tasks: T13101, T4190

Differential Revision: https://secure.phabricator.com/D19194
---
 .../markup/PhabricatorImageRemarkupRule.php   | 66 +++++++++++++++++--
 1 file changed, 59 insertions(+), 7 deletions(-)

diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
index c07ab9d496..5d1979ed3c 100644
--- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
@@ -94,17 +94,69 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
       return;
     }
 
+    // Look for images we've already successfully fetched that aren't about
+    // to get eaten by the GC. For any we find, we can just emit a normal
+    // "<img />" tag pointing directly to the file.
+
+    // For files which we don't hit in the cache, we emit a placeholder
+    // instead and use AJAX to actually perform the fetch.
+
+    $digests = array();
+    foreach ($images as $image) {
+      $uri = $image['args']['uri'];
+      $digests[] = PhabricatorHash::digestForIndex($uri);
+    }
+
+    $caches = id(new PhabricatorFileExternalRequest())->loadAllWhere(
+      'uriIndex IN (%Ls) AND isSuccessful = 1 AND ttl > %d',
+      $digests,
+      PhabricatorTime::getNow() + phutil_units('1 hour in seconds'));
+
+    $file_phids = array();
+    foreach ($caches as $cache) {
+      $file_phids[$cache->getFilePHID()] = $cache->getURI();
+    }
+
+    $file_map = array();
+    if ($file_phids) {
+      $files = id(new PhabricatorFileQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs(array_keys($file_phids))
+        ->execute();
+      foreach ($files as $file) {
+        $phid = $file->getPHID();
+
+        $file_remote_uri = $file_phids[$phid];
+        $file_view_uri = $file->getViewURI();
+
+        $file_map[$file_remote_uri] = $file_view_uri;
+      }
+    }
+
     foreach ($images as $image) {
       $args = $image['args'];
+      $uri = $args['uri'];
 
-      $src_uri = id(new PhutilURI('/file/imageproxy/'))
-        ->setQueryParam('uri', $args['uri']);
+      $direct_uri = idx($file_map, $uri);
+      if ($direct_uri) {
+        $img = phutil_tag(
+          'img',
+          array(
+            'src' => $direct_uri,
+            'alt' => $args['alt'],
+            'width' => $args['width'],
+            'height' => $args['height'],
+          ));
+      } else {
+        $src_uri = id(new PhutilURI('/file/imageproxy/'))
+          ->setQueryParam('uri', $uri);
 
-      $img = id(new PHUIRemarkupImageView())
-        ->setURI($src_uri)
-        ->setAlt($args['alt'])
-        ->setWidth($args['width'])
-        ->setHeight($args['height']);
+        $img = id(new PHUIRemarkupImageView())
+          ->setURI($src_uri)
+          ->setAlt($args['alt'])
+          ->setWidth($args['width'])
+          ->setHeight($args['height']);
+      }
 
       $engine->overwriteStoredText($image['token'], $img);
     }

From 6095d889987d4d53dc07f9ec6972b9a464f3c756 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 06:59:06 -0800
Subject: [PATCH 26/37] Don't require prototypes for "{image ...}"

Summary: Depends on D19194. Fixes T4190. This should be in good-enough shape now to release and support more generally.

Test Plan: Used `{image ...}` in remarkup.

Maniphest Tasks: T4190

Differential Revision: https://secure.phabricator.com/D19195
---
 .../PhabricatorFileImageProxyController.php   | 34 +++++++++----------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/src/applications/files/controller/PhabricatorFileImageProxyController.php b/src/applications/files/controller/PhabricatorFileImageProxyController.php
index b1d94c0801..05a49c7f3e 100644
--- a/src/applications/files/controller/PhabricatorFileImageProxyController.php
+++ b/src/applications/files/controller/PhabricatorFileImageProxyController.php
@@ -8,15 +8,6 @@ final class PhabricatorFileImageProxyController
   }
 
   public function handleRequest(AphrontRequest $request) {
-
-    $show_prototypes = PhabricatorEnv::getEnvConfig(
-      'phabricator.show-prototypes');
-    if (!$show_prototypes) {
-      throw new Exception(
-        pht('Show prototypes is disabled.
-          Set `phabricator.show-prototypes` to `true` to use the image proxy'));
-    }
-
     $viewer = $request->getViewer();
     $img_uri = $request->getStr('uri');
 
@@ -24,9 +15,16 @@ final class PhabricatorFileImageProxyController
     PhabricatorEnv::requireValidRemoteURIForLink($img_uri);
     $uri = new PhutilURI($img_uri);
     $proto = $uri->getProtocol();
-    if (!in_array($proto, array('http', 'https'))) {
+
+    $allowed_protocols = array(
+      'http',
+      'https',
+    );
+    if (!in_array($proto, $allowed_protocols)) {
       throw new Exception(
-        pht('The provided image URI must be either http or https'));
+        pht(
+          'The provided image URI must use one of these protocols: %s.',
+          implode(', ', $allowed_protocols)));
     }
 
     // Check if we already have the specified image URI downloaded
@@ -43,9 +41,9 @@ final class PhabricatorFileImageProxyController
       ->setURI($img_uri)
       ->setTTL($ttl);
 
+    // Cache missed, so we'll need to validate and download the image.
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
     $save_request = false;
-    // Cache missed so we'll need to validate and download the image
     try {
       // Rate limit outbound fetches to make this mechanism less useful for
       // scanning networks and ports.
@@ -60,6 +58,7 @@ final class PhabricatorFileImageProxyController
           'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
           'canCDN' => true,
         ));
+
       if (!$file->isViewableImage()) {
         $mime_type = $file->getMimeType();
         $engine = new PhabricatorDestructionEngine();
@@ -67,15 +66,15 @@ final class PhabricatorFileImageProxyController
         $file = null;
         throw new Exception(
           pht(
-            'The URI "%s" does not correspond to a valid image file, got '.
-            'a file with MIME type "%s". You must specify the URI of a '.
+            'The URI "%s" does not correspond to a valid image file (got '.
+            'a file with MIME type "%s"). You must specify the URI of a '.
             'valid image file.',
             $uri,
             $mime_type));
-      } else {
-        $file->save();
       }
 
+      $file->save();
+
       $external_request
         ->setIsSuccessful(1)
         ->setFilePHID($file->getPHID());
@@ -84,8 +83,7 @@ final class PhabricatorFileImageProxyController
     } catch (HTTPFutureHTTPResponseStatus $status) {
       $external_request
         ->setIsSuccessful(0)
-        ->setResponseMessage($status->getMessage())
-        ->save();
+        ->setResponseMessage($status->getMessage());
 
       $save_request = true;
     } catch (Exception $ex) {

From 98cac2cc2994d4584678c3be79f6f32b2965d2fb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 07:36:32 -0800
Subject: [PATCH 27/37] Always serve "{meme ...}" from the CDN domain, never
 from the primary domain

Summary:
Ref T13101. This is a minimal change to make "{meme ...}" work with the new Content-Security-Policy by using an Ajax request to generate the image and then swapping the source on the client.

This could be much cleaner (see T5258, etc).

Test Plan: Used `{meme, src=cat6, above=i am, below=cat}`, chuckled completely unironically.

Maniphest Tasks: T13101

Differential Revision: https://secure.phabricator.com/D19196
---
 .../PhabricatorMacroMemeController.php        | 118 ++++++++++++------
 .../markup/PhabricatorMemeRemarkupRule.php    |  11 +-
 .../markup/view/PHUIRemarkupImageView.php     |  12 ++
 3 files changed, 99 insertions(+), 42 deletions(-)

diff --git a/src/applications/macro/controller/PhabricatorMacroMemeController.php b/src/applications/macro/controller/PhabricatorMacroMemeController.php
index 03b13f6205..794e24e36d 100644
--- a/src/applications/macro/controller/PhabricatorMacroMemeController.php
+++ b/src/applications/macro/controller/PhabricatorMacroMemeController.php
@@ -13,18 +13,25 @@ final class PhabricatorMacroMemeController
     $lower_text = $request->getStr('lowertext');
     $viewer = $request->getViewer();
 
-    $uri = self::generateMacro($viewer, $macro_name,
-      $upper_text, $lower_text);
-    if ($uri === false) {
-      return new Aphront404Response();
-    }
-    return id(new AphrontRedirectResponse())
-      ->setIsExternal(true)
-      ->setURI($uri);
+    $uri = self::generateMacro(
+      $viewer,
+      $macro_name,
+      $upper_text,
+      $lower_text);
+
+    $content = array(
+      'imageURI' => $uri,
+    );
+
+    return id(new AphrontAjaxResponse())->setContent($content);
   }
 
-  public static function generateMacro($viewer, $macro_name, $upper_text,
-      $lower_text) {
+  public static function generateMacro(
+    PhabricatorUser $viewer,
+    $macro_name,
+    $upper_text,
+    $lower_text) {
+
     $macro = id(new PhabricatorMacroQuery())
       ->setViewer($viewer)
       ->withNames(array($macro_name))
@@ -35,34 +42,75 @@ final class PhabricatorMacroMemeController
     }
     $file = $macro->getFile();
 
-    $upper_text = strtoupper($upper_text);
-    $lower_text = strtoupper($lower_text);
-    $mixed_text = md5($upper_text).':'.md5($lower_text);
-    $hash = 'meme'.hash('sha256', $mixed_text);
-    $xform = id(new PhabricatorTransformedFile())
-      ->loadOneWhere('originalphid=%s and transform=%s',
-        $file->getPHID(), $hash);
+    $upper_text = phutil_utf8_strtoupper($upper_text);
+    $lower_text = phutil_utf8_strtoupper($lower_text);
 
-    if ($xform) {
-      $memefile = id(new PhabricatorFileQuery())
-        ->setViewer($viewer)
-        ->withPHIDs(array($xform->getTransformedPHID()))
-        ->executeOne();
-      if ($memefile) {
-        return $memefile->getBestURI();
-      }
+    $hash = PhabricatorHash::digestForIndex(
+      phutil_json_encode(
+        array(
+          'kind' => 'meme',
+          'upper' => $upper_text,
+          'lower' => $lower_text,
+        )));
+
+    $xfile = self::loadTransformedFile($viewer, $file->getPHID(), $hash);
+    if ($xfile) {
+      return $xfile->getViewURI();
     }
 
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-    $transformers = (new PhabricatorImageTransformer());
-    $newfile = $transformers
-      ->executeMemeTransform($file, $upper_text, $lower_text);
-    $xfile = new PhabricatorTransformedFile();
-    $xfile->setOriginalPHID($file->getPHID());
-    $xfile->setTransformedPHID($newfile->getPHID());
-    $xfile->setTransform($hash);
-    $xfile->save();
+    $transformer = new PhabricatorImageTransformer();
 
-    return $newfile->getBestURI();
+    $new_file = $transformer->executeMemeTransform(
+      $file,
+      $upper_text,
+      $lower_text);
+
+    $xfile = id(new PhabricatorTransformedFile())
+      ->setOriginalPHID($file->getPHID())
+      ->setTransformedPHID($new_file->getPHID())
+      ->setTransform($hash);
+
+    try {
+      $caught = null;
+
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      try {
+        $xfile->save();
+      } catch (Exception $ex) {
+        $caught = $ex;
+      }
+      unset($unguarded);
+
+      if ($caught) {
+        throw $caught;
+      }
+
+      return $new_file->getViewURI();
+    } catch (AphrontDuplicateKeyQueryException $ex) {
+      $xfile = self::loadTransformedFile($viewer, $file->getPHID(), $hash);
+      if (!$xfile) {
+        throw $ex;
+      }
+      return $xfile->getViewURI();
+    }
+  }
+
+  private static function loadTransformedFile(
+    PhabricatorUser $viewer,
+    $file_phid,
+    $hash) {
+
+    $xform = id(new PhabricatorTransformedFile())->loadOneWhere(
+      'originalPHID = %s AND transform = %s',
+      $file_phid,
+      $hash);
+    if (!$xform) {
+      return null;
+    }
+
+    return id(new PhabricatorFileQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($xform->getTransformedPHID()))
+      ->executeOne();
   }
 }
diff --git a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
index 004e415450..278bd403cb 100644
--- a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
+++ b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
@@ -50,13 +50,10 @@ final class PhabricatorMemeRemarkupRule extends PhutilRemarkupRule {
         $options['above'],
         $options['below']);
 
-      $img = $this->newTag(
-        'img',
-        array(
-          'src' => $uri,
-          'alt' => $alt_text,
-          'class' => 'phabricator-remarkup-macro',
-        ));
+      $img = id(new PHUIRemarkupImageView())
+        ->setURI($uri)
+        ->addClass('phabricator-remarkup-macro')
+        ->setAlt($alt_text);
     }
 
     return $this->getEngine()->storeText($img);
diff --git a/src/infrastructure/markup/view/PHUIRemarkupImageView.php b/src/infrastructure/markup/view/PHUIRemarkupImageView.php
index 0a73efb83b..0402f000a4 100644
--- a/src/infrastructure/markup/view/PHUIRemarkupImageView.php
+++ b/src/infrastructure/markup/view/PHUIRemarkupImageView.php
@@ -7,6 +7,7 @@ final class PHUIRemarkupImageView
   private $width;
   private $height;
   private $alt;
+  private $classes = array();
 
   public function setURI($uri) {
     $this->uri = $uri;
@@ -44,6 +45,11 @@ final class PHUIRemarkupImageView
     return $this->alt;
   }
 
+  public function addClass($class) {
+    $this->classes[] = $class;
+    return $this;
+  }
+
   public function render() {
     $id = celerity_generate_unique_node_id();
 
@@ -54,6 +60,11 @@ final class PHUIRemarkupImageView
         'imageID' => $id,
       ));
 
+    $classes = null;
+    if ($this->classes) {
+      $classes = implode(' ', $this->classes);
+    }
+
     return phutil_tag(
       'img',
       array(
@@ -61,6 +72,7 @@ final class PHUIRemarkupImageView
         'width' => $this->getWidth(),
         'height' => $this->getHeight(),
         'alt' => $this->getAlt(),
+        'class' => $classes,
       ));
   }
 

From e83cfa295bff0e198fa2737aa41439fe7b694780 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 08:22:16 -0800
Subject: [PATCH 28/37] Fix image prev/next cycling in lightboxes

Summary:
See <https://discourse.phabricator-community.org/t/lightbox-not-working/1201/3>. The lightbox code is fragile and currently relies on simulating a click on the actual "<a />" tag surrounding other images in the document.

This breaks the prev/next links which ignore the event because it there's no "<img />".

Instead, don't simulate clicks and just call the code we want directly.

Test Plan: Added several images to a page, used lightbox prev/next buttons to cycle between them.

Differential Revision: https://secure.phabricator.com/D19197
---
 resources/celerity/map.php                    | 24 +++++++++----------
 .../js/core/behavior-lightbox-attachments.js  |  8 +++++--
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 48e8362194..38343475d0 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -10,7 +10,7 @@ return array(
     'conpherence.pkg.css' => 'e68cf1fa',
     'conpherence.pkg.js' => '15191c65',
     'core.pkg.css' => 'c218ed53',
-    'core.pkg.js' => '32bb68e9',
+    'core.pkg.js' => '8b7400e7',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '113e692c',
     'differential.pkg.js' => 'f6d809c0',
@@ -494,7 +494,7 @@ return array(
     'rsrc/js/core/behavior-hovercard.js' => 'bcaccd64',
     'rsrc/js/core/behavior-keyboard-pager.js' => 'a8da01f0',
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0',
-    'rsrc/js/core/behavior-lightbox-attachments.js' => '562bcce0',
+    'rsrc/js/core/behavior-lightbox-attachments.js' => '6b31879a',
     'rsrc/js/core/behavior-line-linker.js' => 'a9b946f8',
     'rsrc/js/core/behavior-more.js' => 'a80d0378',
     'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0',
@@ -644,7 +644,7 @@ return array(
     'javelin-behavior-history-install' => '7ee2b591',
     'javelin-behavior-icon-composer' => '8499b6ab',
     'javelin-behavior-launch-icon-composer' => '48086888',
-    'javelin-behavior-lightbox-attachments' => '562bcce0',
+    'javelin-behavior-lightbox-attachments' => '6b31879a',
     'javelin-behavior-line-chart' => 'e4232876',
     'javelin-behavior-load-blame' => '42126667',
     'javelin-behavior-maniphest-batch-selector' => 'ad54037e',
@@ -1362,15 +1362,6 @@ return array(
       'javelin-vector',
       'javelin-dom',
     ),
-    '562bcce0' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-mask',
-      'javelin-util',
-      'phuix-icon-view',
-      'phabricator-busy',
-    ),
     '58dea2fa' => array(
       'javelin-install',
       'javelin-util',
@@ -1463,6 +1454,15 @@ return array(
     '69adf288' => array(
       'javelin-install',
     ),
+    '6b31879a' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-mask',
+      'javelin-util',
+      'phuix-icon-view',
+      'phabricator-busy',
+    ),
     '6b8ef10b' => array(
       'javelin-install',
     ),
diff --git a/webroot/rsrc/js/core/behavior-lightbox-attachments.js b/webroot/rsrc/js/core/behavior-lightbox-attachments.js
index cacec257b3..af134f9b06 100644
--- a/webroot/rsrc/js/core/behavior-lightbox-attachments.js
+++ b/webroot/rsrc/js/core/behavior-lightbox-attachments.js
@@ -56,6 +56,10 @@ JX.behavior('lightbox-attachments', function() {
 
     e.kill();
 
+    activateLightbox(e.getNode('lightboxable'));
+  }
+
+  function activateLightbox(target) {
     var mainFrame = JX.$('main-page-frame');
     var links = JX.DOM.scry(mainFrame, '*', 'lightboxable');
     var phids = {};
@@ -68,7 +72,6 @@ JX.behavior('lightbox-attachments', function() {
     // Now that we have the big picture phid situation sorted out, figure
     // out how the actual node the user clicks fits into that big picture
     // and build some pretty UI to show the attachment.
-    var target      = e.getNode('lightboxable');
     var target_data = JX.Stratcom.getData(target);
     var total       = JX.keys(phids).length;
     var current     = 1;
@@ -324,7 +327,8 @@ JX.behavior('lightbox-attachments', function() {
     }
     e.prevent();
     closeLightBox(e);
-    el.click();
+
+    activateLightbox(el);
   }
 
   // Only look for lightboxable inside the main page, not other lightboxes.

From fc1ee20efe44dba7f02a0cf1a02765fb86f40a5f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 10:52:35 -0800
Subject: [PATCH 29/37] Support repository query by short name in Diffusion

Summary: See PHI432. Ref T13099. Short names never made it to the UI/API but seem stable now, so support them.

Test Plan: {F5465173}

Maniphest Tasks: T13099

Differential Revision: https://secure.phabricator.com/D19202
---
 .../repository/query/PhabricatorRepositorySearchEngine.php | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php
index 5cd88183e1..9b5b721edb 100644
--- a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php
+++ b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php
@@ -24,6 +24,9 @@ final class PhabricatorRepositorySearchEngine
       id(new PhabricatorSearchStringListField())
         ->setLabel(pht('Callsigns'))
         ->setKey('callsigns'),
+      id(new PhabricatorSearchStringListField())
+        ->setLabel(pht('Short Names'))
+        ->setKey('shortNames'),
       id(new PhabricatorSearchSelectField())
         ->setLabel(pht('Status'))
         ->setKey('status')
@@ -51,6 +54,10 @@ final class PhabricatorRepositorySearchEngine
       $query->withCallsigns($map['callsigns']);
     }
 
+    if ($map['shortNames']) {
+      $query->withSlugs($map['shortNames']);
+    }
+
     if ($map['status']) {
       $status = idx($this->getStatusValues(), $map['status']);
       if ($status) {

From a099a06265326057742214f27921f8e1e4421d7e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 08:30:39 -0800
Subject: [PATCH 30/37] Remove some old image transform code with no callsites

Summary: Ref T13101. Ref T5258. This old image transform code no longer has callsites.

Test Plan: Grepped for removed methods, no hits.

Maniphest Tasks: T13101, T5258

Differential Revision: https://secure.phabricator.com/D19198
---
 .../files/PhabricatorImageTransformer.php     | 75 -------------------
 1 file changed, 75 deletions(-)

diff --git a/src/applications/files/PhabricatorImageTransformer.php b/src/applications/files/PhabricatorImageTransformer.php
index f29ce97835..71f07a231e 100644
--- a/src/applications/files/PhabricatorImageTransformer.php
+++ b/src/applications/files/PhabricatorImageTransformer.php
@@ -20,81 +20,6 @@ final class PhabricatorImageTransformer extends Phobject {
       ));
   }
 
-  public function executeConpherenceTransform(
-    PhabricatorFile $file,
-    $top,
-    $left,
-    $width,
-    $height) {
-
-    $image = $this->crasslyCropTo(
-      $file,
-      $top,
-      $left,
-      $width,
-      $height);
-
-    return PhabricatorFile::newFromFileData(
-      $image,
-      array(
-        'name' => 'conpherence-'.$file->getName(),
-        'profile' => true,
-        'canCDN' => true,
-      ));
-  }
-
-  private function crasslyCropTo(PhabricatorFile $file, $top, $left, $w, $h) {
-    $data = $file->loadFileData();
-    $src = imagecreatefromstring($data);
-    $dst = $this->getBlankDestinationFile($w, $h);
-
-    $scale = self::getScaleForCrop($file, $w, $h);
-    $orig_x = $left / $scale;
-    $orig_y = $top / $scale;
-    $orig_w = $w / $scale;
-    $orig_h = $h / $scale;
-
-    imagecopyresampled(
-      $dst,
-      $src,
-      0, 0,
-      $orig_x, $orig_y,
-      $w, $h,
-      $orig_w, $orig_h);
-
-    return self::saveImageDataInAnyFormat($dst, $file->getMimeType());
-  }
-
-  private function getBlankDestinationFile($dx, $dy) {
-    $dst = imagecreatetruecolor($dx, $dy);
-    imagesavealpha($dst, true);
-    imagefill($dst, 0, 0, imagecolorallocatealpha($dst, 255, 255, 255, 127));
-
-    return $dst;
-  }
-
-  public static function getScaleForCrop(
-    PhabricatorFile $file,
-    $des_width,
-    $des_height) {
-
-    $metadata = $file->getMetadata();
-    $width = $metadata[PhabricatorFile::METADATA_IMAGE_WIDTH];
-    $height = $metadata[PhabricatorFile::METADATA_IMAGE_HEIGHT];
-
-    if ($height < $des_height) {
-      $scale = $height / $des_height;
-    } else if ($width < $des_width) {
-      $scale = $width / $des_width;
-    } else {
-      $scale_x = $des_width / $width;
-      $scale_y = $des_height / $height;
-      $scale = max($scale_x, $scale_y);
-    }
-
-    return $scale;
-  }
-
   private function applyMemeToFile(
     PhabricatorFile $file,
     $upper_text,

From c7408f27977dd0f2d0b11f9c4fd7cd71c6711bc2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 09:29:33 -0800
Subject: [PATCH 31/37] PhabricatorMemeEngine HA HA HA HA

Summary:
Depends on D19198. Ref T13101. Ref T5258. Pull compositing logic out of the `Controller`.

This is moving toward fixing memes in email.

Test Plan: Used new and old memes. Used API memes.

Maniphest Tasks: T13101, T5258

Differential Revision: https://secure.phabricator.com/D19200
---
 src/__phutil_library_map__.php                |   2 +
 .../MacroCreateMemeConduitAPIMethod.php       |  21 +--
 .../PhabricatorMacroMemeController.php        | 100 +----------
 .../macro/engine/PhabricatorMemeEngine.php    | 163 ++++++++++++++++++
 .../markup/PhabricatorMemeRemarkupRule.php    |  30 +++-
 5 files changed, 201 insertions(+), 115 deletions(-)
 create mode 100644 src/applications/macro/engine/PhabricatorMemeEngine.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 69c0801f9a..8677f47b6e 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3295,6 +3295,7 @@ phutil_register_library_map(array(
     'PhabricatorMarkupInterface' => 'infrastructure/markup/PhabricatorMarkupInterface.php',
     'PhabricatorMarkupOneOff' => 'infrastructure/markup/PhabricatorMarkupOneOff.php',
     'PhabricatorMarkupPreviewController' => 'infrastructure/markup/PhabricatorMarkupPreviewController.php',
+    'PhabricatorMemeEngine' => 'applications/macro/engine/PhabricatorMemeEngine.php',
     'PhabricatorMemeRemarkupRule' => 'applications/macro/markup/PhabricatorMemeRemarkupRule.php',
     'PhabricatorMentionRemarkupRule' => 'applications/people/markup/PhabricatorMentionRemarkupRule.php',
     'PhabricatorMentionableInterface' => 'applications/transactions/interface/PhabricatorMentionableInterface.php',
@@ -8893,6 +8894,7 @@ phutil_register_library_map(array(
       'PhabricatorMarkupInterface',
     ),
     'PhabricatorMarkupPreviewController' => 'PhabricatorController',
+    'PhabricatorMemeEngine' => 'Phobject',
     'PhabricatorMemeRemarkupRule' => 'PhutilRemarkupRule',
     'PhabricatorMentionRemarkupRule' => 'PhutilRemarkupRule',
     'PhabricatorMercurialGraphStream' => 'PhabricatorRepositoryGraphStream',
diff --git a/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php b/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php
index e63d831a70..3c939f97d5 100644
--- a/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php
+++ b/src/applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php
@@ -35,22 +35,15 @@ final class MacroCreateMemeConduitAPIMethod extends MacroConduitAPIMethod {
   protected function execute(ConduitAPIRequest $request) {
     $user = $request->getUser();
 
-    $macro_name = $request->getValue('macroName');
-    $upper_text = $request->getValue('upperText');
-    $lower_text = $request->getValue('lowerText');
-
-    $uri = PhabricatorMacroMemeController::generateMacro(
-      $user,
-      $macro_name,
-      $upper_text,
-      $lower_text);
-
-    if (!$uri) {
-      throw new ConduitException('ERR-NOT-FOUND');
-    }
+    $file = id(new PhabricatorMemeEngine())
+      ->setViewer($user)
+      ->setTemplate($request->getValue('macroName'))
+      ->setAboveText($request->getValue('upperText'))
+      ->setBelowText($request->getValue('lowerText'))
+      ->newAsset();
 
     return array(
-      'uri' => $uri,
+      'uri' => $file->getViewURI(),
     );
   }
 
diff --git a/src/applications/macro/controller/PhabricatorMacroMemeController.php b/src/applications/macro/controller/PhabricatorMacroMemeController.php
index 794e24e36d..eb1dcbeb2f 100644
--- a/src/applications/macro/controller/PhabricatorMacroMemeController.php
+++ b/src/applications/macro/controller/PhabricatorMacroMemeController.php
@@ -13,104 +13,18 @@ final class PhabricatorMacroMemeController
     $lower_text = $request->getStr('lowertext');
     $viewer = $request->getViewer();
 
-    $uri = self::generateMacro(
-      $viewer,
-      $macro_name,
-      $upper_text,
-      $lower_text);
+    $file = id(new PhabricatorMemeEngine())
+      ->setViewer($viewer)
+      ->setTemplate($macro_name)
+      ->setAboveText($request->getStr('above'))
+      ->setBelowText($request->getStr('below'))
+      ->newAsset();
 
     $content = array(
-      'imageURI' => $uri,
+      'imageURI' => $file->getViewURI(),
     );
 
     return id(new AphrontAjaxResponse())->setContent($content);
   }
 
-  public static function generateMacro(
-    PhabricatorUser $viewer,
-    $macro_name,
-    $upper_text,
-    $lower_text) {
-
-    $macro = id(new PhabricatorMacroQuery())
-      ->setViewer($viewer)
-      ->withNames(array($macro_name))
-      ->needFiles(true)
-      ->executeOne();
-    if (!$macro) {
-      return false;
-    }
-    $file = $macro->getFile();
-
-    $upper_text = phutil_utf8_strtoupper($upper_text);
-    $lower_text = phutil_utf8_strtoupper($lower_text);
-
-    $hash = PhabricatorHash::digestForIndex(
-      phutil_json_encode(
-        array(
-          'kind' => 'meme',
-          'upper' => $upper_text,
-          'lower' => $lower_text,
-        )));
-
-    $xfile = self::loadTransformedFile($viewer, $file->getPHID(), $hash);
-    if ($xfile) {
-      return $xfile->getViewURI();
-    }
-
-    $transformer = new PhabricatorImageTransformer();
-
-    $new_file = $transformer->executeMemeTransform(
-      $file,
-      $upper_text,
-      $lower_text);
-
-    $xfile = id(new PhabricatorTransformedFile())
-      ->setOriginalPHID($file->getPHID())
-      ->setTransformedPHID($new_file->getPHID())
-      ->setTransform($hash);
-
-    try {
-      $caught = null;
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-      try {
-        $xfile->save();
-      } catch (Exception $ex) {
-        $caught = $ex;
-      }
-      unset($unguarded);
-
-      if ($caught) {
-        throw $caught;
-      }
-
-      return $new_file->getViewURI();
-    } catch (AphrontDuplicateKeyQueryException $ex) {
-      $xfile = self::loadTransformedFile($viewer, $file->getPHID(), $hash);
-      if (!$xfile) {
-        throw $ex;
-      }
-      return $xfile->getViewURI();
-    }
-  }
-
-  private static function loadTransformedFile(
-    PhabricatorUser $viewer,
-    $file_phid,
-    $hash) {
-
-    $xform = id(new PhabricatorTransformedFile())->loadOneWhere(
-      'originalPHID = %s AND transform = %s',
-      $file_phid,
-      $hash);
-    if (!$xform) {
-      return null;
-    }
-
-    return id(new PhabricatorFileQuery())
-      ->setViewer($viewer)
-      ->withPHIDs(array($xform->getTransformedPHID()))
-      ->executeOne();
-  }
 }
diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
new file mode 100644
index 0000000000..21a9b44e99
--- /dev/null
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -0,0 +1,163 @@
+<?php
+
+final class PhabricatorMemeEngine extends Phobject {
+
+  private $viewer;
+  private $template;
+  private $aboveText;
+  private $belowText;
+
+  private $templateFile;
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+  public function setTemplate($template) {
+    $this->template = $template;
+    return $this;
+  }
+
+  public function getTemplate() {
+    return $this->template;
+  }
+
+  public function setAboveText($above_text) {
+    $this->aboveText = $above_text;
+    return $this;
+  }
+
+  public function getAboveText() {
+    return $this->aboveText;
+  }
+
+  public function setBelowText($below_text) {
+    $this->belowText = $below_text;
+    return $this;
+  }
+
+  public function getBelowText() {
+    return $this->belowText;
+  }
+
+  public function getGenerateURI() {
+    return id(new PhutilURI('/macro/meme/'))
+      ->alter('macro', $this->getTemplate())
+      ->alter('above', $this->getAboveText())
+      ->alter('below', $this->getBelowText());
+  }
+
+  public function newAsset() {
+    $cache = $this->loadCachedFile();
+    if ($cache) {
+      return $cache;
+    }
+
+    $template = $this->loadTemplateFile();
+    if (!$template) {
+      throw new Exception(
+        pht(
+          'Template "%s" is not a valid template.',
+          $template));
+    }
+
+    $hash = $this->newTransformHash();
+
+    $transformer = new PhabricatorImageTransformer();
+    $asset = $transformer->executeMemeTransform(
+      $template,
+      $this->getAboveText(),
+      $this->getBelowText());
+
+    $xfile = id(new PhabricatorTransformedFile())
+      ->setOriginalPHID($template->getPHID())
+      ->setTransformedPHID($asset->getPHID())
+      ->setTransform($hash);
+
+    try {
+      $caught = null;
+
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      try {
+        $xfile->save();
+      } catch (Exception $ex) {
+        $caught = $ex;
+      }
+      unset($unguarded);
+
+      if ($caught) {
+        throw $caught;
+      }
+
+      return $asset;
+    } catch (AphrontDuplicateKeyQueryException $ex) {
+      $xfile = $this->loadCachedFile();
+      if (!$xfile) {
+        throw $ex;
+      }
+      return $xfile;
+    }
+  }
+
+  private function newTransformHash() {
+    $properties = array(
+      'kind' => 'meme',
+      'above' => phutil_utf8_strtoupper($this->getAboveText()),
+      'below' => phutil_utf8_strtoupper($this->getBelowText()),
+    );
+
+    $properties = phutil_json_encode($properties);
+
+    return PhabricatorHash::digestForIndex($properties);
+  }
+
+  public function loadCachedFile() {
+    $viewer = $this->getViewer();
+
+    $template_file = $this->loadTemplateFile();
+    if (!$template_file) {
+      return null;
+    }
+
+    $hash = $this->newTransformHash();
+
+    $xform = id(new PhabricatorTransformedFile())->loadOneWhere(
+      'originalPHID = %s AND transform = %s',
+      $template_file->getPHID(),
+      $hash);
+    if (!$xform) {
+      return null;
+    }
+
+    return id(new PhabricatorFileQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($xform->getTransformedPHID()))
+      ->executeOne();
+  }
+
+  private function loadTemplateFile() {
+    if ($this->templateFile === null) {
+      $viewer = $this->getViewer();
+      $template = $this->getTemplate();
+
+      $macro = id(new PhabricatorMacroQuery())
+        ->setViewer($viewer)
+        ->withNames(array($template))
+        ->needFiles(true)
+        ->executeOne();
+      if (!$macro) {
+        return null;
+      }
+
+      $this->templateFile = $macro->getFile();
+    }
+
+    return $this->templateFile;
+  }
+
+}
diff --git a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
index 278bd403cb..cdee10676c 100644
--- a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
+++ b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
@@ -29,10 +29,14 @@ final class PhabricatorMemeRemarkupRule extends PhutilRemarkupRule {
     $parser = new PhutilSimpleOptions();
     $options = $parser->parse($matches[1]) + $options;
 
-    $uri = id(new PhutilURI('/macro/meme/'))
-      ->alter('macro', $options['src'])
-      ->alter('uppertext', $options['above'])
-      ->alter('lowertext', $options['below']);
+    $engine = id(new PhabricatorMemeEngine())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->setTemplate($options['src'])
+      ->setAboveText($options['above'])
+      ->setBelowText($options['below']);
+
+    $asset = $engine->loadCachedFile();
+    $uri = $engine->getGenerateURI();
 
     if ($this->getEngine()->isHTMLMailMode()) {
       $uri = PhabricatorEnv::getProductionURI($uri);
@@ -50,10 +54,20 @@ final class PhabricatorMemeRemarkupRule extends PhutilRemarkupRule {
         $options['above'],
         $options['below']);
 
-      $img = id(new PHUIRemarkupImageView())
-        ->setURI($uri)
-        ->addClass('phabricator-remarkup-macro')
-        ->setAlt($alt_text);
+      if ($asset) {
+        $img = $this->newTag(
+          'img',
+          array(
+            'src' => $asset->getViewURI(),
+            'class' => 'phabricator-remarkup-macro',
+            'alt' => $alt_text,
+          ));
+      } else {
+        $img = id(new PHUIRemarkupImageView())
+          ->setURI($uri)
+          ->addClass('phabricator-remarkup-macro')
+          ->setAlt($alt_text);
+      }
     }
 
     return $this->getEngine()->storeText($img);

From a3d282d33efe275b14a73f2d2f96197a9059892b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 10:27:29 -0800
Subject: [PATCH 32/37] Somewhat improve meme transform code so it is merely
 very bad

Summary: Depends on D19200. Fixes T5258. Ref T13101. Attempt to simplify and modernize this code and improve error handling.

Test Plan: did real hard dank memes

Maniphest Tasks: T13101, T5258

Differential Revision: https://secure.phabricator.com/D19201
---
 .../files/PhabricatorImageTransformer.php     | 187 --------------
 .../macro/engine/PhabricatorMemeEngine.php    | 228 +++++++++++++++++-
 2 files changed, 223 insertions(+), 192 deletions(-)

diff --git a/src/applications/files/PhabricatorImageTransformer.php b/src/applications/files/PhabricatorImageTransformer.php
index 71f07a231e..1075969a42 100644
--- a/src/applications/files/PhabricatorImageTransformer.php
+++ b/src/applications/files/PhabricatorImageTransformer.php
@@ -6,193 +6,6 @@
  */
 final class PhabricatorImageTransformer extends Phobject {
 
-  public function executeMemeTransform(
-    PhabricatorFile $file,
-    $upper_text,
-    $lower_text) {
-    $image = $this->applyMemeToFile($file, $upper_text, $lower_text);
-    return PhabricatorFile::newFromFileData(
-      $image,
-      array(
-        'name' => 'meme-'.$file->getName(),
-        'ttl.relative' => phutil_units('24 hours in seconds'),
-        'canCDN' => true,
-      ));
-  }
-
-  private function applyMemeToFile(
-    PhabricatorFile $file,
-    $upper_text,
-    $lower_text) {
-    $data = $file->loadFileData();
-
-    $img_type = $file->getMimeType();
-    $imagemagick = PhabricatorEnv::getEnvConfig('files.enable-imagemagick');
-
-    if ($img_type != 'image/gif' || $imagemagick == false) {
-      return $this->applyMemeTo(
-        $data, $upper_text, $lower_text, $img_type);
-    }
-
-    $data = $file->loadFileData();
-    $input = new TempFile();
-    Filesystem::writeFile($input, $data);
-
-    list($out) = execx('convert %s info:', $input);
-    $split = phutil_split_lines($out);
-    if (count($split) > 1) {
-      return $this->applyMemeWithImagemagick(
-        $input,
-        $upper_text,
-        $lower_text,
-        count($split),
-        $img_type);
-    } else {
-      return $this->applyMemeTo($data, $upper_text, $lower_text, $img_type);
-    }
-  }
-
-  private function applyMemeTo(
-    $data,
-    $upper_text,
-    $lower_text,
-    $mime_type) {
-    $img = imagecreatefromstring($data);
-
-    // Some PNGs have color palettes, and allocating the dark border color
-    // fails and gives us whatever's first in the color table. Copy the image
-    // to a fresh truecolor canvas before working with it.
-
-    $truecolor = imagecreatetruecolor(imagesx($img), imagesy($img));
-    imagecopy($truecolor, $img, 0, 0, 0, 0, imagesx($img), imagesy($img));
-    $img = $truecolor;
-
-    $phabricator_root = dirname(phutil_get_library_root('phabricator'));
-    $font_root = $phabricator_root.'/resources/font/';
-    $font_path = $font_root.'tuffy.ttf';
-    if (Filesystem::pathExists($font_root.'impact.ttf')) {
-      $font_path = $font_root.'impact.ttf';
-    }
-    $text_color = imagecolorallocate($img, 255, 255, 255);
-    $border_color = imagecolorallocatealpha($img, 0, 0, 0, 110);
-    $border_width = 4;
-    $font_max = 200;
-    $font_min = 5;
-    for ($i = $font_max; $i > $font_min; $i--) {
-      $fit = $this->doesTextBoundingBoxFitInImage(
-        $img,
-        $upper_text,
-        $i,
-        $font_path);
-      if ($fit['doesfit']) {
-        $x = ($fit['imgwidth'] - $fit['txtwidth']) / 2;
-        $y = $fit['txtheight'] + 10;
-        $this->makeImageWithTextBorder($img,
-          $i,
-          $x,
-          $y,
-          $text_color,
-          $border_color,
-          $border_width,
-          $font_path,
-          $upper_text);
-        break;
-      }
-    }
-    for ($i = $font_max; $i > $font_min; $i--) {
-      $fit = $this->doesTextBoundingBoxFitInImage($img,
-        $lower_text, $i, $font_path);
-      if ($fit['doesfit']) {
-        $x = ($fit['imgwidth'] - $fit['txtwidth']) / 2;
-        $y = $fit['imgheight'] - 10;
-        $this->makeImageWithTextBorder(
-          $img,
-          $i,
-          $x,
-          $y,
-          $text_color,
-          $border_color,
-          $border_width,
-          $font_path,
-          $lower_text);
-        break;
-      }
-    }
-    return self::saveImageDataInAnyFormat($img, $mime_type);
-  }
-
-  private function makeImageWithTextBorder($img, $font_size, $x, $y,
-    $color, $stroke_color, $bw, $font, $text) {
-    $angle = 0;
-    $bw = abs($bw);
-    for ($c1 = $x - $bw; $c1 <= $x + $bw; $c1++) {
-      for ($c2 = $y - $bw; $c2 <= $y + $bw; $c2++) {
-        if (!(($c1 == $x - $bw || $x + $bw) &&
-          $c2 == $y - $bw || $c2 == $y + $bw)) {
-          $bg = imagettftext($img, $font_size,
-            $angle, $c1, $c2, $stroke_color, $font, $text);
-          }
-        }
-      }
-    imagettftext($img, $font_size, $angle,
-            $x , $y, $color , $font, $text);
-  }
-
-  private function doesTextBoundingBoxFitInImage($img,
-    $text, $font_size, $font_path) {
-    // Default Angle = 0
-    $angle = 0;
-
-    $bbox = imagettfbbox($font_size, $angle, $font_path, $text);
-    $text_height = abs($bbox[3] - $bbox[5]);
-    $text_width = abs($bbox[0] - $bbox[2]);
-    return array(
-      'doesfit' => ($text_height * 1.05 <= imagesy($img) / 2
-        && $text_width * 1.05 <= imagesx($img)),
-      'txtwidth' => $text_width,
-      'txtheight' => $text_height,
-      'imgwidth' => imagesx($img),
-      'imgheight' => imagesy($img),
-    );
-  }
-
-  private function applyMemeWithImagemagick(
-    $input,
-    $above,
-    $below,
-    $count,
-    $img_type) {
-
-    $output = new TempFile();
-    $future = new ExecFuture(
-      'convert %s -coalesce +adjoin %s_%s',
-      $input,
-      $input,
-      '%09d');
-    $future->setTimeout(10)->resolvex();
-
-    $output_files = array();
-    for ($ii = 0; $ii < $count; $ii++) {
-      $frame_name = sprintf('%s_%09d', $input, $ii);
-      $output_name = sprintf('%s_%09d', $output, $ii);
-
-      $output_files[] = $output_name;
-
-      $frame_data = Filesystem::readFile($frame_name);
-      $memed_frame_data = $this->applyMemeTo(
-        $frame_data,
-        $above,
-        $below,
-        $img_type);
-      Filesystem::writeFile($output_name, $memed_frame_data);
-    }
-
-    $future = new ExecFuture('convert -loop 0 %Ls %s', $output_files, $output);
-    $future->setTimeout(10)->resolvex();
-
-    return Filesystem::readFile($output);
-  }
-
 
 /* -(  Saving Image Data  )-------------------------------------------------- */
 
diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
index 21a9b44e99..8e0ffbcbba 100644
--- a/src/applications/macro/engine/PhabricatorMemeEngine.php
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -8,6 +8,7 @@ final class PhabricatorMemeEngine extends Phobject {
   private $belowText;
 
   private $templateFile;
+  private $metrics;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -68,11 +69,7 @@ final class PhabricatorMemeEngine extends Phobject {
 
     $hash = $this->newTransformHash();
 
-    $transformer = new PhabricatorImageTransformer();
-    $asset = $transformer->executeMemeTransform(
-      $template,
-      $this->getAboveText(),
-      $this->getBelowText());
+    $asset = $this->newAssetFile($template);
 
     $xfile = id(new PhabricatorTransformedFile())
       ->setOriginalPHID($template->getPHID())
@@ -160,4 +157,225 @@ final class PhabricatorMemeEngine extends Phobject {
     return $this->templateFile;
   }
 
+  private function newAssetFile(PhabricatorFile $template) {
+    $data = $this->newAssetData($template);
+    return PhabricatorFile::newFromFileData(
+      $data,
+      array(
+        'name' => 'meme-'.$template->getName(),
+        'ttl.relative' => phutil_units('24 hours in seconds'),
+        'canCDN' => true,
+      ));
+  }
+
+  private function newAssetData(PhabricatorFile $template) {
+    $template_data = $template->loadFileData();
+
+    $result = $this->newImagemagickAsset($template, $template_data);
+    if ($result) {
+      return $result;
+    }
+
+    return $this->newGDAsset($template, $template_data);
+  }
+
+  private function newImagemagickAsset(
+    PhabricatorFile $template,
+    $template_data) {
+
+    // We're only going to use Imagemagick on GIFs.
+    $mime_type = $template->getMimeType();
+    if ($mime_type != 'image/gif') {
+      return null;
+    }
+
+    // We're only going to use Imagemagick if it is actually available.
+    $available = PhabricatorEnv::getEnvConfig('files.enable-imagemagick');
+    if (!$available) {
+      return null;
+    }
+
+    // Test of the GIF is an animated GIF. If it's a flat GIF, we'll fall
+    // back to GD.
+    $input = new TempFile();
+    Filesystem::writeFile($input, $template_data);
+    list($err, $out) = exec_manual('convert %s info:', $input);
+    if ($err) {
+      return null;
+    }
+
+    $split = phutil_split_lines($out);
+    $frames = count($split);
+    if ($frames <= 1) {
+      return null;
+    }
+
+    // Split the frames apart, transform each frame, then merge them back
+    // together.
+    $output = new TempFile();
+
+    $future = new ExecFuture(
+      'convert %s -coalesce +adjoin %s_%s',
+      $input,
+      $input,
+      '%09d');
+    $future->setTimeout(10)->resolvex();
+
+    $output_files = array();
+    for ($ii = 0; $ii < $frames; $ii++) {
+      $frame_name = sprintf('%s_%09d', $input, $ii);
+      $output_name = sprintf('%s_%09d', $output, $ii);
+
+      $output_files[] = $output_name;
+
+      $frame_data = Filesystem::readFile($frame_name);
+      $memed_frame_data = $this->newGDAsset($template, $frame_data);
+      Filesystem::writeFile($output_name, $memed_frame_data);
+    }
+
+    $future = new ExecFuture('convert -loop 0 %Ls %s', $output_files, $output);
+    $future->setTimeout(10)->resolvex();
+
+    return Filesystem::readFile($output);
+  }
+
+  private function newGDAsset(PhabricatorFile $template, $data) {
+    $img = imagecreatefromstring($data);
+    if (!$img) {
+      throw new Exception(
+        pht('Failed to imagecreatefromstring() image template data.'));
+    }
+
+    $dx = imagesx($img);
+    $dy = imagesy($img);
+
+    $metrics = $this->getMetrics($dx, $dy);
+    $font = $this->getFont();
+    $size = $metrics['size'];
+
+    $above = $this->getAboveText();
+    if (strlen($above)) {
+      $x = (int)floor(($dx - $metrics['text']['above']['width']) / 2);
+      $y = $metrics['text']['above']['height'] + 12;
+
+      $this->drawText($img, $font, $metrics['size'], $x, $y, $above);
+    }
+
+    $below = $this->getBelowText();
+    if (strlen($below)) {
+      $x = (int)floor(($dx - $metrics['text']['below']['width']) / 2);
+      $y = $dy - 12 - $metrics['text']['below']['descend'];
+
+      $this->drawText($img, $font, $metrics['size'], $x, $y, $below);
+    }
+
+    return PhabricatorImageTransformer::saveImageDataInAnyFormat(
+      $img,
+      $template->getMimeType());
+  }
+
+  private function getFont() {
+    $phabricator_root = dirname(phutil_get_library_root('phabricator'));
+
+    $font_root = $phabricator_root.'/resources/font/';
+    if (Filesystem::pathExists($font_root.'impact.ttf')) {
+      $font_path = $font_root.'impact.ttf';
+    } else {
+      $font_path = $font_root.'tuffy.ttf';
+    }
+
+    return $font_path;
+  }
+
+  private function getMetrics($dim_x, $dim_y) {
+    if ($this->metrics === null) {
+      $font = $this->getFont();
+
+      $font_max = 72;
+      $font_min = 5;
+
+      $last = null;
+      $cursor = floor(($font_max + $font_min) / 2);
+      $min = $font_min;
+      $max = $font_max;
+
+      $texts = array(
+        'above' => $this->getAboveText(),
+        'below' => $this->getBelowText(),
+      );
+
+      $metrics = null;
+      $best = null;
+      while (true) {
+        $all_fit = true;
+        $text_metrics = array();
+        foreach ($texts as $key => $text) {
+          $box = imagettfbbox($cursor, 0, $font, $text);
+          $height = abs($box[3] - $box[5]);
+          $width = abs($box[0] - $box[2]);
+
+          // This is the number of pixels below the baseline that the
+          // text extends, for example if it has a "y".
+          $descend = $box[3];
+
+          if ($height > $dim_y) {
+            $all_fit = false;
+            break;
+          }
+
+          if ($width > $dim_x) {
+            $all_fit = false;
+            break;
+          }
+
+          $text_metrics[$key]['width'] = $width;
+          $text_metrics[$key]['height'] = $height;
+          $text_metrics[$key]['descend'] = $descend;
+        }
+
+        if ($all_fit || $best === null) {
+          $best = $cursor;
+          $metrics = $text_metrics;
+        }
+
+        if ($all_fit) {
+          $min = $cursor;
+        } else {
+          $max = $cursor;
+        }
+
+        $last = $cursor;
+        $cursor = floor(($max + $min) / 2);
+        if ($cursor === $last) {
+          break;
+        }
+      }
+
+      $this->metrics = array(
+        'size' => $best,
+        'text' => $metrics,
+      );
+    }
+
+    return $this->metrics;
+  }
+
+  private function drawText($img, $font, $size, $x, $y, $text) {
+    $text_color = imagecolorallocate($img, 255, 255, 255);
+    $border_color = imagecolorallocate($img, 0, 0, 0);
+
+    $border = 2;
+    for ($xx = ($x - $border); $xx <= ($x + $border); $xx += $border) {
+      for ($yy = ($y - $border); $yy <= ($y + $border); $yy += $border) {
+        if (($xx === $x) && ($yy === $y)) {
+          continue;
+        }
+        imagettftext($img, $size, 0, $xx, $yy, $border_color, $font, $text);
+      }
+    }
+
+    imagettftext($img, $size, 0, $x, $y, $text_color, $font, $text);
+  }
+
+
 }

From 10b3ddf426573f4d4b01ca47ca313d8856fdabcd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 11:02:40 -0800
Subject: [PATCH 33/37] Possibly fix memes in email

Summary:
Depends on D19201. Ref T13101. This likely produces relatively stable-ish image references for email.

They currently TTL after 30 days but this makes the jokes more exclusive and special so it's a feature, not a bug.

Test Plan: I'm just going to test this in production because I'm a ninja superstar developer.

Maniphest Tasks: T13101

Differential Revision: https://secure.phabricator.com/D19203
---
 .../macro/engine/PhabricatorMemeEngine.php    |  5 +-
 .../markup/PhabricatorMemeRemarkupRule.php    | 79 +++++++++++++------
 2 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
index 8e0ffbcbba..f72a161171 100644
--- a/src/applications/macro/engine/PhabricatorMemeEngine.php
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -163,8 +163,11 @@ final class PhabricatorMemeEngine extends Phobject {
       $data,
       array(
         'name' => 'meme-'.$template->getName(),
-        'ttl.relative' => phutil_units('24 hours in seconds'),
         'canCDN' => true,
+
+        // In modern code these can end up linked directly in email, so let
+        // them stick around for a while.
+        'ttl.relative' => phutil_units('30 days in seconds'),
       ));
   }
 
diff --git a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
index cdee10676c..eacb1a33ed 100644
--- a/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
+++ b/src/applications/macro/markup/PhabricatorMemeRemarkupRule.php
@@ -36,38 +36,65 @@ final class PhabricatorMemeRemarkupRule extends PhutilRemarkupRule {
       ->setBelowText($options['below']);
 
     $asset = $engine->loadCachedFile();
-    $uri = $engine->getGenerateURI();
 
-    if ($this->getEngine()->isHTMLMailMode()) {
-      $uri = PhabricatorEnv::getProductionURI($uri);
+    $is_html_mail = $this->getEngine()->isHTMLMailMode();
+    $is_text = $this->getEngine()->isTextMode();
+    $must_inline = ($is_html_mail || $is_text);
+
+    if ($must_inline) {
+      if (!$asset) {
+        try {
+          $asset = $engine->newAsset();
+        } catch (Exception $ex) {
+          return $matches[0];
+        }
+      }
     }
 
-    if ($this->getEngine()->isTextMode()) {
-      $img =
-        ($options['above'] != '' ? "\"{$options['above']}\"\n" : '').
-        $options['src'].' <'.PhabricatorEnv::getProductionURI($uri).'>'.
-        ($options['below'] != '' ? "\n\"{$options['below']}\"" : '');
+    if ($asset) {
+      $uri = $asset->getViewURI();
     } else {
-      $alt_text = pht(
-        'Macro %s: %s %s',
-        $options['src'],
-        $options['above'],
-        $options['below']);
+      $uri = $engine->getGenerateURI();
+    }
 
-      if ($asset) {
-        $img = $this->newTag(
-          'img',
-          array(
-            'src' => $asset->getViewURI(),
-            'class' => 'phabricator-remarkup-macro',
-            'alt' => $alt_text,
-          ));
-      } else {
-        $img = id(new PHUIRemarkupImageView())
-          ->setURI($uri)
-          ->addClass('phabricator-remarkup-macro')
-          ->setAlt($alt_text);
+    if ($is_text) {
+      $parts = array();
+
+      $above = $options['above'];
+      if (strlen($above)) {
+        $parts[] = pht('"%s"', $above);
       }
+
+      $parts[] = $options['src'].' <'.$uri.'>';
+
+      $below = $options['below'];
+      if (strlen($below)) {
+        $parts[] = pht('"%s"', $below);
+      }
+
+      $parts = implode("\n", $parts);
+      return $this->getEngine()->storeText($parts);
+    }
+
+    $alt_text = pht(
+      'Macro %s: %s %s',
+      $options['src'],
+      $options['above'],
+      $options['below']);
+
+    if ($asset) {
+      $img = $this->newTag(
+        'img',
+        array(
+          'src' => $uri,
+          'class' => 'phabricator-remarkup-macro',
+          'alt' => $alt_text,
+        ));
+    } else {
+      $img = id(new PHUIRemarkupImageView())
+        ->setURI($uri)
+        ->addClass('phabricator-remarkup-macro')
+        ->setAlt($alt_text);
     }
 
     return $this->getEngine()->storeText($img);

From 2de06a5375c6b786d8f32d69bc98a695cdbed9ce Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 8 Mar 2018 11:52:48 -0800
Subject: [PATCH 34/37] Add some more UI reminder text about draft revisions

Summary: See PHI433. This beefs up reminder texts for drafts a little bit since some users in the wild aren't always seeing/remembering the existing, fairly subtle hints.

Test Plan: Created a reivsion with `--draft`, viewed it, saw richer reminders.

Differential Revision: https://secure.phabricator.com/D19204
---
 .../constants/DifferentialRevisionStatus.php  |  6 ++---
 .../customfield/DifferentialDraftField.php    | 24 +++++++++++++++++--
 .../editor/DifferentialRevisionEditEngine.php |  8 +++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/src/applications/differential/constants/DifferentialRevisionStatus.php b/src/applications/differential/constants/DifferentialRevisionStatus.php
index 70f0e33b28..e3acace3f0 100644
--- a/src/applications/differential/constants/DifferentialRevisionStatus.php
+++ b/src/applications/differential/constants/DifferentialRevisionStatus.php
@@ -172,10 +172,10 @@ final class DifferentialRevisionStatus extends Phobject {
         'name' => pht('Draft'),
         // For legacy clients, treat this as though it is "Needs Review".
         'legacy' => 0,
-        'icon' => 'fa-file-text-o',
+        'icon' => 'fa-spinner',
         'closed' => false,
-        'color.icon' => 'grey',
-        'color.tag' => 'grey',
+        'color.icon' => 'pink',
+        'color.tag' => 'pink',
         'color.ansi' => null,
       ),
     );
diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php
index 5d625e3ce9..8147708bca 100644
--- a/src/applications/differential/customfield/DifferentialDraftField.php
+++ b/src/applications/differential/customfield/DifferentialDraftField.php
@@ -37,9 +37,14 @@ final class DifferentialDraftField
     }
 
     // If the author has held this revision as a draft explicitly, don't
-    // show any misleading messages about it autosubmitting later.
+    // show any misleading messages about it autosubmitting later. We do show
+    // reminder text.
     if ($revision->getHoldAsDraft()) {
-      return array();
+      return array(
+        pht(
+          'This is a draft revision that has not yet been submitted for '.
+          'review.'),
+      );
     }
 
     $warnings = array();
@@ -93,4 +98,19 @@ final class DifferentialDraftField
     return $warnings;
   }
 
+  public function getWarningsForDetailView() {
+    $revision = $this->getObject();
+
+    if (!$revision->isDraft()) {
+      return array();
+    }
+
+    return array(
+      pht(
+        'This revision is currently a draft. You can leave comments, but '.
+        'no one will be notified until the revision is submitted for '.
+        'review.'),
+    );
+  }
+
 }
diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index fe1b983f1a..d462fd7571 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -71,6 +71,14 @@ final class DifferentialRevisionEditEngine
     return pht('Revision');
   }
 
+  protected function getCommentViewButtonText($object) {
+    if ($object->isDraft()) {
+      return pht('Submit Quietly');
+    }
+
+    return parent::getCommentViewButtonText();
+  }
+
   protected function getObjectViewURI($object) {
     return $object->getURI();
   }

From 1763b516b158d5efad344111d332da524a129e75 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 9 Mar 2018 05:19:06 -0800
Subject: [PATCH 35/37] Fix missing parameter in parent call for Differential
 button text

Summary: See <https://discourse.phabricator-community.org/t/openning-any-differential-fails-with-undefined-variable-object/1216/1>.

Test Plan: Loaded any //non//-draft revision.

Differential Revision: https://secure.phabricator.com/D19205
---
 .../differential/editor/DifferentialRevisionEditEngine.php      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
index d462fd7571..0404bd6201 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php
@@ -76,7 +76,7 @@ final class DifferentialRevisionEditEngine
       return pht('Submit Quietly');
     }
 
-    return parent::getCommentViewButtonText();
+    return parent::getCommentViewButtonText($object);
   }
 
   protected function getObjectViewURI($object) {

From 3e992c671372cafb3bee5ba64a9720b7185fc814 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 9 Mar 2018 12:06:17 -0800
Subject: [PATCH 36/37] Add audit, review, and dominion information to
 "owners.search" API method

Summary:
See PHI439. This fills in additional information about Owners packages.

Also removes dead `primaryOwnerPHID`.

Test Plan: Called `owners.search` and reviewed the results. Grepped for `primaryOwnerPHID`.

Differential Revision: https://secure.phabricator.com/D19207
---
 .../20180309.owners.01.primaryowner.sql       |  2 +
 .../storage/PhabricatorOwnersPackage.php      | 43 ++++++++++++++++++-
 2 files changed, 43 insertions(+), 2 deletions(-)
 create mode 100644 resources/sql/autopatches/20180309.owners.01.primaryowner.sql

diff --git a/resources/sql/autopatches/20180309.owners.01.primaryowner.sql b/resources/sql/autopatches/20180309.owners.01.primaryowner.sql
new file mode 100644
index 0000000000..a5eb4368f2
--- /dev/null
+++ b/resources/sql/autopatches/20180309.owners.01.primaryowner.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  DROP primaryOwnerPHID;
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 7f155d7c89..eb17a3890e 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -16,7 +16,6 @@ final class PhabricatorOwnersPackage
   protected $auditingEnabled;
   protected $autoReview;
   protected $description;
-  protected $primaryOwnerPHID;
   protected $mailKey;
   protected $status;
   protected $viewPolicy;
@@ -122,7 +121,6 @@ final class PhabricatorOwnersPackage
       self::CONFIG_COLUMN_SCHEMA => array(
         'name' => 'sort',
         'description' => 'text',
-        'primaryOwnerPHID' => 'phid?',
         'auditingEnabled' => 'bool',
         'mailKey' => 'bytes20',
         'status' => 'text32',
@@ -601,6 +599,18 @@ final class PhabricatorOwnersPackage
         ->setKey('owners')
         ->setType('list<map<string, wild>>')
         ->setDescription(pht('List of package owners.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('review')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Auto review information.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('audit')
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Auto audit information.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('dominion')
+        ->setType('string')
+        ->setDescription(pht('Dominion setting.')),
     );
   }
 
@@ -612,11 +622,40 @@ final class PhabricatorOwnersPackage
       );
     }
 
+    $review_map = self::getAutoreviewOptionsMap();
+    $review_value = $this->getAutoReview();
+    if (isset($review_map[$review_value])) {
+      $review_label = $review_map[$review_value]['name'];
+    } else {
+      $review_label = pht('Unknown ("%s")', $review_value);
+    }
+
+    $review = array(
+      'value' => $review_value,
+      'label' => $review_label,
+    );
+
+    if ($this->getAuditingEnabled()) {
+      $audit_value = 'audit';
+      $audit_label = pht('Auditing Enabled');
+    } else {
+      $audit_value = 'none';
+      $audit_label = pht('No Auditing');
+    }
+
+    $audit = array(
+      'value' => $audit_value,
+      'label' => $audit_label,
+    );
+
     return array(
       'name' => $this->getName(),
       'description' => $this->getDescription(),
       'status' => $this->getStatus(),
       'owners' => $owner_list,
+      'review' => $review,
+      'audit' => $audit,
+      'dominion' => $this->getDominion(),
     );
   }
 

From 9d0cf3c8b82d550de7f6b44801b29796d688c493 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 9 Mar 2018 12:16:10 -0800
Subject: [PATCH 37/37] Before anyone notices, break the API

Summary: See PHI439. Use slightly richer "dominion" return values for consistency.

Test Plan: Fetched results with `owners.search` API method.

Differential Revision: https://secure.phabricator.com/D19208
---
 .../storage/PhabricatorOwnersPackage.php      | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index eb17a3890e..56dd51a3c6 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -609,8 +609,8 @@ final class PhabricatorOwnersPackage
         ->setDescription(pht('Auto audit information.')),
       id(new PhabricatorConduitSearchFieldSpecification())
         ->setKey('dominion')
-        ->setType('string')
-        ->setDescription(pht('Dominion setting.')),
+        ->setType('map<string, wild>')
+        ->setDescription(pht('Dominion setting information.')),
     );
   }
 
@@ -648,6 +648,22 @@ final class PhabricatorOwnersPackage
       'label' => $audit_label,
     );
 
+    $dominion_value = $this->getDominion();
+    $dominion_map = self::getDominionOptionsMap();
+    if (isset($dominion_map[$dominion_value])) {
+      $dominion_label = $dominion_map[$dominion_value]['name'];
+      $dominion_short = $dominion_map[$dominion_value]['short'];
+    } else {
+      $dominion_label = pht('Unknown ("%s")', $dominion_value);
+      $dominion_short = pht('Unknown ("%s")', $dominion_value);
+    }
+
+    $dominion = array(
+      'value' => $dominion_value,
+      'label' => $dominion_label,
+      'short' => $dominion_short,
+    );
+
     return array(
       'name' => $this->getName(),
       'description' => $this->getDescription(),
@@ -655,7 +671,7 @@ final class PhabricatorOwnersPackage
       'owners' => $owner_list,
       'review' => $review,
       'audit' => $audit,
-      'dominion' => $this->getDominion(),
+      'dominion' => $dominion,
     );
   }