From ad53e7b878159eab1648962dc9cfd71474e321b3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 05:58:44 -0700
Subject: [PATCH 01/39] Record how long storage patches took to apply

Summary:
It's hard for us to predict how long patches and migrations will take in the general case since it varies a lot from install to install, but we can give installs some kind of rough heads up about longer patches. I'm planning to just put a sort of hint for things in the changelog, something like this:

{F905579}

To make this easier, start storing how long stuff took. I'll write a little script to dump this into a table for the changelog.

Test Plan:
Ran `bin/storage status`:

{F905580}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14320
---
 .../autopatches/20151023.patchduration.sql    |  2 +
 .../PhabricatorStorageManagementAPI.php       | 40 +++++++++++++++++--
 ...ricatorStorageManagementStatusWorkflow.php | 11 +++++
 ...icatorStorageManagementUpgradeWorkflow.php |  6 ++-
 .../schema/PhabricatorStorageSchemaSpec.php   |  1 +
 5 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100644 resources/sql/autopatches/20151023.patchduration.sql

diff --git a/resources/sql/autopatches/20151023.patchduration.sql b/resources/sql/autopatches/20151023.patchduration.sql
new file mode 100644
index 0000000000..3e0c363931
--- /dev/null
+++ b/resources/sql/autopatches/20151023.patchduration.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_meta_data.patch_status
+  ADD duration BIGINT UNSIGNED;
diff --git a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php
index 0d8fc79bb0..76e8e0a057 100644
--- a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php
+++ b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php
@@ -17,6 +17,8 @@ final class PhabricatorStorageManagementAPI extends Phobject {
   const COLLATE_SORT = 'COLLATE_SORT';
   const COLLATE_FULLTEXT = 'COLLATE_FULLTEXT';
 
+  const TABLE_STATUS = 'patch_status';
+
   public function setDisableUTF8MB4($disable_utf8_mb4) {
     $this->disableUTF8MB4 = $disable_utf8_mb4;
     return $this;
@@ -118,13 +120,26 @@ final class PhabricatorStorageManagementAPI extends Phobject {
     try {
       $applied = queryfx_all(
         $this->getConn('meta_data'),
-        'SELECT patch FROM patch_status');
+        'SELECT patch FROM %T',
+        self::TABLE_STATUS);
       return ipull($applied, 'patch');
     } catch (AphrontQueryException $ex) {
       return null;
     }
   }
 
+  public function getPatchDurations() {
+    try {
+      $rows = queryfx_all(
+        $this->getConn('meta_data'),
+        'SELECT patch, duration FROM %T WHERE duration IS NOT NULL',
+        self::TABLE_STATUS);
+      return ipull($rows, 'duration', 'patch');
+    } catch (AphrontQueryException $ex) {
+      return array();
+    }
+  }
+
   public function createDatabase($fragment) {
     $info = $this->getCharsetInfo();
 
@@ -168,13 +183,30 @@ final class PhabricatorStorageManagementAPI extends Phobject {
     return $legacy;
   }
 
-  public function markPatchApplied($patch) {
+  public function markPatchApplied($patch, $duration = null) {
+    $conn = $this->getConn('meta_data');
+
     queryfx(
-      $this->getConn('meta_data'),
+      $conn,
       'INSERT INTO %T (patch, applied) VALUES (%s, %d)',
-      'patch_status',
+      self::TABLE_STATUS,
       $patch,
       time());
+
+    // We didn't add this column for a long time, so it may not exist yet.
+    if ($duration !== null) {
+      try {
+        queryfx(
+          $conn,
+          'UPDATE %T SET duration = %d WHERE patch = %s',
+          self::TABLE_STATUS,
+          (int)floor($duration * 1000000),
+          $patch);
+      } catch (AphrontQueryException $ex) {
+        // Just ignore this, as it almost certainly indicates that we just
+        // don't have the column yet.
+      }
+    }
   }
 
   public function applyPatch(PhabricatorStoragePatch $patch) {
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php
index ebff080bd4..774ae8445c 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementStatusWorkflow.php
@@ -29,15 +29,26 @@ final class PhabricatorStorageManagementStatusWorkflow
       ->setShowHeader(false)
       ->addColumn('id',     array('title' => pht('ID')))
       ->addColumn('status', array('title' => pht('Status')))
+      ->addColumn('duration', array('title' => pht('Duration')))
       ->addColumn('type',   array('title' => pht('Type')))
       ->addColumn('name',   array('title' => pht('Name')));
 
+    $durations = $api->getPatchDurations();
+
     foreach ($patches as $patch) {
+      $duration = idx($durations, $patch->getFullKey());
+      if ($duration === null) {
+        $duration = '-';
+      } else {
+        $duration = pht('%s us', new PhutilNumber($duration));
+      }
+
       $table->addRow(array(
         'id' => $patch->getFullKey(),
         'status' => in_array($patch->getFullKey(), $applied)
           ? pht('Applied')
           : pht('Not Applied'),
+        'duration' => $duration,
         'type' => $patch->getType(),
         'name' => $patch->getName(),
       ));
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementUpgradeWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementUpgradeWorkflow.php
index 55cf3b223e..d5a4f41cad 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementUpgradeWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementUpgradeWorkflow.php
@@ -187,9 +187,13 @@ final class PhabricatorStorageManagementUpgradeWorkflow
           echo pht("DRYRUN: Would apply patch '%s'.", $key)."\n";
         } else {
           echo pht("Applying patch '%s'...", $key)."\n";
+
+          $t_begin = microtime(true);
           $api->applyPatch($patch);
+          $t_end = microtime(true);
+
           if (!$skip_mark) {
-            $api->markPatchApplied($key);
+            $api->markPatchApplied($key, ($t_end - $t_begin));
           }
         }
 
diff --git a/src/infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php b/src/infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php
index 886df14e2c..df48ce3812 100644
--- a/src/infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php
+++ b/src/infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php
@@ -10,6 +10,7 @@ final class PhabricatorStorageSchemaSpec
       array(
         'patch' => 'text128',
         'applied' => 'uint32',
+        'duration' => 'uint64?',
       ),
       array(
         'PRIMARY' => array(

From 32dc62955a3c00f4a90330aa168b049c8ff98634 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 08:13:14 -0700
Subject: [PATCH 02/39] Disable "Send Message" profile action if viewer is
 logged out

Summary: Fixes T9598.

Test Plan:
  - Used "Send Message" as a logged-in user.
  - Used "Send Message" as a logged-out user. The action was disabled and clicking it popped up a login dialog.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9598

Differential Revision: https://secure.phabricator.com/D14326
---
 .../people/controller/PhabricatorPeopleProfileController.php   | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php
index 30af5057dd..02e3a2f27d 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php
@@ -72,11 +72,14 @@ final class PhabricatorPeopleProfileController
       $href = id(new PhutilURI('/conpherence/new/'))
         ->setQueryParam('participant', $user->getPHID());
 
+      $can_send = $viewer->isLoggedIn();
+
       $actions->addAction(
         id(new PhabricatorActionView())
           ->setIcon('fa-comments')
           ->setName(pht('Send Message'))
           ->setWorkflow(true)
+          ->setDisabled(!$can_send)
           ->setHref($href));
     }
 

From 1582bb54f649f59057223a0f51614665bc9e3505 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 08:13:22 -0700
Subject: [PATCH 03/39] Move version numbers to a dedicated "Versions" panel

Summary:
Currently, Version numbers are sort of randomly shown on "All Settings" beacuse we didn't have any better place to put them.

Now that we have modules, expose them as a config module.

Test Plan:
{F906426}

Grepped for "all settings" to look for other references to the old location, but didn't get any relevant hits.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14327
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorConfigAllController.php        | 59 --------------
 .../PhabricatorConfigVersionsModule.php       | 79 +++++++++++++++++++
 src/docs/contributor/bug_reports.diviner      | 11 +--
 4 files changed, 87 insertions(+), 64 deletions(-)
 create mode 100644 src/applications/config/module/PhabricatorConfigVersionsModule.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index d00b3a377d..41ae622d45 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1914,6 +1914,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigTransaction' => 'applications/config/storage/PhabricatorConfigTransaction.php',
     'PhabricatorConfigTransactionQuery' => 'applications/config/query/PhabricatorConfigTransactionQuery.php',
     'PhabricatorConfigValidationException' => 'applications/config/exception/PhabricatorConfigValidationException.php',
+    'PhabricatorConfigVersionsModule' => 'applications/config/module/PhabricatorConfigVersionsModule.php',
     'PhabricatorConfigWelcomeController' => 'applications/config/controller/PhabricatorConfigWelcomeController.php',
     'PhabricatorConpherenceApplication' => 'applications/conpherence/application/PhabricatorConpherenceApplication.php',
     'PhabricatorConpherencePreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorConpherencePreferencesSettingsPanel.php',
@@ -5893,6 +5894,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorConfigValidationException' => 'Exception',
+    'PhabricatorConfigVersionsModule' => 'PhabricatorConfigModule',
     'PhabricatorConfigWelcomeController' => 'PhabricatorConfigController',
     'PhabricatorConpherenceApplication' => 'PhabricatorApplication',
     'PhabricatorConpherencePreferencesSettingsPanel' => 'PhabricatorSettingsPanel',
diff --git a/src/applications/config/controller/PhabricatorConfigAllController.php b/src/applications/config/controller/PhabricatorConfigAllController.php
index 17bf65fd7b..d805168eb1 100644
--- a/src/applications/config/controller/PhabricatorConfigAllController.php
+++ b/src/applications/config/controller/PhabricatorConfigAllController.php
@@ -58,31 +58,10 @@ final class PhabricatorConfigAllController
     $panel->setHeaderText(pht('Current Settings'));
     $panel->setTable($table);
 
-    $versions = $this->loadVersions();
-
-    $version_property_list = id(new PHUIPropertyListView());
-    foreach ($versions as $version) {
-      list($name, $hash) = $version;
-      $version_property_list->addProperty($name, $hash);
-    }
-
-    $object_box = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Current Version'))
-      ->addPropertyList($version_property_list);
-
-    $phabricator_root = dirname(phutil_get_library_root('phabricator'));
-    $version_path = $phabricator_root.'/conf/local/VERSION';
-    if (Filesystem::pathExists($version_path)) {
-      $version_from_file = Filesystem::readFile($version_path);
-      $version_property_list->addProperty(
-        pht('Local Version'),
-        $version_from_file);
-    }
 
     $nav = $this->buildSideNavView();
     $nav->selectFilter('all/');
     $nav->setCrumbs($crumbs);
-    $nav->appendChild($object_box);
     $nav->appendChild($panel);
 
 
@@ -93,42 +72,4 @@ final class PhabricatorConfigAllController
       ));
   }
 
-  private function loadVersions() {
-    $specs = array(
-      array(
-        'name' => pht('Phabricator Version'),
-        'root' => 'phabricator',
-      ),
-      array(
-        'name' => pht('Arcanist Version'),
-        'root' => 'arcanist',
-      ),
-      array(
-        'name' => pht('libphutil Version'),
-        'root' => 'phutil',
-      ),
-    );
-
-    $futures = array();
-    foreach ($specs as $key => $spec) {
-      $root = dirname(phutil_get_library_root($spec['root']));
-      $futures[$key] = id(new ExecFuture('git log --format=%%H -n 1 --'))
-        ->setCWD($root);
-    }
-
-    $results = array();
-    foreach ($futures as $key => $future) {
-      list($err, $stdout) = $future->resolve();
-      if (!$err) {
-        $name = trim($stdout);
-      } else {
-        $name = pht('Unknown');
-      }
-      $results[$key] = array($specs[$key]['name'], $name);
-    }
-
-    return array_select_keys($results, array_keys($specs));
-  }
-
-
 }
diff --git a/src/applications/config/module/PhabricatorConfigVersionsModule.php b/src/applications/config/module/PhabricatorConfigVersionsModule.php
new file mode 100644
index 0000000000..b3240aec51
--- /dev/null
+++ b/src/applications/config/module/PhabricatorConfigVersionsModule.php
@@ -0,0 +1,79 @@
+<?php
+
+final class PhabricatorConfigVersionsModule
+  extends PhabricatorConfigModule {
+
+  public function getModuleKey() {
+    return 'versions';
+  }
+
+  public function getModuleName() {
+    return pht('Versions');
+  }
+
+  public function renderModuleStatus(AphrontRequest $request) {
+    $viewer = $request->getViewer();
+
+
+    $versions = $this->loadVersions();
+
+    $version_property_list = id(new PHUIPropertyListView());
+    foreach ($versions as $version) {
+      list($name, $hash) = $version;
+      $version_property_list->addProperty($name, $hash);
+    }
+
+    $object_box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Current Versions'))
+      ->addPropertyList($version_property_list);
+
+    $phabricator_root = dirname(phutil_get_library_root('phabricator'));
+    $version_path = $phabricator_root.'/conf/local/VERSION';
+    if (Filesystem::pathExists($version_path)) {
+      $version_from_file = Filesystem::readFile($version_path);
+      $version_property_list->addProperty(
+        pht('Local Version'),
+        $version_from_file);
+    }
+
+    return $object_box;
+  }
+
+  private function loadVersions() {
+    $specs = array(
+      array(
+        'name' => pht('Phabricator Version'),
+        'root' => 'phabricator',
+      ),
+      array(
+        'name' => pht('Arcanist Version'),
+        'root' => 'arcanist',
+      ),
+      array(
+        'name' => pht('libphutil Version'),
+        'root' => 'phutil',
+      ),
+    );
+
+    $futures = array();
+    foreach ($specs as $key => $spec) {
+      $root = dirname(phutil_get_library_root($spec['root']));
+      $futures[$key] = id(new ExecFuture('git log --format=%%H -n 1 --'))
+        ->setCWD($root);
+    }
+
+    $results = array();
+    foreach ($futures as $key => $future) {
+      list($err, $stdout) = $future->resolve();
+      if (!$err) {
+        $name = trim($stdout);
+      } else {
+        $name = pht('Unknown');
+      }
+      $results[$key] = array($specs[$key]['name'], $name);
+    }
+
+    return array_select_keys($results, array_keys($specs));
+  }
+
+}
diff --git a/src/docs/contributor/bug_reports.diviner b/src/docs/contributor/bug_reports.diviner
index 0ad3635d87..46ac466d83 100644
--- a/src/docs/contributor/bug_reports.diviner
+++ b/src/docs/contributor/bug_reports.diviner
@@ -69,11 +69,12 @@ To update Phabricator, use a script like the one described in
 @{article:Upgrading Phabricator}.
 
 **If you can not update** for some reason, please include the version of
-Phabricator you are running in your report. The version is just the Git hash
-of your local HEAD. You can find the version by running `git show` in
-`phabricator/` and copy/pasting the first line of output, or by browsing to
-{nav Config > All Settings} in the web UI and copy/pasting the information
-at the top.
+Phabricator you are running when you file a report. You can find the version in
+{nav Config > Versions} in the web UI.
+
+(The version is just the Git hash of your local HEAD, so you can also find it
+by running `git show` in `phabricator/` and looking at the first line of
+output.)
 
 
 Supported Issues

From b3d8ea88ecad324072bf8ff9c480babd2d644c6a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 08:13:28 -0700
Subject: [PATCH 04/39] Update a couple of links in "Feature Requests"
 documentation

Summary:
These are a little out of date:

  - Link to Starmap since it explicitly exists now.
  - Link to "Planning" instead of the old task.
  - Link to "Prioritization" instead of telling anyone to build stuff themselves.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14328
---
 src/docs/contributor/feature_requests.diviner | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/docs/contributor/feature_requests.diviner b/src/docs/contributor/feature_requests.diviner
index a5f8b610c0..a1d2a6640c 100644
--- a/src/docs/contributor/feature_requests.diviner
+++ b/src/docs/contributor/feature_requests.diviner
@@ -114,7 +114,8 @@ We have a lot of users and a small team. Even if your feature is something we're
 interested in and a good fit for where we want the product to go, it may take
 us a long time to get around to building it.
 
-We work full time on Phabricator, and our long-term roadmap has many years worth
+We work full time on Phabricator, and our long-term roadmap (which we call our
+[[ https://secure.phabricator.com/w/starmap/ | Starmap ]]) has many years worth
 of work. Your feature request is competing against thousands of other requests
 for priority.
 
@@ -127,7 +128,8 @@ Even if your feature request is simple and has substantial impact for a large
 number of users, the size of the request queue means that it is mathematically
 unlikely to be near the top.
 
-You can find some information about how we prioritize in T4778. In particular,
+You can find some information about how we prioritize in
+[[ https://secure.phabricator.com/w/planning/ | Planning ]]. In particular,
 we reprioritize frequently and can not accurately predict when we'll build a
 feature which isn't very near to top of the queue.
 
@@ -137,9 +139,9 @@ give you any updates or predictions about timelines. One day, out of nowhere,
 your feature will materialize. That day may be a decade from now. You should
 have realistic expectations about this when filing a feature request.
 
-If you want a concrete timeline, you can build the feature yourself. See
-@{article:Contributing Code} for details and alternatives to working with the
-upstream.
+If you want a concrete timeline, you can work with us to pay for some control
+over our roadmap. For details, see
+[[ https://secure.phabricator.com/w/prioritization/ | Prioritization ]].
 
 
 Describe Problems

From 58957e62c1fa58bd180f187ed41d271343b26195 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 08:16:23 -0700
Subject: [PATCH 05/39] Show applications and icons for PHID types in config
 table

Summary: Ref T9625. Some PHID types are missing application or icon specifications. This makes it easier to spot them.

Test Plan: {F906321}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9625

Differential Revision: https://secure.phabricator.com/D14323
---
 .../module/PhabricatorConfigPHIDModule.php    | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/src/applications/config/module/PhabricatorConfigPHIDModule.php b/src/applications/config/module/PhabricatorConfigPHIDModule.php
index 6c363e3bee..4c5c09f9c3 100644
--- a/src/applications/config/module/PhabricatorConfigPHIDModule.php
+++ b/src/applications/config/module/PhabricatorConfigPHIDModule.php
@@ -18,9 +18,35 @@ final class PhabricatorConfigPHIDModule extends PhabricatorConfigModule {
 
     $rows = array();
     foreach ($types as $key => $type) {
+      $class_name = $type->getPHIDTypeApplicationClass();
+      if ($class_name !== null) {
+        $app = PhabricatorApplication::getByClass($class_name);
+        $app_name = $app->getName();
+
+        $icon = $app->getFontIcon();
+        if ($icon) {
+          $app_icon = id(new PHUIIconView())->setIconFont($icon);
+        } else {
+          $app_icon = null;
+        }
+      } else {
+        $app_name = null;
+        $app_icon = null;
+      }
+
+      $icon = $type->getTypeIcon();
+      if ($icon) {
+        $type_icon = id(new PHUIIconView())->setIconFont($icon);
+      } else {
+        $type_icon = null;
+      }
+
       $rows[] = array(
         $type->getTypeConstant(),
         get_class($type),
+        $app_icon,
+        $app_name,
+        $type_icon,
         $type->getTypeName(),
       );
     }
@@ -30,12 +56,18 @@ final class PhabricatorConfigPHIDModule extends PhabricatorConfigModule {
         array(
           pht('Constant'),
           pht('Class'),
+          null,
+          pht('Application'),
+          null,
           pht('Name'),
         ))
       ->setColumnClasses(
         array(
           null,
           'pri',
+          'icon',
+          null,
+          'icon',
           'wide',
         ));
 

From d0098bc43626ef6b69a2f177dcee6fa8ab2c30ed Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 09:41:13 -0700
Subject: [PATCH 06/39] Provide an application link for the Macro PHID type

Summary: Ref T9625. This is an example of how to fill in the missing calls.

Test Plan:
  - Verified that an icon is now shown for feed stories.
  - Verified that an icon is now shown in the "PHID Types" module panel in Config.

{F906325}

{F906326}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9625

Differential Revision: https://secure.phabricator.com/D14324
---
 src/applications/macro/phid/PhabricatorMacroMacroPHIDType.php | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/macro/phid/PhabricatorMacroMacroPHIDType.php b/src/applications/macro/phid/PhabricatorMacroMacroPHIDType.php
index 7d2dcfc9ad..19c7f5a1fc 100644
--- a/src/applications/macro/phid/PhabricatorMacroMacroPHIDType.php
+++ b/src/applications/macro/phid/PhabricatorMacroMacroPHIDType.php
@@ -8,6 +8,10 @@ final class PhabricatorMacroMacroPHIDType extends PhabricatorPHIDType {
     return pht('Image Macro');
   }
 
+  public function getPHIDTypeApplicationClass() {
+    return 'PhabricatorMacroApplication';
+  }
+
   public function getTypeIcon() {
     return 'fa-meh-o';
   }

From a39ec26a672addbdd86779f1798f2f1c7fbfcf4a Mon Sep 17 00:00:00 2001
From: Yongmin Hong <revi@pobox.com>
Date: Sat, 24 Oct 2015 18:12:34 -0700
Subject: [PATCH 07/39] Provide an application link for Ponder Answer PHID type

Summary: Ref T9625. I want this to be fixed ASAP hence here's the patch.

Test Plan:
 - ~~Apply D14323~~ (This patch was made before it was merged)
 - Apply this patch
 - voila! Now I see the Ponder answer has correct logo.

{F906357}

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, revi

Maniphest Tasks: T9625

Differential Revision: https://secure.phabricator.com/D14331
---
 src/applications/ponder/phid/PonderAnswerPHIDType.php | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/ponder/phid/PonderAnswerPHIDType.php b/src/applications/ponder/phid/PonderAnswerPHIDType.php
index 0bb0596737..be148af2f2 100644
--- a/src/applications/ponder/phid/PonderAnswerPHIDType.php
+++ b/src/applications/ponder/phid/PonderAnswerPHIDType.php
@@ -8,6 +8,10 @@ final class PonderAnswerPHIDType extends PhabricatorPHIDType {
     return pht('Ponder Answer');
   }
 
+  public function getPHIDTypeApplicationClass() {
+    return 'PhabricatorPonderApplication';
+  }
+
   public function newObject() {
     return new PonderAnswer();
   }

From 59c931710100b02f25076844801625cff2d1fd6c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 18:12:56 -0700
Subject: [PATCH 08/39] Prevent mailing lists from being bin/auth recover'd

Summary:
Fixes T9610.

  - We currently permit you to `bin/auth recover` users who can not establish web sessions (but this will never work). Prevent this.
  - We don't emit a tailored error if you follow one of these links. Tailor the error.

Even with the first fix, you can still hit the second case by doing something like:

  - Recover a normal user.
  - Make them a mailing list in the DB.
  - Follow the recovery link.

The original issue here was an install that did a large migration and set all users to be mailing lists. Normal installs should never encounter this, but it's not wholly unreasonable to have daemons or mailing lists with the administrator flag.

Test Plan:
  - Tried to follow a recovery link for a mailing list.
  - Tried to generate a recovery link for a mailing list.
  - Generated and followed a recovery link for a normal administrator.

{F906342}

```
epriestley@orbital ~/dev/phabricator $ ./bin/auth recover tortise-list
Usage Exception: This account ("tortise-list") can not establish web sessions, so it is not possible to generate a functional recovery link. Special accounts like daemons and mailing lists can not log in via the web UI.
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9610

Differential Revision: https://secure.phabricator.com/D14325
---
 .../PhabricatorAuthOneTimeLoginController.php    | 16 ++++++++++++++++
 .../PhabricatorAuthManagementRecoverWorkflow.php | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index 6dfa49d860..ccc66ffeb7 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -84,6 +84,22 @@ final class PhabricatorAuthOneTimeLoginController
         ->addCancelButton('/login/email/', pht('Send Another Email'));
     }
 
+    if (!$target_user->canEstablishWebSessions()) {
+      return $this->newDialog()
+        ->setTitle(pht('Unable to Establish Web Session'))
+        ->setShortTitle(pht('Login Failure'))
+        ->appendParagraph(
+          pht(
+            'You are trying to gain access to an account ("%s") that can not '.
+            'establish a web session.',
+            $target_user->getUsername()))
+        ->appendParagraph(
+          pht(
+            'Special users like daemons and mailing lists are not permitted '.
+            'to log in via the web. Log in as a normal user instead.'))
+        ->addCancelButton('/');
+    }
+
     if ($request->isFormPost()) {
       // If we have an email bound into this URI, verify email so that clicking
       // the link in the "Welcome" email is good enough, without requiring users
diff --git a/src/applications/auth/management/PhabricatorAuthManagementRecoverWorkflow.php b/src/applications/auth/management/PhabricatorAuthManagementRecoverWorkflow.php
index ad843d56ce..a31b108f08 100644
--- a/src/applications/auth/management/PhabricatorAuthManagementRecoverWorkflow.php
+++ b/src/applications/auth/management/PhabricatorAuthManagementRecoverWorkflow.php
@@ -71,6 +71,16 @@ final class PhabricatorAuthManagementRecoverWorkflow
           $can_recover));
     }
 
+    if (!$user->canEstablishWebSessions()) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'This account ("%s") can not establish web sessions, so it is '.
+          'not possible to generate a functional recovery link. Special '.
+          'accounts like daemons and mailing lists can not log in via the '.
+          'web UI.',
+          $username));
+    }
+
     $engine = new PhabricatorAuthSessionEngine();
     $onetime_uri = $engine->getOneTimeLoginURI(
       $user,

From 1a4e5931f86f49dffff6c244c5c3a8d87d1f5cb2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 24 Oct 2015 18:39:16 -0700
Subject: [PATCH 09/39] Remove push to IRC from "readme.md" too

Summary: Brings this in line with other support docs and pushes users toward the modern stuff.

Test Plan: Read documentation.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14329
---
 README.md | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/README.md b/README.md
index 165c9cbac8..c2375d4340 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,4 @@
-**Phabricator** is an open source collection of web applications which help software companies build better software.
+**Phabricator** is a collection of web applications which help software companies build better software.
 
 Phabricator includes applications for:
 
@@ -18,25 +18,13 @@ Phabricator is developed and maintained by [Phacility](http://phacility.com).
 
 ----------
 
-**BUG REPORTS**
+**SUPPORT RESOURCES**
 
-Please update your install to **HEAD** before filing bug reports. Follow our [bug reporting guide](https://secure.phabricator.com/book/phabcontrib/article/bug_reports/) for complete instructions.
+For resources on filing bugs, requesting features, reporting security issues, and getting other kinds of support, see [Support Resources](https://secure.phabricator.com/book/phabricator/article/support/).
 
-**FEATURE REQUESTS**
+**NO PULL REQUESTS!**
 
-We're big fans of feature requests that state core problems, not just 'add this'. We've compiled a short guide to effective upstream requests [here](https://secure.phabricator.com/book/phabcontrib/article/feature_requests/).
-
-**COMMUNITY CHAT**
-
-Please visit our [IRC Channel (#phabricator on FreeNode)](irc://chat.freenode.net/phabricator) to talk with other members of the Phabricator community. There might be someone there who can help you with setup issues or what image to choose for a macro.
-
-**SECURITY ISSUES**
-
-Phabricator participates in HackerOne and may pay out for various issues reported there. You can find out more information on our [HackerOne page](https://hackerone.com/phabricator).
-
-**PULL REQUESTS**
-
-We do not accept pull requests through GitHub. If you would like to contribute code, please read our [Contributor's Guide](https://secure.phabricator.com/book/phabcontrib/article/contributing_code/) for more information.
+We do not accept pull requests through GitHub. If you would like to contribute code, please read our [Contributor's Guide](https://secure.phabricator.com/book/phabcontrib/article/contributing_code/).
 
 **LICENSE**
 

From 2beeb2fab0e2187946b9ed12c2994be55ba8083f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 25 Oct 2015 14:51:50 -0700
Subject: [PATCH 10/39] Write more detailed documentation about Differential
 inlines

Summary: Ref T9628. The porting feature has been fairly stable for a while, so make some reasonable effort to document how it works and some of the tradeoffs it involves.

Test Plan: Generated and read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9628

Differential Revision: https://secure.phabricator.com/D14335
---
 src/docs/user/userguide/differential.diviner  |  29 +--
 .../userguide/differential_inlines.diviner    | 168 ++++++++++++++++++
 2 files changed, 185 insertions(+), 12 deletions(-)
 create mode 100644 src/docs/user/userguide/differential_inlines.diviner

diff --git a/src/docs/user/userguide/differential.diviner b/src/docs/user/userguide/differential.diviner
index edf992f720..414ad87a4f 100644
--- a/src/docs/user/userguide/differential.diviner
+++ b/src/docs/user/userguide/differential.diviner
@@ -50,19 +50,24 @@ just some intern who you don't trust) you can write a Herald rule to
 automatically CC you on any revisions which match rules (like content, author,
 files affected, etc.)
 
-= Differential Tips =
+Inline Comments
+===============
 
-  - You can leave inline comments by clicking the line numbers in the diff.
-  - You can leave a comment across multiple lines by dragging across the line
-    numbers.
-  - Inline comments are initially saved as drafts. They are not submitted until
-    you submit a comment at the bottom of the page.
-  - Press "?" to view keyboard shortcuts.
+You can leave inline comments by clicking the line number next to a line. For
+an in-depth look at inline comments, see
+@{article:Differential User Guide: Inline Comments}.
 
-= Next Steps =
 
-  - Read the FAQ at @{article:Differential User Guide: FAQ}; or
-  - learn about handling large changesets at
+Next Steps
+==========
+
+Continue by:
+
+  - diving into the details of inline comments in
+    @{article:Differential User Guide: Inline Comments}; or
+  - reading the FAQ at @{article:Differential User Guide: FAQ}; or
+  - learning about handling large changesets in
     @{article:Differential User Guide: Large Changes}; or
-  - learn about test plans at @{article:Differential User Guide: Test Plans}; or
-  - learn more about Herald at @{article:Herald User Guide}.
+  - learning about test plans in
+    @{article:Differential User Guide: Test Plans}; or
+  - learning more about Herald in @{article:Herald User Guide}.
diff --git a/src/docs/user/userguide/differential_inlines.diviner b/src/docs/user/userguide/differential_inlines.diviner
new file mode 100644
index 0000000000..80ac9891ed
--- /dev/null
+++ b/src/docs/user/userguide/differential_inlines.diviner
@@ -0,0 +1,168 @@
+@title Differential User Guide: Inline Comments
+@group userguide
+
+Guide to inline comments in Differential.
+
+Overview
+========
+
+Differential allows reviewers to leave feedback about changes to code inline,
+within the body of the diff itself. These comments are called "inline
+comments", and can be used to discuss specific parts of a change.
+
+(NOTE) Click the line number next to a line to leave an inline comment.
+
+To leave an inline comment, click the line number next to a line when reviewing
+a change in Differential. You can also leave a comment on a range of adjacent
+lines by clicking one line number and dragging to a nearby line number.
+
+(NOTE) Other users can't see your comments right away!
+
+When you make a comment, it is initially an **unsubmitted draft**, indicated by
+an "Unsubmitted" badge in the comment header. Other users can't see it yet.
+This behavior is different from the behavior of other software you may be
+familiar with.
+
+To publish your inline comments, scroll to the bottom of the page and submit
+the form there. All your unsubmitted inline comments will be published when you
+do, alongside an optional normal comment and optional action (like accepting
+the revision).
+
+Differential doesn't publish inlines initially because having a draft phase
+gives reviewers more time to revise and adjust the inlines, and make their
+feedback more cohesive, and contextualize their inlines with discussion in a
+normal comment. It also allows Differential to send fewer, more relevant emails
+and notifications.
+
+
+Porting / Ghost Comments
+========================
+
+When a revision is updated, we attempt to port inline comments forward and
+display them on the new diff. Ported comments have a pale, ghostly appearance
+and include a button which allows you to jump back to the original context
+where the comment appeared.
+
+Ported comments sometimes appear in unexpected locations. There are two major
+reasons for this:
+
+  - In the general case, it is not possible to always port comments to the same
+    lines humans would automatically.
+  - We are very aggressive about porting comments forward, even in the presence
+    of heavy changes. This helps prevent mistakes and makes it easier to verify
+    feedback has been addressed.
+
+You can disable this behavior in
+{nav Settings > Diff Preferences > Show Older Inlines} if you prefer comments
+stay anchored to their original diff.
+
+To understand why porting comments forward is difficult and can produce
+unexpected results, and why we choose to be aggressive about it, let's look at
+a case where the right behavior is ambiguous. Imagine this code is added as
+part of a change:
+
+```name=important.c
+111 ...
+112
+113 if (a() || b() || c()) {
+114   return;
+115 }
+116
+117 ...
+```
+
+Suppose a reviewer leaves this comment on line 113:
+
+> important.c:113 This is a serious security vulnerability!
+
+The author later updates the diff, and the code now looks like this, with some
+other changes elsewhere so the line numbers have also shifted:
+
+```name=important.c
+140 ...
+141
+142 if (a()) {
+143   return;
+144 }
+145
+146 if (b()) {
+147   return;
+148 }
+149
+150 ...
+```
+
+If we port the inline forward from the first change to the second change, where
+should it go? A human would probably do one of three things:
+
+  # Put it on line 142, with the call to `a()`.
+  # Put it on line 146, with the call to `b()`.
+  # Don't bring it forward at all.
+
+A human would choose between (1) and (2) based on context about what `a()` and
+`b()` are and what the reviewer meant. The algorithm can not possibly read the
+comment and understand which part of the statement it talked about. Humans
+might not even agree on which line is the better fit.
+
+When we choose one of these behaviors, humans will sometimes think the other
+behavior was the better one, because they have more information about what
+`a()` and `b()` are and what the comment actually meant. The line we choose may
+be unexpected, but it is the best the algorithm can do without being able to
+actually read the code or understand what the comment means.
+
+A human might also choose not to bring the comment forward if they knew that
+removing `c()` addressed it, but the algorithm can not know that. The call to
+`a()` or `b()` may be the dangerous thing the reviewer was talking about, and
+we err on the side of caution by bringing comments forward aggressively.
+
+When a line of code with an inline comment on it changes, we can not know if
+the change addressed the inline or not. We take the cautious route and
+//always// assume it did not, and that humans need to verify problems have
+really been addressed.
+
+This means that inlines are sometimes ported forward into places that don't
+make sense (the code has changed completely), but we won't drop important
+inlines just because the structure of the code has changed.
+
+Here's another case where bringing inlines forward seems desirable. Imagine
+this code is added as part of a change:
+
+```name=warpgate.c
+12 ...
+13 function_x();
+14 ...
+```
+
+Suppose a reviewer leaves this comment on line 13:
+
+> warpgate.c:13 This should be function_y() instead.
+
+The author later updates the diff, and the code now looks like this:
+
+```name=warpgate.c
+12 ...
+13 function_y();
+14 ...
+```
+
+For the reasons discussed above, we port the comment forward, so it will appear
+under line 13.
+
+We think this is desirable: it makes it trivial for an author or reviewer to
+look through the changes and verify that the feedback has really been
+addressed, because you can see that the code now uses the proper function.
+
+This isn't to say that we always do the best we can. There may be cases where
+the algorithm can be smarter than it currently is or otherwise produce a better
+result. If you find such a case, file a bug report. But it's expected that the
+algorithm will sometimes port comments into places that aren't optimal or no
+longer make sense, because this is frequently the best behavior available among
+the possible alternatives.
+
+
+Next Steps
+==========
+
+Continue by:
+
+  - returning to the @{article:Differential User Guide}.

From 43569d4e278c365a1feb9497acfa4ce336a8262c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 25 Oct 2015 14:53:24 -0700
Subject: [PATCH 11/39] Make WorkingCopy build step slightly more durable

Summary: Fixes T9631. Build steps created before I added this option may not have it specified, which could throw later. Make handling a little more robust.

Test Plan: Will ask @yelirekim to report back.

Reviewers: chad

Reviewed By: chad

Subscribers: yelirekim

Maniphest Tasks: T9631

Differential Revision: https://secure.phabricator.com/D14336
---
 .../HarbormasterLeaseWorkingCopyBuildStepImplementation.php    | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
index 1bf5b33a0c..3c4d18f4b4 100644
--- a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
+++ b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
@@ -135,6 +135,9 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
     }
 
     $also_phids = $build_target->getFieldValue('repositoryPHIDs');
+    if (!is_array($also_phids)) {
+      $also_phids = array();
+    }
 
     $all_phids = $also_phids;
     $all_phids[] = $repository_phid;

From 5ee4a1a306c5c003b95fca2ec712e288bb029711 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 12:38:21 -0700
Subject: [PATCH 12/39] Give Harbormaster Build Plans real policies

Summary:
Ref T9614. Currently, a lot of Build Plan behavior is covered by a global "can manage" policy.

One install in particular is experiencing difficulty with warring factions within engineering aborting one another's builds.

As a first step to remedy this, and also generally make Harbormaster more flexible and bring it in line with other applications in terms of policy power:

  - Give Build Plans normal view/edit policies.
  - Require "Can Edit" to run a plan manually.

Having "Can View" on plans may be a little weird in some cases (the status of a Buildable might be bad because of a build you can't see) but we can cross that bridge when we come to it.

Next change here will require "Can Edit" to abort a build. This will reasonably allow installs to reserve pause/abort for administrators/adults. (I might let anyone restart a plan, though?)

Test Plan:
  - Created a new build plan.
  - Verified defaults were inherited from application defaults (swapped them around, too).
  - Saved build plan.
  - Edited policies.
  - Verified autoplans get the right policies.
  - Verified old plans got migrated properly.
  - Tried to run a plan I couldn't edit (denied).
  - Ran a plan from CLI with `bin/harbormaster`.
  - Tried to create a plan with an unprivileged user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9614

Differential Revision: https://secure.phabricator.com/D14321
---
 .../autopatches/20151023.harborpolicy.1.sql   |  5 ++
 .../autopatches/20151023.harborpolicy.2.php   | 21 ++++++++
 src/__phutil_library_map__.php                |  8 ++-
 .../PhabricatorHarbormasterApplication.php    | 12 ++++-
 ...ormasterBuildPlanDefaultEditCapability.php | 12 +++++
 ...ormasterBuildPlanDefaultViewCapability.php | 16 ++++++
 ... => HarbormasterCreatePlansCapability.php} |  6 +--
 .../HarbormasterPlanDisableController.php     |  3 --
 .../HarbormasterPlanEditController.php        | 54 +++++++++++++++----
 .../HarbormasterPlanListController.php        |  2 +-
 .../HarbormasterPlanRunController.php         | 13 ++---
 .../HarbormasterPlanViewController.php        | 18 ++-----
 .../HarbormasterStepAddController.php         |  3 --
 .../HarbormasterStepDeleteController.php      |  3 --
 .../HarbormasterStepEditController.php        |  3 --
 .../editor/HarbormasterBuildPlanEditor.php    |  3 +-
 .../configuration/HarbormasterBuildPlan.php   | 27 +++++++---
 17 files changed, 149 insertions(+), 60 deletions(-)
 create mode 100644 resources/sql/autopatches/20151023.harborpolicy.1.sql
 create mode 100644 resources/sql/autopatches/20151023.harborpolicy.2.php
 create mode 100644 src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php
 create mode 100644 src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php
 rename src/applications/harbormaster/capability/{HarbormasterManagePlansCapability.php => HarbormasterCreatePlansCapability.php} (59%)

diff --git a/resources/sql/autopatches/20151023.harborpolicy.1.sql b/resources/sql/autopatches/20151023.harborpolicy.1.sql
new file mode 100644
index 0000000000..516cd1af00
--- /dev/null
+++ b/resources/sql/autopatches/20151023.harborpolicy.1.sql
@@ -0,0 +1,5 @@
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan
+  ADD viewPolicy VARBINARY(64) NOT NULL;
+
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan
+  ADD editPolicy VARBINARY(64) NOT NULL;
diff --git a/resources/sql/autopatches/20151023.harborpolicy.2.php b/resources/sql/autopatches/20151023.harborpolicy.2.php
new file mode 100644
index 0000000000..133bcb6ee7
--- /dev/null
+++ b/resources/sql/autopatches/20151023.harborpolicy.2.php
@@ -0,0 +1,21 @@
+<?php
+
+$table = new HarbormasterBuildPlan();
+$conn_w = $table->establishConnection('w');
+
+$view_policy = PhabricatorPolicies::getMostOpenPolicy();
+queryfx(
+  $conn_w,
+  'UPDATE %T SET viewPolicy = %s WHERE viewPolicy = %s',
+  $table->getTableName(),
+  $view_policy,
+  '');
+
+$edit_policy = id(new PhabricatorHarbormasterApplication())
+  ->getPolicy(HarbormasterCreatePlansCapability::CAPABILITY);
+queryfx(
+  $conn_w,
+  'UPDATE %T SET editPolicy = %s WHERE editPolicy = %s',
+  $table->getTableName(),
+  $edit_policy,
+  '');
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 41ae622d45..37b691e876 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -991,6 +991,8 @@ phutil_register_library_map(array(
     'HarbormasterBuildPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPHIDType.php',
     'HarbormasterBuildPlan' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php',
     'HarbormasterBuildPlanDatasource' => 'applications/harbormaster/typeahead/HarbormasterBuildPlanDatasource.php',
+    'HarbormasterBuildPlanDefaultEditCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php',
+    'HarbormasterBuildPlanDefaultViewCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php',
     'HarbormasterBuildPlanEditor' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditor.php',
     'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php',
     'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php',
@@ -1036,6 +1038,7 @@ phutil_register_library_map(array(
     'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php',
     'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php',
+    'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php',
     'HarbormasterDAO' => 'applications/harbormaster/storage/HarbormasterDAO.php',
     'HarbormasterDrydockBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterDrydockBuildStepGroup.php',
     'HarbormasterDrydockCommandBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterDrydockCommandBuildStepImplementation.php',
@@ -1049,7 +1052,6 @@ phutil_register_library_map(array(
     'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php',
     'HarbormasterLintMessagesController' => 'applications/harbormaster/controller/HarbormasterLintMessagesController.php',
     'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php',
-    'HarbormasterManagePlansCapability' => 'applications/harbormaster/capability/HarbormasterManagePlansCapability.php',
     'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php',
     'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php',
     'HarbormasterManagementWorkflow' => 'applications/harbormaster/management/HarbormasterManagementWorkflow.php',
@@ -4816,6 +4818,8 @@ phutil_register_library_map(array(
       'PhabricatorSubscribableInterface',
     ),
     'HarbormasterBuildPlanDatasource' => 'PhabricatorTypeaheadDatasource',
+    'HarbormasterBuildPlanDefaultEditCapability' => 'PhabricatorPolicyCapability',
+    'HarbormasterBuildPlanDefaultViewCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterBuildPlanEditor' => 'PhabricatorApplicationTransactionEditor',
     'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType',
     'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
@@ -4875,6 +4879,7 @@ phutil_register_library_map(array(
     'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod',
     'HarbormasterController' => 'PhabricatorController',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod',
+    'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterDAO' => 'PhabricatorLiskDAO',
     'HarbormasterDrydockBuildStepGroup' => 'HarbormasterBuildStepGroup',
     'HarbormasterDrydockCommandBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
@@ -4888,7 +4893,6 @@ phutil_register_library_map(array(
     'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterLintMessagesController' => 'HarbormasterController',
     'HarbormasterLintPropertyView' => 'AphrontView',
-    'HarbormasterManagePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementWorkflow' => 'PhabricatorManagementWorkflow',
diff --git a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
index 59b8ce6444..b2b7f3a931 100644
--- a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
+++ b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
@@ -95,8 +95,16 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication {
 
   protected function getCustomCapabilities() {
     return array(
-      HarbormasterManagePlansCapability::CAPABILITY => array(
-        'caption' => pht('Can create and manage build plans.'),
+      HarbormasterCreatePlansCapability::CAPABILITY => array(
+        'default' => PhabricatorPolicies::POLICY_ADMIN,
+      ),
+      HarbormasterBuildPlanDefaultViewCapability::CAPABILITY => array(
+        'template' => HarbormasterBuildPlanPHIDType::TYPECONST,
+        'capability' => PhabricatorPolicyCapability::CAN_VIEW,
+      ),
+      HarbormasterBuildPlanDefaultEditCapability::CAPABILITY => array(
+        'template' => HarbormasterBuildPlanPHIDType::TYPECONST,
+        'capability' => PhabricatorPolicyCapability::CAN_EDIT,
         'default' => PhabricatorPolicies::POLICY_ADMIN,
       ),
     );
diff --git a/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php b/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php
new file mode 100644
index 0000000000..fa9e10c956
--- /dev/null
+++ b/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php
@@ -0,0 +1,12 @@
+<?php
+
+final class HarbormasterBuildPlanDefaultEditCapability
+  extends PhabricatorPolicyCapability {
+
+  const CAPABILITY = 'harbormaster.plan.default.edit';
+
+  public function getCapabilityName() {
+    return pht('Default Build Plan Edit Policy');
+  }
+
+}
diff --git a/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php b/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php
new file mode 100644
index 0000000000..5d3d46caa0
--- /dev/null
+++ b/src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php
@@ -0,0 +1,16 @@
+<?php
+
+final class HarbormasterBuildPlanDefaultViewCapability
+  extends PhabricatorPolicyCapability {
+
+  const CAPABILITY = 'harbomaster.plan.default.view';
+
+  public function getCapabilityName() {
+    return pht('Default Build Plan View Policy');
+  }
+
+  public function shouldAllowPublicPolicySetting() {
+    return true;
+  }
+
+}
diff --git a/src/applications/harbormaster/capability/HarbormasterManagePlansCapability.php b/src/applications/harbormaster/capability/HarbormasterCreatePlansCapability.php
similarity index 59%
rename from src/applications/harbormaster/capability/HarbormasterManagePlansCapability.php
rename to src/applications/harbormaster/capability/HarbormasterCreatePlansCapability.php
index a3134d61bd..0553f1fcb0 100644
--- a/src/applications/harbormaster/capability/HarbormasterManagePlansCapability.php
+++ b/src/applications/harbormaster/capability/HarbormasterCreatePlansCapability.php
@@ -1,17 +1,17 @@
 <?php
 
-final class HarbormasterManagePlansCapability
+final class HarbormasterCreatePlansCapability
   extends PhabricatorPolicyCapability {
 
   const CAPABILITY = 'harbormaster.plans';
 
   public function getCapabilityName() {
-    return pht('Can Manage Build Plans');
+    return pht('Can Create Build Plans');
   }
 
   public function describeCapabilityRejection() {
     return pht(
-      'You do not have permission to manage Harbormaster build plans.');
+      'You do not have permission to create Harbormaster build plans.');
   }
 
 }
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php
index 28cb8340a6..ccf6b8986f 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanDisableController.php
@@ -6,9 +6,6 @@ final class HarbormasterPlanDisableController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($request->getURIData('id')))
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanEditController.php b/src/applications/harbormaster/controller/HarbormasterPlanEditController.php
index 98494d881d..a4f177e7dc 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanEditController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanEditController.php
@@ -5,9 +5,6 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $id = $request->getURIData('id');
     if ($id) {
       $plan = id(new HarbormasterBuildPlanQuery())
@@ -23,23 +20,42 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
         return new Aphront404Response();
       }
     } else {
+      $this->requireApplicationCapability(
+        HarbormasterCreatePlansCapability::CAPABILITY);
+
       $plan = HarbormasterBuildPlan::initializeNewBuildPlan($viewer);
     }
 
     $e_name = true;
     $v_name = $plan->getName();
+    $v_view = $plan->getViewPolicy();
+    $v_edit = $plan->getEditPolicy();
     $validation_exception = null;
     if ($request->isFormPost()) {
       $xactions = array();
 
       $v_name = $request->getStr('name');
+      $v_view = $request->getStr('viewPolicy');
+      $v_edit = $request->getStr('editPolicy');
+
       $e_name = null;
+
       $type_name = HarbormasterBuildPlanTransaction::TYPE_NAME;
+      $type_view = PhabricatorTransactions::TYPE_VIEW_POLICY;
+      $type_edit = PhabricatorTransactions::TYPE_EDIT_POLICY;
 
       $xactions[] = id(new HarbormasterBuildPlanTransaction())
         ->setTransactionType($type_name)
         ->setNewValue($v_name);
 
+      $xactions[] = id(new HarbormasterBuildPlanTransaction())
+        ->setTransactionType($type_view)
+        ->setNewValue($v_view);
+
+      $xactions[] = id(new HarbormasterBuildPlanTransaction())
+        ->setTransactionType($type_edit)
+        ->setNewValue($v_edit);
+
       $editor = id(new HarbormasterBuildPlanEditor())
         ->setActor($viewer)
         ->setContinueOnNoEffect(true)
@@ -71,19 +87,37 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
       $save_button = pht('Save Build Plan');
     }
 
+    $policies = id(new PhabricatorPolicyQuery())
+      ->setViewer($viewer)
+      ->setObject($plan)
+      ->execute();
+
     $form = id(new AphrontFormView())
       ->setUser($viewer)
-      ->appendChild(
+      ->appendControl(
         id(new AphrontFormTextControl())
           ->setLabel(pht('Plan Name'))
           ->setName('name')
           ->setError($e_name)
-          ->setValue($v_name));
-
-    $form->appendChild(
-      id(new AphrontFormSubmitControl())
-        ->setValue($save_button)
-        ->addCancelButton($cancel_uri));
+          ->setValue($v_name))
+      ->appendControl(
+        id(new AphrontFormPolicyControl())
+          ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
+          ->setPolicyObject($plan)
+          ->setPolicies($policies)
+          ->setValue($v_view)
+          ->setName('viewPolicy'))
+      ->appendControl(
+        id(new AphrontFormPolicyControl())
+          ->setCapability(PhabricatorPolicyCapability::CAN_EDIT)
+          ->setPolicyObject($plan)
+          ->setPolicies($policies)
+          ->setValue($v_edit)
+          ->setName('editPolicy'))
+      ->appendControl(
+        id(new AphrontFormSubmitControl())
+          ->setValue($save_button)
+          ->addCancelButton($cancel_uri));
 
     $box = id(new PHUIObjectBoxView())
       ->setHeaderText($title)
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanListController.php b/src/applications/harbormaster/controller/HarbormasterPlanListController.php
index e0f9ce07c7..a67debd533 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanListController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanListController.php
@@ -42,7 +42,7 @@ final class HarbormasterPlanListController extends HarbormasterPlanController {
     $crumbs = parent::buildApplicationCrumbs();
 
     $can_create = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
+      HarbormasterCreatePlansCapability::CAPABILITY);
 
     $crumbs->addAction(
       id(new PHUIListItemView())
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php
index be6d138cf7..6655fd6806 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php
@@ -4,19 +4,16 @@ final class HarbormasterPlanRunController extends HarbormasterController {
 
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
-
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan_id = $request->getURIData('id');
 
-    // NOTE: At least for now, this only requires the "Can Manage Plans"
-    // capability, not the "Can Edit" capability. Possibly it should have
-    // a more stringent requirement, though.
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($plan_id))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
       ->executeOne();
     if (!$plan) {
       return new Aphront404Response();
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
index 680594277d..ed1ecfd8ca 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
@@ -3,7 +3,7 @@
 final class HarbormasterPlanViewController extends HarbormasterPlanController {
 
   public function handleRequest(AphrontRequest $request) {
-    $viewer = $this->getviewer();
+    $viewer = $this->getViewer();
     $id = $request->getURIData('id');
 
     $plan = id(new HarbormasterBuildPlanQuery())
@@ -81,16 +81,11 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->execute();
     $steps = mpull($steps, null, 'getPHID');
 
-    $has_manage = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
-    $has_edit = PhabricatorPolicyFilter::hasCapability(
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       $plan,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $can_edit = ($has_manage && $has_edit);
-
     $step_list = id(new PHUIObjectItemListView())
       ->setUser($viewer)
       ->setNoDataString(
@@ -252,16 +247,11 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->setObject($plan)
       ->setObjectURI($this->getApplicationURI("plan/{$id}/"));
 
-    $has_manage = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
-    $has_edit = PhabricatorPolicyFilter::hasCapability(
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       $plan,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $can_edit = ($has_manage && $has_edit);
-
     $list->addAction(
       id(new PhabricatorActionView())
         ->setName(pht('Edit Plan'))
@@ -288,7 +278,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
           ->setIcon('fa-ban'));
     }
 
-    $can_run = ($has_manage && $plan->canRunManually());
+    $can_run = ($can_edit && $plan->canRunManually());
 
     $list->addAction(
       id(new PhabricatorActionView())
diff --git a/src/applications/harbormaster/controller/HarbormasterStepAddController.php b/src/applications/harbormaster/controller/HarbormasterStepAddController.php
index 21cdc3f12c..a5ec41b2e7 100644
--- a/src/applications/harbormaster/controller/HarbormasterStepAddController.php
+++ b/src/applications/harbormaster/controller/HarbormasterStepAddController.php
@@ -5,9 +5,6 @@ final class HarbormasterStepAddController extends HarbormasterController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($request->getURIData('id')))
diff --git a/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php
index 94afff9d3b..1aa8c24e4a 100644
--- a/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php
+++ b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php
@@ -5,9 +5,6 @@ final class HarbormasterStepDeleteController extends HarbormasterController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $id = $request->getURIData('id');
 
     $step = id(new HarbormasterBuildStepQuery())
diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php
index 9e99cc6c7f..64b87e0f9b 100644
--- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php
+++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php
@@ -6,9 +6,6 @@ final class HarbormasterStepEditController extends HarbormasterController {
     $viewer = $this->getViewer();
     $id = $request->getURIData('id');
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     if ($id) {
       $step = id(new HarbormasterBuildStepQuery())
         ->setViewer($viewer)
diff --git a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php
index e3b8a73ad6..3b45ff108c 100644
--- a/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php
+++ b/src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php
@@ -15,7 +15,8 @@ final class HarbormasterBuildPlanEditor
     $types = parent::getTransactionTypes();
     $types[] = HarbormasterBuildPlanTransaction::TYPE_NAME;
     $types[] = HarbormasterBuildPlanTransaction::TYPE_STATUS;
-    $types[] = PhabricatorTransactions::TYPE_COMMENT;
+    $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
+    $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
     return $types;
   }
 
diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php
index cf3d9ee7d7..11cb6260b7 100644
--- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php
+++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php
@@ -12,6 +12,8 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   protected $name;
   protected $planStatus;
   protected $planAutoKey;
+  protected $viewPolicy;
+  protected $editPolicy;
 
   const STATUS_ACTIVE   = 'active';
   const STATUS_DISABLED = 'disabled';
@@ -19,10 +21,22 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   private $buildSteps = self::ATTACHABLE;
 
   public static function initializeNewBuildPlan(PhabricatorUser $actor) {
+    $app = id(new PhabricatorApplicationQuery())
+      ->setViewer($actor)
+      ->withClasses(array('PhabricatorHarbormasterApplication'))
+      ->executeOne();
+
+    $view_policy = $app->getPolicy(
+      HarbormasterBuildPlanDefaultViewCapability::CAPABILITY);
+    $edit_policy = $app->getPolicy(
+      HarbormasterBuildPlanDefaultEditCapability::CAPABILITY);
+
     return id(new HarbormasterBuildPlan())
       ->setName('')
       ->setPlanStatus(self::STATUS_ACTIVE)
-      ->attachBuildSteps(array());
+      ->attachBuildSteps(array())
+      ->setViewPolicy($view_policy)
+      ->setEditPolicy($edit_policy);
   }
 
   protected function getConfiguration() {
@@ -156,16 +170,15 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   public function getPolicy($capability) {
     switch ($capability) {
       case PhabricatorPolicyCapability::CAN_VIEW:
-        return PhabricatorPolicies::getMostOpenPolicy();
+        if ($this->isAutoplan()) {
+          return PhabricatorPolicies::getMostOpenPolicy();
+        }
+        return $this->getViewPolicy();
       case PhabricatorPolicyCapability::CAN_EDIT:
-        // NOTE: In practice, this policy is always limited by the "Mangage
-        // Build Plans" policy.
-
         if ($this->isAutoplan()) {
           return PhabricatorPolicies::POLICY_NOONE;
         }
-
-        return PhabricatorPolicies::getMostOpenPolicy();
+        return $this->getEditPolicy();
     }
   }
 

From 3f193cb9e06c0e114a22afa0694803b422122a58 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 12:38:32 -0700
Subject: [PATCH 13/39] Give Harbormaster build steps a "View" page

Summary:
Fixes T9519. Right now, build steps go straight from the build to the edit screen.

This means that there's no way to see their edit history or review details without edit permission. In particular, this makes it a bit harder to catch the Drydock Blueprint authorization warnings from T9519.

  - Add a standard view screen.
  - Add a little warning callout to blueprint authorizations.

This also does a bit of a touchup on the weird dropshadow element from T9586. Maybe not totally design-approved now but it's less ugly, at least.

Test Plan:
{F906695}

{F906696}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9519

Differential Revision: https://secure.phabricator.com/D14330
---
 resources/celerity/map.php                    |   4 +-
 src/__phutil_library_map__.php                |   2 +
 .../drydock/storage/DrydockAuthorization.php  |   2 +-
 .../view/DrydockObjectAuthorizationView.php   |  19 +++
 .../PhabricatorHarbormasterApplication.php    |   1 +
 .../HarbormasterPlanViewController.php        |  29 +----
 .../HarbormasterStepEditController.php        |  27 ++--
 .../HarbormasterStepViewController.php        | 122 ++++++++++++++++++
 .../PhabricatorUSEnglishTranslation.php       |   5 +
 .../application/harbormaster/harbormaster.css |   9 +-
 10 files changed, 175 insertions(+), 45 deletions(-)
 create mode 100644 src/applications/harbormaster/controller/HarbormasterStepViewController.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 176bf6509c..930d8faf27 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -71,7 +71,7 @@ return array(
     'rsrc/css/application/feed/feed.css' => 'ecd4ec57',
     'rsrc/css/application/files/global-drag-and-drop.css' => '697324ad',
     'rsrc/css/application/flag/flag.css' => '5337623f',
-    'rsrc/css/application/harbormaster/harbormaster.css' => '49d64eb4',
+    'rsrc/css/application/harbormaster/harbormaster.css' => 'b0758ca5',
     'rsrc/css/application/herald/herald-test.css' => 'a52e323e',
     'rsrc/css/application/herald/herald.css' => '826075fa',
     'rsrc/css/application/maniphest/batch-editor.css' => 'b0f0b6d5',
@@ -528,7 +528,7 @@ return array(
     'font-lato' => '5ab1a46a',
     'font-roboto-slab' => 'f24a53cb',
     'global-drag-and-drop-css' => '697324ad',
-    'harbormaster-css' => '49d64eb4',
+    'harbormaster-css' => 'b0758ca5',
     'herald-css' => '826075fa',
     'herald-rule-editor' => '91a6031b',
     'herald-test-css' => 'a52e323e',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 37b691e876..de893e82e4 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1078,6 +1078,7 @@ phutil_register_library_map(array(
     'HarbormasterStepAddController' => 'applications/harbormaster/controller/HarbormasterStepAddController.php',
     'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php',
     'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php',
+    'HarbormasterStepViewController' => 'applications/harbormaster/controller/HarbormasterStepViewController.php',
     'HarbormasterTargetEngine' => 'applications/harbormaster/engine/HarbormasterTargetEngine.php',
     'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php',
     'HarbormasterTestBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterTestBuildStepGroup.php',
@@ -4919,6 +4920,7 @@ phutil_register_library_map(array(
     'HarbormasterStepAddController' => 'HarbormasterController',
     'HarbormasterStepDeleteController' => 'HarbormasterController',
     'HarbormasterStepEditController' => 'HarbormasterController',
+    'HarbormasterStepViewController' => 'HarbormasterController',
     'HarbormasterTargetEngine' => 'Phobject',
     'HarbormasterTargetWorker' => 'HarbormasterWorker',
     'HarbormasterTestBuildStepGroup' => 'HarbormasterBuildStepGroup',
diff --git a/src/applications/drydock/storage/DrydockAuthorization.php b/src/applications/drydock/storage/DrydockAuthorization.php
index 8872ef7f6f..ea1160b23b 100644
--- a/src/applications/drydock/storage/DrydockAuthorization.php
+++ b/src/applications/drydock/storage/DrydockAuthorization.php
@@ -66,7 +66,7 @@ final class DrydockAuthorization extends DrydockDAO
 
   public static function getBlueprintStateIcon($state) {
     $map = array(
-      self::BLUEPRINTAUTH_REQUESTED => 'fa-exclamation-circle indigo',
+      self::BLUEPRINTAUTH_REQUESTED => 'fa-exclamation-circle pink',
       self::BLUEPRINTAUTH_AUTHORIZED => 'fa-check-circle green',
       self::BLUEPRINTAUTH_DECLINED => 'fa-times red',
     );
diff --git a/src/applications/drydock/view/DrydockObjectAuthorizationView.php b/src/applications/drydock/view/DrydockObjectAuthorizationView.php
index 261829bfe5..25c1d582a3 100644
--- a/src/applications/drydock/view/DrydockObjectAuthorizationView.php
+++ b/src/applications/drydock/view/DrydockObjectAuthorizationView.php
@@ -47,6 +47,7 @@ final class DrydockObjectAuthorizationView extends AphrontView {
       $authorizations = array();
     }
 
+    $warnings = array();
     $items = array();
     foreach ($blueprint_phids as $phid) {
       $authorization = idx($authorizations, $phid);
@@ -65,10 +66,28 @@ final class DrydockObjectAuthorizationView extends AphrontView {
         null,
         DrydockAuthorization::getBlueprintStateName($state));
 
+      switch ($state) {
+        case DrydockAuthorization::BLUEPRINTAUTH_REQUESTED:
+        case DrydockAuthorization::BLUEPRINTAUTH_DECLINED:
+          $warnings[] = $authorization;
+          break;
+      }
+
       $items[] = $item;
     }
 
     $status = new PHUIStatusListView();
+
+    if ($warnings) {
+      $status->addItem(
+        id(new PHUIStatusItemView())
+          ->setIcon('fa-exclamation-triangle', 'pink')
+          ->setTarget(
+            pht(
+              'WARNING: There are %s unapproved authorization(s)!',
+              new PhutilNumber(count($warnings)))));
+    }
+
     foreach ($items as $item) {
       $status->addItem($item);
     }
diff --git a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
index b2b7f3a931..c9026e722f 100644
--- a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
+++ b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php
@@ -61,6 +61,7 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication {
           'add/(?:(?P<id>\d+)/)?' => 'HarbormasterStepAddController',
           'new/(?P<plan>\d+)/(?P<class>[^/]+)/'
             => 'HarbormasterStepEditController',
+          'view/(?P<id>\d+)/' => 'HarbormasterStepViewController',
           'edit/(?:(?P<id>\d+)/)?' => 'HarbormasterStepEditController',
           'delete/(?:(?P<id>\d+)/)?' => 'HarbormasterStepDeleteController',
         ),
diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
index ed1ecfd8ca..ef8f98bdb2 100644
--- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
+++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php
@@ -117,16 +117,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
           ->setStatusIcon('fa-warning red')
           ->addAttribute(pht(
             'This step has an invalid implementation (%s).',
-            $step->getClassName()))
-          ->addAction(
-            id(new PHUIListItemView())
-              ->setIcon('fa-times')
-              ->addSigil('harbormaster-build-step-delete')
-              ->setWorkflow(true)
-              ->setRenderNameAsTooltip(true)
-              ->setName(pht('Delete'))
-              ->setHref(
-                $this->getApplicationURI('step/delete/'.$step->getID().'/')));
+            $step->getClassName()));
         $step_list->addItem($item);
         continue;
       }
@@ -137,23 +128,9 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       $item->addAttribute($implementation->getDescription());
 
       $step_id = $step->getID();
-      $edit_uri = $this->getApplicationURI("step/edit/{$step_id}/");
-      $delete_uri = $this->getApplicationURI("step/delete/{$step_id}/");
 
-      if ($can_edit) {
-        $item->setHref($edit_uri);
-      }
-
-      $item
-        ->setHref($edit_uri)
-        ->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('fa-times')
-            ->addSigil('harbormaster-build-step-delete')
-            ->setWorkflow(true)
-            ->setDisabled(!$can_edit)
-            ->setHref(
-              $this->getApplicationURI('step/delete/'.$step->getID().'/')));
+      $view_uri = $this->getApplicationURI("step/view/{$step_id}/");
+      $item->setHref($view_uri);
 
       $depends = $step->getStepImplementation()->getDependencies($step);
       $inputs = $step->getStepImplementation()->getArtifactInputs();
diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php
index 64b87e0f9b..37bef5f411 100644
--- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php
+++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php
@@ -58,6 +58,12 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
     $plan_uri = $this->getApplicationURI('plan/'.$plan->getID().'/');
 
+    if ($is_new) {
+      $cancel_uri = $plan_uri;
+    } else {
+      $cancel_uri = $this->getApplicationURI('step/view/'.$step->getID().'/');
+    }
+
     $implementation = $step->getStepImplementation();
 
     $field_list = PhabricatorCustomField::getObjectFields(
@@ -119,7 +125,10 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
       try {
         $editor->applyTransactions($step, $xactions);
-        return id(new AphrontRedirectResponse())->setURI($plan_uri);
+
+        $step_uri = $this->getApplicationURI('step/view/'.$step->getID().'/');
+
+        return id(new AphrontRedirectResponse())->setURI($step_uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
         $validation_exception = $ex;
       }
@@ -162,31 +171,31 @@ final class HarbormasterStepEditController extends HarbormasterController {
           ->setError($e_description)
           ->setValue($v_description));
 
+    $crumbs = $this->buildApplicationCrumbs();
+    $id = $plan->getID();
+    $crumbs->addTextCrumb(pht('Plan %d', $id), $plan_uri);
+
     if ($is_new) {
       $submit = pht('Create Build Step');
       $header = pht('New Step: %s', $implementation->getName());
-      $crumb = pht('Add Step');
+      $crumbs->addTextCrumb(pht('Add Step'));
     } else {
       $submit = pht('Save Build Step');
       $header = pht('Edit Step: %s', $implementation->getName());
-      $crumb = pht('Edit Step');
+      $crumbs->addTextCrumb(pht('Step %d', $step->getID()), $cancel_uri);
+      $crumbs->addTextCrumb(pht('Edit Step'));
     }
 
     $form->appendChild(
       id(new AphrontFormSubmitControl())
         ->setValue($submit)
-        ->addCancelButton($plan_uri));
+        ->addCancelButton($cancel_uri));
 
     $box = id(new PHUIObjectBoxView())
       ->setHeaderText($header)
       ->setValidationException($validation_exception)
       ->setForm($form);
 
-    $crumbs = $this->buildApplicationCrumbs();
-    $id = $plan->getID();
-    $crumbs->addTextCrumb(pht('Plan %d', $id), $plan_uri);
-    $crumbs->addTextCrumb($crumb);
-
     $variables = $this->renderBuildVariablesTable();
 
     if ($is_new) {
diff --git a/src/applications/harbormaster/controller/HarbormasterStepViewController.php b/src/applications/harbormaster/controller/HarbormasterStepViewController.php
new file mode 100644
index 0000000000..1af7f2fe16
--- /dev/null
+++ b/src/applications/harbormaster/controller/HarbormasterStepViewController.php
@@ -0,0 +1,122 @@
+<?php
+
+final class HarbormasterStepViewController extends HarbormasterController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');
+
+    $step = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->executeOne();
+    if (!$step) {
+      return new Aphront404Response();
+    }
+    $plan = $step->getBuildPlan();
+
+    $plan_id = $plan->getID();
+    $plan_uri = $this->getApplicationURI("plan/{$plan_id}/");
+
+    $implementation = $step->getStepImplementation();
+
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $step,
+      PhabricatorCustomField::ROLE_VIEW);
+    $field_list
+      ->setViewer($viewer)
+      ->readFieldsFromStorage($step);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb(pht('Plan %d', $plan_id), $plan_uri);
+    $crumbs->addTextCrumb(pht('Step %d', $id));
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Build Step %d: %s', $id, $step->getName()));
+
+    $properties = $this->buildPropertyList($step, $field_list);
+    $actions = $this->buildActionList($step);
+    $properties->setActionList($actions);
+
+    $box->addPropertyList($properties);
+
+    $timeline = $this->buildTransactionTimeline(
+      $step,
+      new HarbormasterBuildStepTransactionQuery());
+    $timeline->setShouldTerminate(true);
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $box,
+        $timeline,
+      ),
+      array(
+        'title' => pht('Step %d', $id),
+      ));
+  }
+
+  private function buildPropertyList(
+    HarbormasterBuildStep $step,
+    PhabricatorCustomFieldList $field_list) {
+    $viewer = $this->getViewer();
+
+    $view = id(new PHUIPropertyListView())
+      ->setUser($viewer)
+      ->setObject($step);
+
+    $view->addProperty(
+      pht('Created'),
+      phabricator_datetime($step->getDateCreated(), $viewer));
+
+    $field_list->appendFieldsToPropertyList(
+      $step,
+      $viewer,
+      $view);
+
+    $view->invokeWillRenderEvent();
+
+    $description = $step->getDescription();
+    if (strlen($description)) {
+      $view->addSectionHeader(
+        pht('Description'),
+        PHUIPropertyListView::ICON_SUMMARY);
+      $view->addTextContent(
+        new PHUIRemarkupView($viewer, $description));
+    }
+
+    return $view;
+  }
+
+
+  private function buildActionList(HarbormasterBuildStep $step) {
+    $viewer = $this->getViewer();
+    $id = $step->getID();
+
+    $list = id(new PhabricatorActionListView())
+      ->setUser($viewer)
+      ->setObject($step);
+
+    $can_edit = true;
+
+    $list->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Edit Step'))
+        ->setHref($this->getApplicationURI("step/edit/{$id}/"))
+        ->setWorkflow(!$can_edit)
+        ->setDisabled(!$can_edit)
+        ->setIcon('fa-pencil'));
+
+    $list->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Delete Step'))
+        ->setHref($this->getApplicationURI("step/delete/{$id}/"))
+        ->setWorkflow(true)
+        ->setDisabled(!$can_edit)
+        ->setIcon('fa-times'));
+
+    return $list;
+  }
+
+
+}
diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
index a3c1833468..5d3dc359d0 100644
--- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
+++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
@@ -1420,6 +1420,11 @@ final class PhabricatorUSEnglishTranslation
         ),
       ),
 
+      'WARNING: There are %s unapproved authorization(s)!' => array(
+        'WARNING: There is an unapproved authorization!',
+        'WARNING: There are unapproved authorizations!',
+      ),
+
     );
   }
 
diff --git a/webroot/rsrc/css/application/harbormaster/harbormaster.css b/webroot/rsrc/css/application/harbormaster/harbormaster.css
index 9878d3f893..6158582ddd 100644
--- a/webroot/rsrc/css/application/harbormaster/harbormaster.css
+++ b/webroot/rsrc/css/application/harbormaster/harbormaster.css
@@ -3,13 +3,8 @@
  */
 
 .harbormaster-artifact-io {
-  margin: 0 0 0 8px;
-  padding: 4px 8px;
-  border-width: 1px 0 0 1px;
-  border-style: solid;
-  box-shadow: inset 2px 2px 1px rgba(0, 0, 0, 0.075);
-  background: {$lightbluebackground};
-  border-color: {$lightblueborder};
+  margin: 4px 0 4px 8px;
+  padding: 8px;
 }
 
 .harbormaster-artifact-summary-header {

From c059149eb98edb56cacc54ab3cc27b0b2acce8fd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 12:39:47 -0700
Subject: [PATCH 14/39] Remove Drydock host resource limits and give working
 copies simple limits

Summary:
Ref T9252. Right now, we have very strict limits on Drydock: one lease per host, and one working copy per working copy blueprint.

These are silly and getting in the way of using "Land Revision" more widely, since we need at least one working copy for each landable repository.

For now, just remove the host limit and put a simple limit on working copies. This might need to be fancier some day (e.g., limit working copies per-host) but it is generally reasonable for the use cases of today.

Also add a `--background` flag to make testing a little easier.

(Limits are also less important nowadays than they were in the past, because pools expand slowly now and we seem to have stamped out all the "runaway train" bugs where allocators go crazy and allocate a million things.)

Test Plan:
  - With a limit of 5, ran 10 concurrent builds and saw them finish after allocating 5 total resources.
  - Removed limit, raised taskmaster concurrency to 128, ran thousands of builds in blocks of 128 or 256.
    - Saw Drydock gradually expand the pool, allocating a few more working copies at first and a lot of working copies later.
    - Got ~256 builds in ~140 seconds, which isn't a breakneck pace or anything but isn't too bad.
    - This stuff seems to be mostly bottlenecked on `sbuild` throttling inbound SSH connections. I haven't tweaked it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14334
---
 ...anacServiceHostBlueprintImplementation.php |  15 +--
 .../DrydockBlueprintImplementation.php        | 109 +++++++++++++++++-
 ...dockWorkingCopyBlueprintImplementation.php |  32 ++---
 .../HarbormasterManagementBuildWorkflow.php   |  11 +-
 4 files changed, 135 insertions(+), 32 deletions(-)

diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php
index e62c7b7558..19e476c4ba 100644
--- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php
@@ -109,11 +109,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
     DrydockBlueprint $blueprint,
     DrydockResource $resource,
     DrydockLease $lease) {
-
-    if (!DrydockSlotLock::isLockFree($this->getLeaseSlotLock($resource))) {
-      return false;
-    }
-
     return true;
   }
 
@@ -124,7 +119,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
 
     $lease
       ->setActivateWhenAcquired(true)
-      ->needSlotLock($this->getLeaseSlotLock($resource))
       ->acquireOnResource($resource);
   }
 
@@ -146,11 +140,6 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
     return;
   }
 
-  private function getLeaseSlotLock(DrydockResource $resource) {
-    $resource_phid = $resource->getPHID();
-    return "almanac.host.lease({$resource_phid})";
-  }
-
   public function getType() {
     return 'host';
   }
@@ -188,7 +177,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
     }
   }
 
-  public function getFieldSpecifications() {
+  protected function getCustomFieldSpecifications() {
     return array(
       'almanacServicePHIDs' => array(
         'name' => pht('Almanac Services'),
@@ -207,7 +196,7 @@ final class DrydockAlmanacServiceHostBlueprintImplementation
         'credential.type' =>
           PassphraseSSHPrivateKeyTextCredentialType::CREDENTIAL_TYPE,
       ),
-    ) + parent::getFieldSpecifications();
+    );
   }
 
   private function loadServices(DrydockBlueprint $blueprint) {
diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
index 0c53b19fcf..01c2067280 100644
--- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php
@@ -16,6 +16,26 @@ abstract class DrydockBlueprintImplementation extends Phobject {
   abstract public function getDescription();
 
   public function getFieldSpecifications() {
+    $fields = array();
+
+    $fields += $this->getCustomFieldSpecifications();
+
+    if ($this->shouldUseConcurrentResourceLimit()) {
+      $fields += array(
+        'allocator.limit' => array(
+          'name' => pht('Limit'),
+          'caption' => pht(
+            'Maximum number of resources this blueprint can have active '.
+            'concurrently.'),
+          'type' => 'int',
+        ),
+      );
+    }
+
+    return $fields;
+  }
+
+  protected function getCustomFieldSpecifications() {
     return array();
   }
 
@@ -316,6 +336,85 @@ abstract class DrydockBlueprintImplementation extends Phobject {
   }
 
 
+  /**
+   * Does this implementation use concurrent resource limits?
+   *
+   * Implementations can override this method to opt into standard limit
+   * behavior, which provides a simple concurrent resource limit.
+   *
+   * @return bool True to use limits.
+   */
+  protected function shouldUseConcurrentResourceLimit() {
+    return false;
+  }
+
+
+  /**
+   * Get the effective concurrent resource limit for this blueprint.
+   *
+   * @param DrydockBlueprint Blueprint to get the limit for.
+   * @return int|null Limit, or `null` for no limit.
+   */
+  protected function getConcurrentResourceLimit(DrydockBlueprint $blueprint) {
+    if ($this->shouldUseConcurrentResourceLimit()) {
+      $limit = $blueprint->getFieldValue('allocator.limit');
+      $limit = (int)$limit;
+      if ($limit > 0) {
+        return $limit;
+      } else {
+        return null;
+      }
+    }
+
+    return null;
+  }
+
+
+  protected function getConcurrentResourceLimitSlotLock(
+    DrydockBlueprint $blueprint) {
+
+    $limit = $this->getConcurrentResourceLimit($blueprint);
+    if ($limit === null) {
+      return;
+    }
+
+    $blueprint_phid = $blueprint->getPHID();
+
+    // TODO: This logic shouldn't do anything awful, but is a little silly. It
+    // would be nice to unify the "huge limit" and "small limit" cases
+    // eventually but it's a little tricky.
+
+    // If the limit is huge, just pick a random slot. This is just stopping
+    // us from exploding if someone types a billion zillion into the box.
+    if ($limit > 1024) {
+      $slot = mt_rand(0, $limit - 1);
+      return "allocator({$blueprint_phid}).limit({$slot})";
+    }
+
+    // For reasonable limits, actually check for an available slot.
+    $locks = DrydockSlotLock::loadLocks($blueprint_phid);
+    $locks = mpull($locks, null, 'getLockKey');
+
+    $slots = range(0, $limit - 1);
+    shuffle($slots);
+
+    foreach ($slots as $slot) {
+      $slot_lock = "allocator({$blueprint_phid}).limit({$slot})";
+      if (empty($locks[$slot_lock])) {
+        return $slot_lock;
+      }
+    }
+
+    // If we found no free slot, just return whatever we checked last (which
+    // is just a random slot). There's a small chance we'll get lucky and the
+    // lock will be free by the time we try to take it, but usually we'll just
+    // fail to grab the lock, throw an appropriate lock exception, and get back
+    // on the right path to retry later.
+    return $slot_lock;
+  }
+
+
+
   /**
    * Apply standard limits on resource allocation rate.
    *
@@ -329,7 +428,7 @@ abstract class DrydockBlueprintImplementation extends Phobject {
     // configurable by the blueprint implementation.
 
     // Limit on total number of active resources.
-    $total_limit = 1;
+    $total_limit = $this->getConcurrentResourceLimit($blueprint);
 
     // Always allow at least this many allocations to be in flight at once.
     $min_allowed = 1;
@@ -358,9 +457,11 @@ abstract class DrydockBlueprintImplementation extends Phobject {
 
     // If we're at the limit on total active resources, limit additional
     // allocations.
-    $n_total = ($n_alloc + $n_active + $n_broken + $n_released);
-    if ($n_total >= $total_limit) {
-      return true;
+    if ($total_limit !== null) {
+      $n_total = ($n_alloc + $n_active + $n_broken + $n_released);
+      if ($n_total >= $total_limit) {
+        return true;
+      }
     }
 
     // If the number of in-flight allocations is fewer than the minimum number
diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
index 1d7776af14..f4b33adb8b 100644
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -125,15 +125,7 @@ final class DrydockWorkingCopyBlueprintImplementation
       ->setOwnerPHID($resource_phid)
       ->setAttribute('workingcopy.resourcePHID', $resource_phid)
       ->setAllowedBlueprintPHIDs($blueprint_phids);
-
-    $resource
-      ->setAttribute('host.leasePHID', $host_lease->getPHID())
-      ->save();
-
-    $host_lease->queueForActivation();
-
-    // TODO: Add some limits to the number of working copies we can have at
-    // once?
+    $resource->setAttribute('host.leasePHID', $host_lease->getPHID());
 
     $map = $lease->getAttribute('repositories.map');
     foreach ($map as $key => $value) {
@@ -143,10 +135,18 @@ final class DrydockWorkingCopyBlueprintImplementation
           'phid',
         ));
     }
+    $resource->setAttribute('repositories.map', $map);
 
-    return $resource
-      ->setAttribute('repositories.map', $map)
-      ->allocateResource();
+    $slot_lock = $this->getConcurrentResourceLimitSlotLock($blueprint);
+    if ($slot_lock !== null) {
+      $resource->needSlotLock($slot_lock);
+    }
+
+    $resource->allocateResource();
+
+    $host_lease->queueForActivation();
+
+    return $resource;
   }
 
   public function activateResource(
@@ -393,14 +393,18 @@ final class DrydockWorkingCopyBlueprintImplementation
     return $lease;
   }
 
-  public function getFieldSpecifications() {
+  protected function getCustomFieldSpecifications() {
     return array(
       'blueprintPHIDs' => array(
         'name' => pht('Use Blueprints'),
         'type' => 'blueprints',
         'required' => true,
       ),
-    ) + parent::getFieldSpecifications();
+    );
+  }
+
+  protected function shouldUseConcurrentResourceLimit() {
+    return true;
   }
 
 
diff --git a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php
index bc6a52018c..7110d98d6e 100644
--- a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php
+++ b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php
@@ -15,6 +15,12 @@ final class HarbormasterManagementBuildWorkflow
             'param'       => 'id',
             'help'        => pht('ID of build plan to run.'),
           ),
+          array(
+            'name' => 'background',
+            'help' => pht(
+              'Submit builds into the build queue normally instead of '.
+              'running them in the foreground.'),
+          ),
           array(
             'name'        => 'buildable',
             'wildcard'    => true,
@@ -88,7 +94,10 @@ final class HarbormasterManagementBuildWorkflow
       "\n    %s\n\n",
       PhabricatorEnv::getProductionURI('/B'.$buildable->getID()));
 
-    PhabricatorWorker::setRunAllTasksInProcess(true);
+    if (!$args->getArg('background')) {
+      PhabricatorWorker::setRunAllTasksInProcess(true);
+    }
+
     $buildable->applyPlan($plan, array());
 
     $console->writeOut("%s\n", pht('Done.'));

From 9c394937961758d77f911de2a5da58335b055108 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 12:40:16 -0700
Subject: [PATCH 15/39] Make WorkingCopyBlueprint responsible for performing
 merges

Summary:
Ref T182. Currently, the "RepositoryLand" operation is responsible for performing merges when landing a revision.

However, we'd like to be able to perform these merges in a larger set of cases in the future. For example:

  - After Releeph is revamped, when someone says "I want to merge bug fix X into stable branch Y", it would probably be nice to make that a Buildable and let tests run against it without requring that it actually be pushed anywhere.
  - Same deal if we want a merge-from-Diffusion or cherry-pick-from-Diffusion operation.
  - Similar deal if we want a "random web UI edits from Diffusion".

Move the merging part into WorkingCopy so more applications can share/use it in the future.

A big chunk of this is me making stuff up for now (the ol' undocumented dictionary full of arbitrary magic keys), but I anticipate formalizing it as we move along.

Test Plan: Pushed rGITTEST0d58eef3ce0fa5a10732d2efefc56aec126bc219 up from my local install via "Land Revision".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14337
---
 ...dockWorkingCopyBlueprintImplementation.php |  37 +++++-
 .../command/DrydockCommandInterface.php       |  28 +++-
 .../DrydockLandRepositoryOperation.php        | 121 ++++++++++--------
 .../DrydockRepositoryOperationType.php        |   4 +
 .../storage/DrydockRepositoryOperation.php    |   5 +
 ...DrydockRepositoryOperationUpdateWorker.php |   8 +-
 6 files changed, 138 insertions(+), 65 deletions(-)

diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
index f4b33adb8b..0eb735439a 100644
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -237,8 +237,7 @@ final class DrydockWorkingCopyBlueprintImplementation
       $cmd = array();
       $arg = array();
 
-      $cmd[] = 'cd %s';
-      $arg[] = "{$root}/repo/{$directory}/";
+      $interface->pushWorkingDirectory("{$root}/repo/{$directory}/");
 
       $cmd[] = 'git clean -d --force';
       $cmd[] = 'git fetch';
@@ -285,6 +284,15 @@ final class DrydockWorkingCopyBlueprintImplementation
       if (idx($spec, 'default')) {
         $default = $directory;
       }
+
+      $merges = idx($spec, 'merges');
+      if ($merges) {
+        foreach ($merges as $merge) {
+          $this->applyMerge($interface, $merge);
+        }
+      }
+
+      $interface->popWorkingDirectory();
     }
 
     if ($default === null) {
@@ -333,7 +341,7 @@ final class DrydockWorkingCopyBlueprintImplementation
         $command_interface = $host_lease->getInterface($type);
 
         $path = $lease->getAttribute('workingcopy.default');
-        $command_interface->setWorkingDirectory($path);
+        $command_interface->pushWorkingDirectory($path);
 
         return $command_interface;
     }
@@ -407,5 +415,28 @@ final class DrydockWorkingCopyBlueprintImplementation
     return true;
   }
 
+  private function applyMerge(
+    DrydockCommandInterface $interface,
+    array $merge) {
+
+    $src_uri = $merge['src.uri'];
+    $src_ref = $merge['src.ref'];
+
+    $interface->execx(
+      'git fetch --no-tags -- %s +%s:%s',
+      $src_uri,
+      $src_ref,
+      $src_ref);
+
+    try {
+      $interface->execx(
+        'git merge --no-stat --squash --ff-only -- %s',
+        $src_ref);
+    } catch (CommandException $ex) {
+      // TODO: Specifically note this as a merge conflict.
+      throw $ex;
+    }
+  }
+
 
 }
diff --git a/src/applications/drydock/interface/command/DrydockCommandInterface.php b/src/applications/drydock/interface/command/DrydockCommandInterface.php
index 8034e0f732..f6b8b132da 100644
--- a/src/applications/drydock/interface/command/DrydockCommandInterface.php
+++ b/src/applications/drydock/interface/command/DrydockCommandInterface.php
@@ -4,15 +4,27 @@ abstract class DrydockCommandInterface extends DrydockInterface {
 
   const INTERFACE_TYPE = 'command';
 
-  private $workingDirectory;
+  private $workingDirectoryStack = array();
 
-  public function setWorkingDirectory($working_directory) {
-    $this->workingDirectory = $working_directory;
+  public function pushWorkingDirectory($working_directory) {
+    $this->workingDirectoryStack[] = $working_directory;
     return $this;
   }
 
-  public function getWorkingDirectory() {
-    return $this->workingDirectory;
+  public function popWorkingDirectory() {
+    if (!$this->workingDirectoryStack) {
+      throw new Exception(
+        pht(
+          'Unable to pop working directory, directory stack is empty.'));
+    }
+    return array_pop($this->workingDirectoryStack);
+  }
+
+  public function peekWorkingDirectory() {
+    if ($this->workingDirectoryStack) {
+      return last($this->workingDirectoryStack);
+    }
+    return null;
   }
 
   final public function getInterfaceType() {
@@ -38,12 +50,14 @@ abstract class DrydockCommandInterface extends DrydockInterface {
   abstract public function getExecFuture($command);
 
   protected function applyWorkingDirectoryToArgv(array $argv) {
-    if ($this->getWorkingDirectory() !== null) {
+    $directory = $this->peekWorkingDirectory();
+
+    if ($directory !== null) {
       $cmd = $argv[0];
       $cmd = "(cd %s && {$cmd})";
       $argv = array_merge(
         array($cmd),
-        array($this->getWorkingDirectory()),
+        array($directory),
         array_slice($argv, 1));
     }
 
diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
index df888f3f6f..00b5c71181 100644
--- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
+++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
@@ -35,6 +35,28 @@ final class DrydockLandRepositoryOperation
     }
   }
 
+  public function getWorkingCopyMerges(DrydockRepositoryOperation $operation) {
+    $repository = $operation->getRepository();
+    $merges = array();
+
+    $object = $operation->getObject();
+    if ($object instanceof DifferentialRevision) {
+      $diff = $this->loadDiff($operation);
+      $merges[] = array(
+        'src.uri' => $repository->getStagingURI(),
+        'src.ref' => $diff->getStagingRef(),
+      );
+    } else {
+      throw new Exception(
+        pht(
+          'Invalid or unknown object ("%s") for land operation, expected '.
+          'Differential Revision.',
+          $operation->getObjectPHID()));
+    }
+
+    return $merges;
+  }
+
   public function applyOperation(
     DrydockRepositoryOperation $operation,
     DrydockInterface $interface) {
@@ -48,36 +70,7 @@ final class DrydockLandRepositoryOperation
     if ($object instanceof DifferentialRevision) {
       $revision = $object;
 
-      $diff_phid = $operation->getProperty('differential.diffPHID');
-
-      $diff = id(new DifferentialDiffQuery())
-        ->setViewer($viewer)
-        ->withPHIDs(array($diff_phid))
-        ->executeOne();
-      if (!$diff) {
-        throw new Exception(
-          pht(
-            'Unable to load diff "%s".',
-            $diff_phid));
-      }
-
-      $diff_revid = $diff->getRevisionID();
-      $revision_id = $revision->getID();
-      if ($diff_revid != $revision_id) {
-        throw new Exception(
-          pht(
-            'Diff ("%s") has wrong revision ID ("%s", expected "%s").',
-            $diff_phid,
-            $diff_revid,
-            $revision_id));
-      }
-
-      $cmd[] = 'git fetch --no-tags -- %s +%s:%s';
-      $arg[] = $repository->getStagingURI();
-      $arg[] = $diff->getStagingRef();
-      $arg[] = $diff->getStagingRef();
-
-      $merge_src = $diff->getStagingRef();
+      $diff = $this->loadDiff($operation);
 
       $dict = $diff->getDiffAuthorshipDict();
       $author_name = idx($dict, 'authorName');
@@ -104,7 +97,6 @@ final class DrydockLandRepositoryOperation
     switch ($type) {
       case 'branch':
         $push_dst = 'refs/heads/'.$name;
-        $merge_dst = 'refs/remotes/origin/'.$name;
         break;
       default:
         throw new Exception(
@@ -116,30 +108,24 @@ final class DrydockLandRepositoryOperation
 
     $committer_info = $this->getCommitterInfo($operation);
 
-    $cmd[] = 'git checkout %s';
-    $arg[] = $merge_dst;
+    // NOTE: We're doing this commit with "-F -" so we don't run into trouble
+    // with enormous commit messages which might otherwise exceed the maximum
+    // size of a command.
 
-    $cmd[] = 'git merge --no-stat --squash --ff-only -- %s';
-    $arg[] = $merge_src;
+    $future = $interface->getExecFuture(
+      'git -c user.name=%s -c user.email=%s commit --author %s -F - --',
+      $committer_info['name'],
+      $committer_info['email'],
+      "{$author_name} <{$author_email}>");
 
-    $cmd[] = 'git -c user.name=%s -c user.email=%s commit --author %s -m %s';
+    $future
+      ->write($commit_message)
+      ->resolvex();
 
-    $arg[] = $committer_info['name'];
-    $arg[] = $committer_info['email'];
-
-    $arg[] = "{$author_name} <{$author_email}>";
-    $arg[] = $commit_message;
-
-    $cmd[] = 'git push origin -- %s:%s';
-    $arg[] = 'HEAD';
-    $arg[] = $push_dst;
-
-    $cmd = implode(' && ', $cmd);
-    $argv = array_merge(array($cmd), $arg);
-
-    $result = call_user_func_array(
-      array($interface, 'execx'),
-      $argv);
+    $interface->execx(
+      'git push origin -- %s:%s',
+      'HEAD',
+      $push_dst);
   }
 
   private function getCommitterInfo(DrydockRepositoryOperation $operation) {
@@ -172,4 +158,35 @@ final class DrydockLandRepositoryOperation
     );
   }
 
+  private function loadDiff(DrydockRepositoryOperation $operation) {
+    $viewer = $this->getViewer();
+    $revision = $operation->getObject();
+
+    $diff_phid = $operation->getProperty('differential.diffPHID');
+
+    $diff = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($diff_phid))
+      ->executeOne();
+    if (!$diff) {
+      throw new Exception(
+        pht(
+          'Unable to load diff "%s".',
+          $diff_phid));
+    }
+
+    $diff_revid = $diff->getRevisionID();
+    $revision_id = $revision->getID();
+    if ($diff_revid != $revision_id) {
+      throw new Exception(
+        pht(
+          'Diff ("%s") has wrong revision ID ("%s", expected "%s").',
+          $diff_phid,
+          $diff_revid,
+          $revision_id));
+    }
+
+    return $diff;
+  }
+
 }
diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php
index 1ae6d6e4f8..7ec3689aff 100644
--- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php
+++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php
@@ -16,6 +16,10 @@ abstract class DrydockRepositoryOperationType extends Phobject {
     DrydockRepositoryOperation $operation,
     PhabricatorUser $viewer);
 
+  public function getWorkingCopyMerges(DrydockRepositoryOperation $operation) {
+    return array();
+  }
+
   final public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
     return $this;
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 2ee09f3227..6ee86bf5bb 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -158,6 +158,11 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return false;
   }
 
+  public function getWorkingCopyMerges() {
+    return $this->getImplementation()->getWorkingCopyMerges(
+      $this);
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
index 556119f6b3..e0361d3eed 100644
--- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
@@ -53,6 +53,9 @@ final class DrydockRepositoryOperationUpdateWorker
     // waiting for a lease we're holding.
 
     try {
+      $operation->getImplementation()
+        ->setViewer($viewer);
+
       $lease = $this->loadWorkingCopyLease($operation);
 
       $interface = $lease->getInterface(
@@ -61,9 +64,6 @@ final class DrydockRepositoryOperationUpdateWorker
       // No matter what happens here, destroy the lease away once we're done.
       $lease->releaseOnDestruction(true);
 
-      $operation->getImplementation()
-        ->setViewer($viewer);
-
       $operation->applyOperation($interface);
 
     } catch (PhabricatorWorkerYieldException $ex) {
@@ -166,6 +166,8 @@ final class DrydockRepositoryOperationUpdateWorker
             $target));
     }
 
+    $spec['merges'] = $operation->getWorkingCopyMerges();
+
     $map = array();
     $map[$repository->getCloneName()] = array(
       'phid' => $repository->getPHID(),

From a0fba642b382da2337f3c92371129fcb0290cecb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 19:58:37 +0000
Subject: [PATCH 16/39] Show the oldest non-failing revision land operation, or
 the newest failure

Summary:
Ref T182.

  - We just show the oldest operation right now, but we usually care about the oldest non-failure.
  - Only query for actual land operations when rendering the revision operations dialog (maybe eventually we'll show more stuff?).
  - For now, prevent multiple lands / repeated lands or queueing up lands while other lands are happening.

Test Plan: Landed a revision. Tried to land it more / again.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14338
---
 ...ifferentialRevisionOperationController.php | 44 ++++++++++++++++++-
 .../DifferentialRevisionViewController.php    | 15 ++++++-
 .../query/DrydockRepositoryOperationQuery.php | 13 ++++++
 .../storage/DrydockRepositoryOperation.php    |  4 ++
 4 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php
index 6ec5b8edec..85f2a0b1b2 100644
--- a/src/applications/differential/controller/DifferentialRevisionOperationController.php
+++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php
@@ -58,6 +58,48 @@ final class DifferentialRevisionOperationController
           $repository->getMonogram()));
     }
 
+    $op = new DrydockLandRepositoryOperation();
+
+    // Check for other operations. Eventually this should probably be more
+    // general (e.g., it's OK to land to multiple different branches
+    // simultaneously) but just put this in as a sanity check for now.
+    $other_operations = id(new DrydockRepositoryOperationQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($revision->getPHID()))
+      ->withOperationTypes(
+        array(
+          $op->getOperationConstant(),
+        ))
+      ->withOperationStates(
+        array(
+          DrydockRepositoryOperation::STATE_WAIT,
+          DrydockRepositoryOperation::STATE_WORK,
+          DrydockRepositoryOperation::STATE_DONE,
+        ))
+      ->execute();
+
+    if ($other_operations) {
+      $any_done = false;
+      foreach ($other_operations as $operation) {
+        if ($operation->isDone()) {
+          $any_done = true;
+          break;
+        }
+      }
+
+      if ($any_done) {
+        return $this->rejectOperation(
+          $revision,
+          pht('Already Complete'),
+          pht('This revision has already landed.'));
+      } else {
+        return $this->rejectOperation(
+          $revision,
+          pht('Already In Flight'),
+          pht('This revision is already landing.'));
+      }
+    }
+
     if ($request->isFormPost()) {
       // NOTE: The operation is locked to the current active diff, so if the
       // revision is updated before the operation applies nothing sneaky
@@ -65,8 +107,6 @@ final class DifferentialRevisionOperationController
 
       $diff = $revision->getActiveDiff();
 
-      $op = new DrydockLandRepositoryOperation();
-
       $operation = DrydockRepositoryOperation::initializeNewOperation($op)
         ->setAuthorPHID($viewer->getPHID())
         ->setObjectPHID($revision->getPHID())
diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index f6c90362df..f05b6ab97d 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1047,6 +1047,10 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $operations = id(new DrydockRepositoryOperationQuery())
       ->setViewer($viewer)
       ->withObjectPHIDs(array($revision->getPHID()))
+      ->withOperationTypes(
+        array(
+          DrydockLandRepositoryOperation::OPCONST,
+        ))
       ->withOperationStates(
         array(
           DrydockRepositoryOperation::STATE_WAIT,
@@ -1058,7 +1062,16 @@ final class DifferentialRevisionViewController extends DifferentialController {
       return null;
     }
 
-    $operation = head(msort($operations, 'getID'));
+    $state_fail = DrydockRepositoryOperation::STATE_FAIL;
+
+    // We're going to show the oldest operation which hasn't failed, or the
+    // most recent failure if they're all failures.
+    $operations = msort($operations, 'getID');
+    foreach ($operations as $operation) {
+      if ($operation->getOperationState() != $state_fail) {
+        break;
+      }
+    }
 
     $box_view = id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Active Operations'));
diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php
index b72e654603..38e1b4787b 100644
--- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php
+++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php
@@ -7,6 +7,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery {
   private $objectPHIDs;
   private $repositoryPHIDs;
   private $operationStates;
+  private $operationTypes;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -33,6 +34,11 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery {
     return $this;
   }
 
+  public function withOperationTypes(array $types) {
+    $this->operationTypes = $types;
+    return $this;
+  }
+
   public function newResultObject() {
     return new DrydockRepositoryOperation();
   }
@@ -139,6 +145,13 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery {
         $this->operationStates);
     }
 
+    if ($this->operationTypes !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'operationType IN (%Ls)',
+        $this->operationTypes);
+    }
+
     return $where;
   }
 
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 6ee86bf5bb..32e6ded9f3 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -158,6 +158,10 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return false;
   }
 
+  public function isDone() {
+    return ($this->getOperationState() === self::STATE_DONE);
+  }
+
   public function getWorkingCopyMerges() {
     return $this->getImplementation()->getWorkingCopyMerges(
       $this);

From 2326d5f8d090f5ca50c90d23d0e344869a7bee0a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 20:00:49 +0000
Subject: [PATCH 17/39] Show lease on Repository Operation detail view and
 awaken on failures

Summary:
Ref T182. Couple of minor improvements here:

  - Show the Drydock lease when viewing a Repository Operation detail screen. This just makes it easier to jump around between relevant objects.
  - When tasks are waiting for a lease, awaken them when it breaks or is released, not just when it is acquired. This makes the queue move forward faster when errors occur.

Test Plan:
  - Viewed a repository operation and saw a link to the lease.
  - Did a bad land (intentional merge problem) and got an error in about ~3 seconds instead of ~17.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14341
---
 ...rydockRepositoryOperationViewController.php |  9 +++++++++
 .../drydock/storage/DrydockLease.php           | 18 ++++++++++++++----
 .../storage/DrydockRepositoryOperation.php     |  9 +++++++++
 .../worker/DrydockLeaseUpdateWorker.php        | 18 +++++++++++-------
 .../DrydockRepositoryOperationUpdateWorker.php |  2 +-
 5 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
index e740073dd9..9a509a5c04 100644
--- a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
+++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
@@ -83,6 +83,15 @@ final class DrydockRepositoryOperationViewController
       pht('Object'),
       $viewer->renderHandle($operation->getObjectPHID()));
 
+    $lease_phid = $operation->getWorkingCopyLeasePHID();
+    if ($lease_phid) {
+      $lease_display = $viewer->renderHandle($lease_phid);
+    } else {
+      $lease_display = phutil_tag('em', array(), pht('None'));
+    }
+
+    $view->addProperty(pht('Working Copy'), $lease_display);
+
     return $view;
   }
 
diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php
index af475c5d61..0dd4e36b48 100644
--- a/src/applications/drydock/storage/DrydockLease.php
+++ b/src/applications/drydock/storage/DrydockLease.php
@@ -429,10 +429,7 @@ final class DrydockLease extends DrydockDAO
       $this->scheduleUpdate($expires);
     }
 
-    $awaken_ids = $this->getAttribute('internal.awakenTaskIDs');
-    if (is_array($awaken_ids) && $awaken_ids) {
-      PhabricatorWorker::awakenTaskIDs($awaken_ids);
-    }
+    $this->awakenTasks();
   }
 
   public function logEvent($type, array $data = array()) {
@@ -454,6 +451,19 @@ final class DrydockLease extends DrydockDAO
     return $log->save();
   }
 
+  /**
+   * Awaken yielded tasks after a state change.
+   *
+   * @return this
+   */
+  public function awakenTasks() {
+    $awaken_ids = $this->getAttribute('internal.awakenTaskIDs');
+    if (is_array($awaken_ids) && $awaken_ids) {
+      PhabricatorWorker::awakenTaskIDs($awaken_ids);
+    }
+
+    return $this;
+  }
 
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 32e6ded9f3..6d7151b25d 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -167,6 +167,15 @@ final class DrydockRepositoryOperation extends DrydockDAO
       $this);
   }
 
+  public function setWorkingCopyLeasePHID($lease_phid) {
+    return $this->setProperty('exec.leasePHID', $lease_phid);
+  }
+
+  public function getWorkingCopyLeasePHID() {
+    return $this->getProperty('exec.leasePHID');
+  }
+
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
index 7d8cc98219..0c93647b89 100644
--- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
@@ -751,6 +751,15 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker {
       ->setStatus(DrydockLeaseStatus::STATUS_BROKEN)
       ->save();
 
+    $lease->logEvent(
+      DrydockLeaseActivationFailureLogType::LOGCONST,
+      array(
+        'class' => get_class($ex),
+        'message' => $ex->getMessage(),
+      ));
+
+    $lease->awakenTasks();
+
     $this->queueTask(
       __CLASS__,
       array(
@@ -760,13 +769,6 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker {
         'objectPHID' => $lease->getPHID(),
       ));
 
-    $lease->logEvent(
-      DrydockLeaseActivationFailureLogType::LOGCONST,
-      array(
-        'class' => get_class($ex),
-        'message' => $ex->getMessage(),
-      ));
-
     throw new PhabricatorWorkerPermanentFailureException(
       pht(
         'Permanent failure while activating lease ("%s"): %s',
@@ -796,6 +798,8 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker {
       ->save();
 
     $lease->logEvent(DrydockLeaseDestroyedLogType::LOGCONST);
+
+    $lease->awakenTasks();
   }
 
 }
diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
index e0361d3eed..a0dc1e5ebd 100644
--- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
@@ -127,7 +127,7 @@ final class DrydockRepositoryOperationUpdateWorker
       }
 
       $operation
-        ->setProperty('exec.leasePHID', $lease->getPHID())
+        ->setWorkingCopyLeasePHID($lease->getPHID())
         ->save();
 
       $lease->queueForActivation();

From 0b24a6e2006ee7f8e0dc95b32dc8e7b070386a94 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 20:11:21 +0000
Subject: [PATCH 18/39] Make "Land Revision" show merge conflicts more clearly

Summary:
Ref T182. We just show "an error happened" right now. Improve this behavior.

This error handling chain is a bit ad-hoc for now but we can formalize it as we hit other cases.

Test Plan:
{F910247}

{F910248}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14343
---
 ...dockWorkingCopyBlueprintImplementation.php | 46 ++++++++-
 .../storage/DrydockRepositoryOperation.php    |  8 ++
 .../DrydockRepositoryOperationStatusView.php  | 96 ++++++++++++++++++-
 ...DrydockRepositoryOperationUpdateWorker.php | 13 ++-
 4 files changed, 153 insertions(+), 10 deletions(-)

diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
index 0eb735439a..ca4ffc9088 100644
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -3,6 +3,8 @@
 final class DrydockWorkingCopyBlueprintImplementation
   extends DrydockBlueprintImplementation {
 
+  const PHASE_SQUASHMERGE = 'squashmerge';
+
   public function isEnabled() {
     return true;
   }
@@ -288,7 +290,7 @@ final class DrydockWorkingCopyBlueprintImplementation
       $merges = idx($spec, 'merges');
       if ($merges) {
         foreach ($merges as $merge) {
-          $this->applyMerge($interface, $merge);
+          $this->applyMerge($lease, $interface, $merge);
         }
       }
 
@@ -416,6 +418,7 @@ final class DrydockWorkingCopyBlueprintImplementation
   }
 
   private function applyMerge(
+    DrydockLease $lease,
     DrydockCommandInterface $interface,
     array $merge) {
 
@@ -428,15 +431,48 @@ final class DrydockWorkingCopyBlueprintImplementation
       $src_ref,
       $src_ref);
 
+    $command = csprintf(
+      'git merge --no-stat --squash --ff-only -- %R',
+      $src_ref);
+
     try {
-      $interface->execx(
-        'git merge --no-stat --squash --ff-only -- %s',
-        $src_ref);
+      $interface->execx('%C', $command);
     } catch (CommandException $ex) {
-      // TODO: Specifically note this as a merge conflict.
+      $this->setWorkingCopyVCSErrorFromCommandException(
+        $lease,
+        self::PHASE_SQUASHMERGE,
+        $command,
+        $ex);
+
       throw $ex;
     }
   }
 
+  protected function setWorkingCopyVCSErrorFromCommandException(
+    DrydockLease $lease,
+    $phase,
+    $command,
+    CommandException $ex) {
+
+    $error = array(
+      'phase' => $phase,
+      'command' => (string)$command,
+      'raw' => (string)$ex->getCommand(),
+      'err' => $ex->getError(),
+      'stdout' => $ex->getStdout(),
+      'stderr' => $ex->getStderr(),
+    );
+
+    $lease->setAttribute('workingcopy.vcs.error', $error);
+  }
+
+  public function getWorkingCopyVCSError(DrydockLease $lease) {
+    $error = $lease->getAttribute('workingcopy.vcs.error');
+    if (!$error) {
+      return null;
+    } else {
+      return $error;
+    }
+  }
 
 }
diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index 6d7151b25d..c6fc383193 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -175,6 +175,14 @@ final class DrydockRepositoryOperation extends DrydockDAO
     return $this->getProperty('exec.leasePHID');
   }
 
+  public function setWorkingCopyVCSError(array $error) {
+    return $this->setProperty('exec.workingcopy.error', $error);
+  }
+
+  public function getWorkingCopyVCSError() {
+    return $this->getProperty('exec.workingcopy.error');
+  }
+
 
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
diff --git a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
index 3bdb529c20..2638926816 100644
--- a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
+++ b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
@@ -73,12 +73,104 @@ final class DrydockRepositoryOperationStatusView
     if ($state != DrydockRepositoryOperation::STATE_FAIL) {
       $item->addAttribute($operation->getOperationCurrentStatus($viewer));
     } else {
-      // TODO: Make this more useful.
-      $item->addAttribute(pht('Operation encountered an error.'));
+      $vcs_error = $operation->getWorkingCopyVCSError();
+      if ($vcs_error) {
+        switch ($vcs_error['phase']) {
+          case DrydockWorkingCopyBlueprintImplementation::PHASE_SQUASHMERGE:
+            $message = pht(
+              'This change did not merge cleanly. This usually indicates '.
+              'that the change is out of date and needs to be updated.');
+            break;
+          default:
+            $message = pht(
+              'Operation encountered an error while performing repository '.
+              'operations.');
+            break;
+        }
+
+        $item->addAttribute($message);
+
+        $table = $this->renderVCSErrorTable($vcs_error);
+        list($links, $info) = $this->renderDetailToggles($table);
+
+        $item->addAttribute($links);
+        $item->appendChild($info);
+      } else {
+        $item->addAttribute(pht('Operation encountered an error.'));
+      }
     }
 
     return id(new PHUIObjectItemListView())
       ->addItem($item);
   }
 
+  private function renderVCSErrorTable(array $vcs_error) {
+    $rows = array();
+    $rows[] = array(pht('Command'), $vcs_error['command']);
+    $rows[] = array(pht('Error'), $vcs_error['err']);
+    $rows[] = array(pht('Stdout'), $vcs_error['stdout']);
+    $rows[] = array(pht('Stderr'), $vcs_error['stderr']);
+
+    $table = id(new AphrontTableView($rows))
+      ->setColumnClasses(
+        array(
+          'header',
+          'wide',
+        ));
+
+    return $table;
+  }
+
+  private function renderDetailToggles(AphrontTableView $table) {
+    $show_id = celerity_generate_unique_node_id();
+    $hide_id = celerity_generate_unique_node_id();
+    $info_id = celerity_generate_unique_node_id();
+
+    Javelin::initBehavior('phabricator-reveal-content');
+
+    $show_details = javelin_tag(
+      'a',
+      array(
+        'id' => $show_id,
+        'href' => '#',
+        'sigil' => 'reveal-content',
+        'mustcapture' => true,
+        'meta' => array(
+          'hideIDs' => array($show_id),
+          'showIDs' => array($hide_id, $info_id),
+        ),
+      ),
+      pht('Show Details'));
+
+    $hide_details = javelin_tag(
+      'a',
+      array(
+        'id' => $hide_id,
+        'href' => '#',
+        'sigil' => 'reveal-content',
+        'mustcapture' => true,
+        'style' => 'display: none',
+        'meta' => array(
+          'hideIDs' => array($hide_id, $info_id),
+          'showIDs' => array($show_id),
+        ),
+      ),
+      pht('Hide Details'));
+
+    $info = javelin_tag(
+      'div',
+      array(
+        'id' => $info_id,
+        'style' => 'display: none',
+      ),
+      $table);
+
+    $links = array(
+      $show_details,
+      $hide_details,
+    );
+
+    return array($links, $info);
+  }
+
 }
diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
index a0dc1e5ebd..f6e8b55e74 100644
--- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
@@ -90,6 +90,9 @@ final class DrydockRepositoryOperationUpdateWorker
     // TODO: This is very similar to leasing in Harbormaster, maybe we can
     // share some of the logic?
 
+    $working_copy = new DrydockWorkingCopyBlueprintImplementation();
+    $working_copy_type = $working_copy->getType();
+
     $lease_phid = $operation->getProperty('exec.leasePHID');
     if ($lease_phid) {
       $lease = id(new DrydockLeaseQuery())
@@ -103,9 +106,6 @@ final class DrydockRepositoryOperationUpdateWorker
             $lease_phid));
       }
     } else {
-      $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation())
-        ->getType();
-
       $repository = $operation->getRepository();
 
       $allowed_phids = $repository->getAutomationBlueprintPHIDs();
@@ -138,6 +138,13 @@ final class DrydockRepositoryOperationUpdateWorker
     }
 
     if (!$lease->isActive()) {
+      $vcs_error = $working_copy->getWorkingCopyVCSError($lease);
+      if ($vcs_error) {
+        $operation
+          ->setWorkingCopyVCSError($vcs_error)
+          ->save();
+      }
+
       throw new PhabricatorWorkerPermanentFailureException(
         pht(
           'Lease "%s" never activated.',

From 5a35dd233b19a04d695678ac740901ffe3170b40 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 20:26:56 +0000
Subject: [PATCH 19/39] Don't use --ff-only inside "Land Revision"

Summary:
Ref T182. I lifted this logic out of `arc`, but the context is a little different there, and this option is too strict in "Land Revision".

Specifically, it prevents `git` from merging unless the merge is //strictly// a fast-foward, even with `--squash`. That means revisions can't merge unless they're rebased on the current `master`, even if they have no conflicts.

(This whole process will probably need additional refinement, but the behavior without this flag is more reasonable overall than the behavior with it for now.)

Test Plan: Will land stuff in production~~

Reviewers: chad, Mnkras

Reviewed By: Mnkras

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14346
---
 .../blueprint/DrydockWorkingCopyBlueprintImplementation.php     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
index ca4ffc9088..6694e0d72a 100644
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -432,7 +432,7 @@ final class DrydockWorkingCopyBlueprintImplementation
       $src_ref);
 
     $command = csprintf(
-      'git merge --no-stat --squash --ff-only -- %R',
+      'git merge --no-stat --squash -- %R',
       $src_ref);
 
     try {

From bbf4ce79e34131e17691e551cbc7f3480fb7a802 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 21:12:16 +0000
Subject: [PATCH 20/39] Provide a username and email when running `git merge
 --squash`

Summary:
Ref T182. This command should never actually generate a commit because `--squash` prevents that, but `git` seems to sometimes hit a check for username/email configuration (maybe when merging a non-fastforward?).

Give it some dummy values to placate it. This command shouldn't commit anything so these values should never actually be used.

Test Plan: Landed rGITTESTd8c8643cb02bbe60048c6c206afc2940c760a77e.

Reviewers: chad, Mnkras

Reviewed By: Mnkras

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14347
---
 ...dockWorkingCopyBlueprintImplementation.php | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
index 6694e0d72a..f39353a8f4 100644
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php
@@ -431,17 +431,28 @@ final class DrydockWorkingCopyBlueprintImplementation
       $src_ref,
       $src_ref);
 
-    $command = csprintf(
-      'git merge --no-stat --squash -- %R',
+    // NOTE: This can never actually generate a commit because we pass
+    // "--squash", but git sometimes runs code to check that a username and
+    // email are configured anyway.
+    $real_command = csprintf(
+      'git -c user.name=%s -c user.email=%s merge --no-stat --squash -- %R',
+      'drydock',
+      'drydock@phabricator',
+      $src_ref);
+
+    // Show the user a simplified command if the operation fails and we need to
+    // report an error.
+    $show_command = csprintf(
+      'git merge --squash -- %R',
       $src_ref);
 
     try {
-      $interface->execx('%C', $command);
+      $interface->execx('%C', $real_command);
     } catch (CommandException $ex) {
       $this->setWorkingCopyVCSErrorFromCommandException(
         $lease,
         self::PHASE_SQUASHMERGE,
-        $command,
+        $show_command,
         $ex);
 
       throw $ex;

From 6e7ceb996bb34e5fbd2644e1921485fbe546ce82 Mon Sep 17 00:00:00 2001
From: Michael Krasnow <mnkras@gmail.com>
Date: Mon, 26 Oct 2015 21:15:45 +0000
Subject: [PATCH 21/39] Set a property so that unit tests run on PHP7

Summary: Without this change PHP throws because idx() is passed null as the property is not intialzied

Test Plan: arc unit --everything

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D14345
---
 src/aphront/AphrontRequest.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 026129404e..6f0518a3fb 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -28,7 +28,7 @@ final class AphrontRequest extends Phobject {
   private $applicationConfiguration;
   private $site;
   private $controller;
-  private $uriData;
+  private $uriData = array();
   private $cookiePrefix;
 
   public function __construct($host, $path) {

From cea633f698daad27d5e2b781124198d9f0767878 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 26 Oct 2015 21:27:04 +0000
Subject: [PATCH 22/39] Don't show error operations after a successful land
 operation

Summary:
Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.

Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).

Also make the table a little nicer, particularly for merge failure output.

Test Plan: {F910385}

Reviewers: chad, Mnkras

Reviewed By: Mnkras

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14348
---
 resources/celerity/map.php                           |  6 +++---
 .../DifferentialRevisionViewController.php           | 12 ++++++------
 .../view/DrydockRepositoryOperationStatusView.php    |  2 +-
 webroot/rsrc/css/aphront/table-view.css              |  1 +
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 930d8faf27..ee959fa58a 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => 'c65b251d',
+    'core.pkg.css' => 'd80f5c3e',
     'core.pkg.js' => '47dc9ebb',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '2de124c9',
@@ -25,7 +25,7 @@ return array(
     'rsrc/css/aphront/notification.css' => '9c279160',
     'rsrc/css/aphront/panel-view.css' => '8427b78d',
     'rsrc/css/aphront/phabricator-nav-view.css' => 'a24cb589',
-    'rsrc/css/aphront/table-view.css' => '63985f5b',
+    'rsrc/css/aphront/table-view.css' => '61543e7a',
     'rsrc/css/aphront/tokenizer.css' => '04875312',
     'rsrc/css/aphront/tooltip.css' => '7672b60f',
     'rsrc/css/aphront/typeahead-browse.css' => 'd8581d2c',
@@ -493,7 +493,7 @@ return array(
     'aphront-list-filter-view-css' => '5d6f0526',
     'aphront-multi-column-view-css' => 'fd18389d',
     'aphront-panel-view-css' => '8427b78d',
-    'aphront-table-view-css' => '63985f5b',
+    'aphront-table-view-css' => '61543e7a',
     'aphront-tokenizer-control-css' => '04875312',
     'aphront-tooltip-css' => '7672b60f',
     'aphront-typeahead-control-css' => '0e403212',
diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index f05b6ab97d..621464cc9b 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1051,12 +1051,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
         array(
           DrydockLandRepositoryOperation::OPCONST,
         ))
-      ->withOperationStates(
-        array(
-          DrydockRepositoryOperation::STATE_WAIT,
-          DrydockRepositoryOperation::STATE_WORK,
-          DrydockRepositoryOperation::STATE_FAIL,
-        ))
       ->execute();
     if (!$operations) {
       return null;
@@ -1073,6 +1067,12 @@ final class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
+    // If we found a completed operation, don't render anything. We don't want
+    // to show an older error after the thing worked properly.
+    if ($operation->isDone()) {
+      return null;
+    }
+
     $box_view = id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Active Operations'));
 
diff --git a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
index 2638926816..bf1040afe6 100644
--- a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
+++ b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php
@@ -115,7 +115,7 @@ final class DrydockRepositoryOperationStatusView
       ->setColumnClasses(
         array(
           'header',
-          'wide',
+          'wide prewrap',
         ));
 
     return $table;
diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css
index e0e06317c8..254599625f 100644
--- a/webroot/rsrc/css/aphront/table-view.css
+++ b/webroot/rsrc/css/aphront/table-view.css
@@ -61,6 +61,7 @@ th.aphront-table-view-sortable-selected {
   text-align: right;
   color: {$bluetext};
   font-weight: bold;
+  vertical-align: top;
 }
 
 .aphront-table-view td {

From a763f9510e76666ed7c8fff1e7fdcb64ea1be0d5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 27 Oct 2015 18:04:02 +0000
Subject: [PATCH 23/39] Add some Drydock documentation plus "Test
 Configuration" for repository automation

Summary:
Ref T182. Ref T9252.

  - Adds a "Test" repository operation that just runs `git status` to see if things work.
  - Adds a button for it in Edit Repository.
  - Shows operation status on the operation detail view to make this workflow work a little better.
  - Adds a lot of words. Words words words words.

Test Plan:
  - Tested repository operation.
  - Read words.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182, T9252

Differential Revision: https://secure.phabricator.com/D14349
---
 src/__phutil_library_map__.php                |   4 +
 .../PhabricatorDiffusionApplication.php       |   1 +
 ...sionRepositoryEditAutomationController.php |   2 +-
 .../DiffusionRepositoryEditMainController.php |  13 ++
 ...sionRepositoryTestAutomationController.php |  78 +++++++
 ...ydockRepositoryOperationViewController.php |   5 +
 .../DrydockTestRepositoryOperation.php        |  53 +++++
 ...DrydockRepositoryOperationUpdateWorker.php |   3 +
 .../user/userguide/differential_land.diviner  |  53 +++++
 src/docs/user/userguide/drydock.diviner       | 203 +++++++++++++++++-
 .../user/userguide/drydock_blueprints.diviner |  80 +++++++
 src/docs/user/userguide/drydock_hosts.diviner | 126 +++++++++++
 .../userguide/drydock_quick_start.diviner     |  74 +++++++
 .../drydock_repository_automation.diviner     |  52 ++++-
 .../userguide/drydock_working_copies.diviner  |  39 ++++
 15 files changed, 776 insertions(+), 10 deletions(-)
 create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php
 create mode 100644 src/applications/drydock/operation/DrydockTestRepositoryOperation.php
 create mode 100644 src/docs/user/userguide/differential_land.diviner
 create mode 100644 src/docs/user/userguide/drydock_blueprints.diviner
 create mode 100644 src/docs/user/userguide/drydock_hosts.diviner
 create mode 100644 src/docs/user/userguide/drydock_quick_start.diviner
 create mode 100644 src/docs/user/userguide/drydock_working_copies.diviner

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index de893e82e4..df013da0bf 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -709,6 +709,7 @@ phutil_register_library_map(array(
     'DiffusionRepositoryRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php',
     'DiffusionRepositorySymbolsController' => 'applications/diffusion/controller/DiffusionRepositorySymbolsController.php',
     'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php',
+    'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php',
     'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php',
     'DiffusionResolveRefsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionResolveRefsConduitAPIMethod.php',
     'DiffusionResolveUserQuery' => 'applications/diffusion/query/DiffusionResolveUserQuery.php',
@@ -909,6 +910,7 @@ phutil_register_library_map(array(
     'DrydockSlotLock' => 'applications/drydock/storage/DrydockSlotLock.php',
     'DrydockSlotLockException' => 'applications/drydock/exception/DrydockSlotLockException.php',
     'DrydockSlotLockFailureLogType' => 'applications/drydock/logtype/DrydockSlotLockFailureLogType.php',
+    'DrydockTestRepositoryOperation' => 'applications/drydock/operation/DrydockTestRepositoryOperation.php',
     'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/DrydockWebrootInterface.php',
     'DrydockWorker' => 'applications/drydock/worker/DrydockWorker.php',
     'DrydockWorkingCopyBlueprintImplementation' => 'applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php',
@@ -4465,6 +4467,7 @@ phutil_register_library_map(array(
     'DiffusionRepositoryRemarkupRule' => 'PhabricatorObjectRemarkupRule',
     'DiffusionRepositorySymbolsController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryTag' => 'Phobject',
+    'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController',
     'DiffusionRequest' => 'Phobject',
     'DiffusionResolveRefsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionResolveUserQuery' => 'Phobject',
@@ -4705,6 +4708,7 @@ phutil_register_library_map(array(
     'DrydockSlotLock' => 'DrydockDAO',
     'DrydockSlotLockException' => 'Exception',
     'DrydockSlotLockFailureLogType' => 'DrydockLogType',
+    'DrydockTestRepositoryOperation' => 'DrydockRepositoryOperationType',
     'DrydockWebrootInterface' => 'DrydockInterface',
     'DrydockWorker' => 'PhabricatorWorker',
     'DrydockWorkingCopyBlueprintImplementation' => 'DrydockBlueprintImplementation',
diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
index 09a5b35f19..2e3d2a4110 100644
--- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
+++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php
@@ -103,6 +103,7 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
             'symbol/' => 'DiffusionRepositorySymbolsController',
             'staging/' => 'DiffusionRepositoryEditStagingController',
             'automation/' => 'DiffusionRepositoryEditAutomationController',
+            'testautomation/' => 'DiffusionRepositoryTestAutomationController',
           ),
           'pathtree/(?P<dblob>.*)' => 'DiffusionPathTreeController',
           'mirror/' => array(
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php
index 1c94e8bbc5..8fe5b70f97 100644
--- a/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php
+++ b/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php
@@ -4,7 +4,7 @@ final class DiffusionRepositoryEditAutomationController
   extends DiffusionRepositoryEditController {
 
   protected function processDiffusionRequest(AphrontRequest $request) {
-    $viewer = $request->getUser();
+    $viewer = $this->getViewer();
     $drequest = $this->diffusionRequest;
     $repository = $drequest->getRepository();
 
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php
index 5def1c754d..f18ffe4163 100644
--- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php
+++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php
@@ -688,6 +688,19 @@ final class DiffusionRepositoryEditMainController
         $this->getRepositoryControllerURI($repository, 'edit/automation/'));
     $view->addAction($edit);
 
+    $can_test = $repository->canPerformAutomation();
+
+    $test = id(new PhabricatorActionView())
+      ->setIcon('fa-gamepad')
+      ->setName(pht('Test Configuration'))
+      ->setWorkflow(true)
+      ->setDisabled(!$can_test)
+      ->setHref(
+        $this->getRepositoryControllerURI(
+          $repository,
+          'edit/testautomation/'));
+    $view->addAction($test);
+
     return $view;
   }
 
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php b/src/applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php
new file mode 100644
index 0000000000..6a9039889e
--- /dev/null
+++ b/src/applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php
@@ -0,0 +1,78 @@
+<?php
+
+final class DiffusionRepositoryTestAutomationController
+  extends DiffusionRepositoryEditController {
+
+  protected function processDiffusionRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $drequest = $this->diffusionRequest;
+    $repository = $drequest->getRepository();
+
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->withIDs(array($repository->getID()))
+      ->executeOne();
+    if (!$repository) {
+      return new Aphront404Response();
+    }
+
+    $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/');
+
+    if (!$repository->canPerformAutomation()) {
+      return $this->newDialog()
+        ->setTitle(pht('Automation Not Configured'))
+        ->appendParagraph(
+          pht(
+            'You can not run a configuration test for this repository '.
+            'because you have not configured repository automation yet. '.
+            'Configure it first, then test the configuration.'))
+        ->addCancelButton($edit_uri);
+    }
+
+    if ($request->isFormPost()) {
+      $op = new DrydockTestRepositoryOperation();
+
+      $operation = DrydockRepositoryOperation::initializeNewOperation($op)
+        ->setAuthorPHID($viewer->getPHID())
+        ->setObjectPHID($repository->getPHID())
+        ->setRepositoryPHID($repository->getPHID())
+        ->setRepositoryTarget('none:')
+        ->save();
+
+      $operation->scheduleUpdate();
+
+      $operation_id = $operation->getID();
+      $operation_uri = "/drydock/operation/{$operation_id}/";
+
+      return id(new AphrontRedirectResponse())
+        ->setURI($operation_uri);
+    }
+
+    return $this->newDialog()
+      ->setTitle(pht('Test Automation Configuration'))
+      ->appendParagraph(
+        pht(
+          'This configuration test will build a working copy of the '.
+          'repository and perform some basic validation. If it works, '.
+          'your configuration is substantially correct.'))
+      ->appendParagraph(
+        pht(
+          'The test will not perform any writes against the repository, so '.
+          'write operations may still fail even if the test passes. This '.
+          'test covers building and reading working copies, but not writing '.
+          'to them.'))
+      ->appendParagraph(
+        pht(
+          'If you run into write failures despite passing this test, '.
+          'it suggests that your setup is nearly correct but authentication '.
+          'is probably not fully configured.'))
+      ->addCancelButton($edit_uri)
+      ->addSubmitButton(pht('Start Test'));
+  }
+
+}
diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
index 9a509a5c04..e43ffbd367 100644
--- a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
+++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php
@@ -46,10 +46,15 @@ final class DrydockRepositoryOperationViewController
       ->setHeader($header)
       ->addPropertyList($properties);
 
+    $status_view = id(new DrydockRepositoryOperationStatusView())
+      ->setUser($viewer)
+      ->setOperation($operation);
+
     return $this->buildApplicationPage(
       array(
         $crumbs,
         $object_box,
+        $status_view,
       ),
       array(
         'title' => $title,
diff --git a/src/applications/drydock/operation/DrydockTestRepositoryOperation.php b/src/applications/drydock/operation/DrydockTestRepositoryOperation.php
new file mode 100644
index 0000000000..0b0efbad0a
--- /dev/null
+++ b/src/applications/drydock/operation/DrydockTestRepositoryOperation.php
@@ -0,0 +1,53 @@
+<?php
+
+final class DrydockTestRepositoryOperation
+  extends DrydockRepositoryOperationType {
+
+  const OPCONST = 'test';
+
+  public function getOperationDescription(
+    DrydockRepositoryOperation $operation,
+    PhabricatorUser $viewer) {
+    return pht('Test Configuration');
+  }
+
+  public function getOperationCurrentStatus(
+    DrydockRepositoryOperation $operation,
+    PhabricatorUser $viewer) {
+
+    $repository = $operation->getRepository();
+    switch ($operation->getOperationState()) {
+      case DrydockRepositoryOperation::STATE_WAIT:
+        return pht(
+          'Waiting to test configuration for %s...',
+          $repository->getMonogram());
+      case DrydockRepositoryOperation::STATE_WORK:
+        return pht(
+          'Testing configuration for %s. This may take a moment if Drydock '.
+          'has to clone the repository for the first time.',
+          $repository->getMonogram());
+      case DrydockRepositoryOperation::STATE_DONE:
+        return pht(
+          'Success! Automation is configured properly and Drydock can '.
+          'operate on %s.',
+          $repository->getMonogram());
+    }
+  }
+
+  public function applyOperation(
+    DrydockRepositoryOperation $operation,
+    DrydockInterface $interface) {
+    $repository = $operation->getRepository();
+
+    if ($repository->isGit()) {
+      $interface->execx('git status');
+    } else if ($repository->isHg()) {
+      $interface->execx('hg status');
+    } else if ($repository->isSVN()) {
+      $interface->execx('svn status');
+    } else {
+      throw new PhutilMethodNotImplementedException();
+    }
+  }
+
+}
diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
index f6e8b55e74..6160575fd0 100644
--- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php
@@ -165,6 +165,9 @@ final class DrydockRepositoryOperationUpdateWorker
           'branch' => $name,
         );
         break;
+      case 'none':
+        $spec = array();
+        break;
       default:
         throw new Exception(
           pht(
diff --git a/src/docs/user/userguide/differential_land.diviner b/src/docs/user/userguide/differential_land.diviner
new file mode 100644
index 0000000000..8d5f30d784
--- /dev/null
+++ b/src/docs/user/userguide/differential_land.diviner
@@ -0,0 +1,53 @@
+@title Differential User Guide: Automated Landing
+@group userguide
+
+Configuring Phabricator so you can "Land Revision" from the web UI.
+
+
+Overview
+========
+
+IMPORTANT: This feature is a prototype and has substantial limitations.
+
+Phabricator can be configured so that approved revisions may be published
+directly from the web interface. This can make publishing changes more
+convenient, particularly for open source projects where authors may not have
+commit access to the repository. This document explains the workflow and how to
+configure it.
+
+When properly configured, a {nav Land Revision} action will appear in
+Differential. This action works like `arc land` on the command line, and
+merges and publishes the revision.
+
+This feature has significant limitations:
+
+  - This feature is a prototype.
+  - This feature is only supported in Git.
+  - This feature always lands changes onto `master`.
+  - This feature does not currently provide chain of custody, and what lands
+    may be arbitrarily different than what is shown in Differential.
+
+To be landable, a revision must satisfy these requirements:
+
+  - It must belong to a repository which is tracked in Diffusion
+    (both hosted and imported repositories will work).
+  - The repository must have a **Staging Area** configured.
+  - The repository must have **Repository Automation** configured. For
+    details, see @{article:Drydock User Guide: Repository Automation}.
+  - The revision must have been created with `arc diff` and pushed to the
+    configured staging area at creation time.
+  - The user clicking the "Land Revision" button must have permission to push
+    to the repository.
+
+If these requirements are met, the {nav Land Revision} action should be
+available in the UI.
+
+
+Next Steps
+==========
+
+Continue by:
+
+  - configuring repository automation with
+    @{article:Drydock User Guide: Repository Automation}; or
+  - returning to the @{article:Differential User Guide}.
diff --git a/src/docs/user/userguide/drydock.diviner b/src/docs/user/userguide/drydock.diviner
index 49b1f1df9e..44a645bf8a 100644
--- a/src/docs/user/userguide/drydock.diviner
+++ b/src/docs/user/userguide/drydock.diviner
@@ -15,16 +15,19 @@ applications coordinate during complex build and deployment tasks. Typically,
 you will configure Drydock to enable capabilities in other applications:
 
   - Harbormaster can use Drydock to host builds.
-  - In the future, Differential will be able to use Drydock to perform
-    server-side merges.
+  - Differential can use Drydock to perform server-side merges.
 
 Users will not normally interact with Drydock directly.
 
+If you want to get started with Drydock right away, see
+@{article:Drydock User Guide: Quick Start} for specific instructions on
+configuring integrations.
+
 
 What Drydock Does
 =================
 
-Drydock manages working copies, build hosts, and other software and hardware
+Drydock manages working copies, hosts, and other software and hardware
 resources that build and deployment processes may require in order to perform
 useful work.
 
@@ -49,15 +52,202 @@ doing the actual work.
 
 Drydock solves these scaling problems by providing a central allocation
 framework for //resources//, which are physical or virtual resources like a
-build host or a working copy. Processes which need to share hardware or
-software can use Drydock to coordinate creation, access, and destruction of
-those resources.
+host or a working copy. Processes which need to share hardware or software can
+use Drydock to coordinate creation, access, and destruction of those resources.
 
 Applications ask Drydock for resources matching a description, and it allocates
 a corresponding resource by either finding a suitable unused resource or
 creating a new resource. When work completes, the resource is returned to the
 resource pool or destroyed.
 
+
+Getting Started with Drydock
+============================
+
+In general, you will interact with Drydock by configuring blueprints, which
+tell Drydock how to build resources. You can jump into this topic directly
+in @{article:Drydock Blueprints}.
+
+For help on configuring specific application features:
+
+  - to configure server-side merges from Differential, see
+    @{article:Differential User Guide: Automated Landing}.
+
+You should also understand the Drydock security model before deploying it
+in a production environment. See @{article:Drydock User Guide: Security}.
+
+The remainder of this document has some additional high-level discussion about
+how Drydock works and why it works that way, which may be helpful in
+understanding the application as a whole.
+
+
+Drydock Concepts
+================
+
+The major concepts in Drydock are **Blueprints**, **Resources**, **Leases**,
+and the **Allocator**.
+
+**Blueprints** are configuration that tells Drydock how to create resources:
+where it can put them, how to access them, how many it can make at once, who is
+allowed to ask for access to them, how to actually build them, how to clean
+them up when they are no longer in use, and so on.
+
+Drydock starts without any blueprints. You'll add blueprints to configure
+Drydock and enable it to satisfy requests for resources. You can learn more
+about blueprints in @{article:Drydock Blueprints}.
+
+**Resources** represent things (like hosts or working copies) that Drydock has
+created, is managing the lifecycle for, and can give other applications access
+to.
+
+**Leases** are requests for resources with certain qualities by other
+applications. For example, Harbormaster may request a working copy of a
+particular repository so it can run unit tests.
+
+The **Allocator** is where Drydock actually does work. It works roughly like
+this:
+
+  - An application creates a lease describing a resource it needs, and
+    uses this lease to ask Drydock for an appropriate resource.
+  - Drydock looks at free resources to try to find one it can use to satisfy
+    the request. If it finds one, it marks the resource as in use and gives
+    the application details about how to access it.
+  - If it can't find an appropriate resource that already exists, it looks at
+    the blueprints it has configured to try to build one. If it can, it creates
+    a new resource, then gives the application access to it.
+  - Once the application finishes using the resource, it frees it. Depending
+    on configuration, Drydock may reuse it, destroy it, or hold onto it and
+    make a decision later.
+
+Some minor concepts in Drydock are **Slot Locks** and **Repository Operations**.
+
+**Slot Locks** are simple optimistic locks that most Drydock blueprints use to
+avoid race conditions. Their design is not particularly interesting or novel,
+they're just a fairly good fit for most of the locking problems that Drydock
+blueprints tend to encounter and Drydock provides APIs to make them easy to
+work with.
+
+**Repository Operations** help other applications coordinate writes to
+repositories. Multiple applications perform similar kinds of writes, and these
+writes require more sequencing/coordination and user feedback than other
+operations.
+
+
+Architecture Overview
+=====================
+
+This section describes some of Drydock's design goals and architectural
+choices, so you can understand its strengths and weaknesses and which problem
+domains it is well or poorly suited for.
+
+A typical use case for Drydock is giving another application access to a
+working copy in order to run a build or unit test operation. Drydock can
+satisfy the request and resume execution of application code in 1-2 seconds
+under reasonable conditions and with moderate tradeoffs, and can satisfy a
+large number of these requests in parallel.
+
+**Scalable**: Drydock is designed to scale easily to something in the realm of
+thousands of hosts in hundreds of pools, and far beyond that with a little
+work.
+
+Drydock is intended to solve resource management problems at very large scales
+and minimzes blocking operations, locks, and artificial sequencing. Drydock is
+designed to fully utilize an almost arbitrarily large pool of resources and
+improve performance roughly linearly with available hardware.
+
+Because the application assumes that deployment at this scale and complexity
+level is typical, you may need to configure more things and do more work than
+you would under the simplifying assumptions of small scale.
+
+**Heavy Resources**: Drydock assumes that resources are relatively
+heavyweight and and require a meaningful amount (a second or more) of work to
+build, maintain and tear down. It also assumes that leases will often have
+substantial lifespans (seconds or minutes) while performing operations.
+
+Resources like working copies (which typically take several seconds to create
+with a command like `git clone`) and VMs (which typically take several seconds
+to spin up) are good fits for Drydock and for the problems it is intended to
+solve.
+
+Lease operations like running unit tests, performing builds, executing merges,
+generating documentation and running temporary services (which typically last
+at least a few seconds) are also good fits for Drydock.
+
+In both cases, the general concern with lightweight resources and operations is
+that Drydock operation overhead is roughly on the order of a second for many
+tasks, so overhead from Drydock will be substantial if resources are built and
+torn down in a few milliseconds or lease operations require only a fraction of
+a second to execute.
+
+As a rule of thumb, Drydock may be a poor fit for a problem if operations
+typically take less than a second to build, execute, and destroy.
+
+**Focus on Resource Construction**: Drydock is primarily solving a resource
+construction problem: something needs a resource matching some description, so
+Drydock finds or builds that resource as quickly as possible.
+
+Drydock generally prioritizes responding to requests quickly over other
+concerns, like minimizing waste or performing complex scheduling. Although you
+can make adjustments to some of these behaviors, it generally assumes that
+resources are cheap compared to the cost of waiting for resource construction.
+
+This isn't to say that Drydock is grossly wasteful or has a terrible scheduler,
+just that efficient utilization and efficient scheduling aren't the primary
+problems the design focuses on.
+
+This prioritization corresponds to scenarios where resources are something like
+hosts or working copies, and operations are something like builds, and the cost
+of hosts and storage is small compared to the cost of engineer time spent
+waiting on jobs to get scheduled.
+
+Drydock may be a weak fit for a problem if it is bounded by resource
+availability and using resources as efficiently as possible is very important.
+Drydock generally assumes you will respond to a resource deficit by making more
+resources available (usually very cheap), rather than by paying engineers to
+wait for operations to complete (usually very expensive).
+
+**Isolation Tradeoffs**: Drydock assumes that multiple operations running at
+similar levels of trust may be interested in reducing isolation to improve
+performance, reduce complexity, or satisfy some other similar goal. It does not
+guarantee isolation and assumes most operations will not run in total isolation.
+
+If this isn't true for your use case, you'll need to be careful in configuring
+Drydock to make sure that operations are fully isolated and can not interact.
+Complete isolation will reduce the performance of the allocator as it will
+generally prevent it from reusing resources, which is one of the major ways it
+can improve performance.
+
+You can find more discussion of these tradeoffs in
+@{article:Drydock User Guide: Security}.
+
+**Agentless**: Drydock does not require an agent or daemon to be installed on
+hosts. It interacts with hosts over SSH.
+
+**Very Abstract**: Drydock's design is //extremely// abstract. Resources have
+very little hardcoded behavior. The allocator has essentially zero specialized
+knowledge about what it is actually doing.
+
+One aspect of this abstractness is that Drydock is composable, and solves
+complex allocation problems by //asking itself// to build the pieces it needs.
+To build a working copy, Drydock first asks itself for a suitable host. It
+solves this allocation sub-problem, then resolves the original request.
+
+This allows new types of resources to build on Drydock's existing knowledge of
+resource construction by just saying "build one of these other things you
+already know how to build, then apply a few adjustments". This also means that
+you can tell Drydock about a new way to build hosts (say, bring up VMs from a
+different service provider) and the rest of the pipeline can use these new
+hosts interchangeably with the old hosts.
+
+While this design theoretically makes Drydock more powerful and more flexible
+than a less abstract approach, abstraction is frequently a double-edged sword.
+
+Drydock is almost certainly at the extreme upper end of abstraction for tools
+in this space, and the level of abstraction may ultimately match poorly with a
+particular problem domain. Alternative approaches may give you more specialized
+and useful tools for approaching a given problem.
+
+
 Next Steps
 ==========
 
@@ -65,5 +255,6 @@ Continue by:
 
   - understanding Drydock security concerns with
     @{article:Drydock User Guide: Security}; or
+  - learning about blueprints in @{article:Drydock Blueprints}; or
   - allowing Phabricator to write to repositories with
     @{article:Drydock User Guide: Repository Automation}.
diff --git a/src/docs/user/userguide/drydock_blueprints.diviner b/src/docs/user/userguide/drydock_blueprints.diviner
new file mode 100644
index 0000000000..46c8a27f5e
--- /dev/null
+++ b/src/docs/user/userguide/drydock_blueprints.diviner
@@ -0,0 +1,80 @@
+@title Drydock Blueprints
+@group userguide
+
+Overview of Drydock blueprint types.
+
+
+Overview
+========
+
+IMPORTANT: Drydock is not a mature application and may be difficult to
+configure and use for now.
+
+Drydock builds and manages various hardware and software resources, like
+hosts and repository working copies. Other applications can use these resources
+to perform useful work (like running tests or builds).
+
+For additional disussion of Drydock, see @{article:Drydock User Guide}.
+
+Drydock can't create any resources until you configure it. You'll configure
+Drydock by creating **Blueprints**. Each blueprint tells Drydock how to build
+a specific kind of resource, how many it is allowed to build, where it should
+build them, who is authorized to request them, and so on.
+
+You can create a new blueprint in Drydock from the web UI:
+
+{nav Drydock > Blueprints > New Blueprint}
+
+Each blueprint builds resources of a specific type, like hosts or repository
+working copies. Detailed topic guides are available for each resource type:
+
+**Hosts**: Hosts are the building block for most other resources. For details,
+see @{article:Drydock Blueprints: Hosts}.
+
+**Working Copies**: Working copies allow Drydock to perform repository
+operations like running tests, performing builds, and handling merges.
+
+
+Authorizing Access
+==================
+
+Before objects in other applications can use a blueprint, the blueprint must
+authorize them.
+
+This mostly serves to prevent users with limited access from executing
+operations on trusted hosts. For additional discussion, see
+@{article:Drydock User Guide: Security}.
+
+This also broadly prevents Drydock from surprising you by coming up with a
+valid but unintended solution to an allocation problem which runs some
+operation on resources that are techincally suitable but not desirable. For
+example, you may not want your Android builds running on your iPhone build
+tier, even if there's no technical reason they can't.
+
+You can review active authorizations and pending authorization requests in
+the "Active Authorizations" section of the blueprint detail screen.
+
+To approve an authorization, click it and select {nav Approve Authorization}.
+Until you do, the requesting object won't be able to access resources from
+the blueprint.
+
+You can also decline an authorization. This prevents use of resources and
+removes it from the authorization approval queue.
+
+
+Disabling Blueprints
+====================
+
+You can disable a blueprint by selecting {nav Disable Blueprint} from the
+blueprint detail screen.
+
+Disabled blueprints will no longer be used for new allocations. However,
+existing resources will continue to function.
+
+
+Next Steps
+==========
+
+Continue by:
+
+  - returning to the @{article:Drydock User Guide}.
diff --git a/src/docs/user/userguide/drydock_hosts.diviner b/src/docs/user/userguide/drydock_hosts.diviner
new file mode 100644
index 0000000000..8bfed7dc60
--- /dev/null
+++ b/src/docs/user/userguide/drydock_hosts.diviner
@@ -0,0 +1,126 @@
+@title Drydock Blueprints: Hosts
+@group userguide
+
+Guide to configuring Drydock host blueprints.
+
+
+Overview
+========
+
+IMPORTANT: Drydock is not a mature application and may be difficult to
+configure and use for now.
+
+To give Drydock access to machines so it can perform work, you'll configure
+**host blueprints**. These blueprints tell Drydock where to find machines (or
+how to build machines) and how to connect to them.
+
+Once Drydock has access to hosts it can use them to build more interesting and
+complex types of resources, like repository working copies.
+
+Drydock currently supports these kinds of host blueprints:
+
+  - **Almanac Hosts**: Gives Drydock access to a predefined list of hosts.
+
+Drydock may support additional blueprints in the future.
+
+
+Security
+========
+
+Drydock can be used to run semi-trusted and untrusted code, and you may want
+to isolate specific processes or classes of processes from one another. See
+@{article:Drydock User Guide: Security} for discussion of security
+concerns and guidance on how to make isolation tradeoffs.
+
+
+General Considerations
+======================
+
+**You must install software on hosts.** Drydock does not currently handle
+installing software on hosts. You'll need to make sure any hosts are configured
+properly with any software you need, and have tools like `git`, `hg` or `svn`
+that may be required to interact with working copies.
+
+You do **not** need to install PHP, arcanist, libphutil or Phabricator on the
+hosts unless you are specifically running `arc` commands.
+
+**You must configure authentication.** Drydock also does not handle credentials
+for VCS operations. If you're interacting with repositories hosted on
+Phabricator, the simplest way to set this up is something like this:
+
+  - Create a new bot user in Phabricator.
+  - In {nav Settings > SSH Public Keys}, add a public key or generate a
+    keypair.
+  - Put the private key on your build hosts as `~/.ssh/id_rsa` for whatever
+    user you're connecting with.
+
+This will let processes on the host access Phabricator as the bot user, and
+use the bot user's permissions to pull and push changes.
+
+If you're using hosted repositories from an external service, you can follow
+similar steps for that service.
+
+Note that any processes running under the given user account will have access
+to the private key, so you should give the bot the smallest acceptable level of
+permissions if you're running semi-trusted or untrusted code like unit tests.
+
+**You must create a `/var/drydock` directory.** This is hard-coded in Drydock
+for now, so you need to create it on the hosts. This can be a symlink to
+a different location if you prefer.
+
+
+Almanac Hosts
+=============
+
+The **Almanac Hosts** blueprint type gives Drydock access to a predefined list
+of hosts which you configure in the Almanac application. This is the simplest
+type of blueprint to set up.
+
+For more information about Almanac, see @{article:Almanac User Guide}.
+
+For example, suppose you have `build001.mycompany.com` and
+`build002.mycompany.com`, and want to configure Drydock to be able to use these
+hosts. To do this:
+
+**Create Almanac Devices**: Create a device record in Almanac for each your
+hosts.
+
+{nav Almanac > Devices > Create Device}
+
+Enter the device names (like `build001.mycompany.com`). After creating the
+devices, use {nav Add Interface} to configure the ports and IP addresses that
+Drydock should connect to over SSH (normally, this is port `22`).
+
+**Create an Almanac Service**: In the Almanac application, create a new service
+to define the pool of devices you want to use.
+
+{nav Almanac > Services > Create Service}
+
+Choose the service type **Drydock: Resource Pool**. This will allow Drydock
+to use the devices that are bound to the service.
+
+Now, use {nav Add Binding} to bind all of the devices to the service.
+
+You can add more hosts to the pool later by binding additional devices, and
+Drydock will automatically start using them. Likewise, you can remove bindings
+to take hosts out of service.
+
+**Create a Drydock Blueprint**: Now, create a new blueprint in Drydock.
+
+{nav Drydock > Blueprints > New Blueprint}
+
+Choose the **Almanac Hosts** blueprint type.
+
+In **Almanac Services**, select the service you previously created. For
+**Credentials**, select an SSH private key you want Drydock to use to connect
+to the hosts.
+
+Drydock should now be able to build resources from these hosts.
+
+
+Next Steps
+==========
+
+Continue by:
+
+  - returning to @{article:Drydock Blueprints}.
diff --git a/src/docs/user/userguide/drydock_quick_start.diviner b/src/docs/user/userguide/drydock_quick_start.diviner
new file mode 100644
index 0000000000..4b0bd7110d
--- /dev/null
+++ b/src/docs/user/userguide/drydock_quick_start.diviner
@@ -0,0 +1,74 @@
+@title Drydock User Guide: Quick Start
+@group userguide
+
+Guide to getting Drydock
+
+Quick Start: Land Revisions
+===========================
+
+Quick start guide to getting "Land Revision" working in Differential. For
+a more detailed guide, see @{article:Drydock User Guide: Repository Automation}.
+
+Choose a repository you want to enable "Land Revision" for. We'll call this
+**Repository X**.
+
+You need to configure a staging area for this repository if you haven't
+already. You can do this in Diffusion in {nav Edit Repository > Edit Staging}.
+We'll call this **Staging Area Y**.
+
+Choose or create a host you want to run merges on. We'll call this
+`automation001`. For example, you might bring up a new host in EC2 and
+label it `automation001.mycompany.com`. You can use an existing host if you
+prefer.
+
+Create a user account on the host, or choose an existing user account. This is
+the user that merges will execute under: Drydock will connect to it and run a
+bunch of `git` commands, then ultimately run `git push`. We'll call this user
+`builder`.
+
+Install `git`, `hg` or `svn` if you haven't already and set up private keys
+for `builder` so it can pull and push any repositories you want to operate
+on.
+
+If your repository and/or staging area are hosted in Phabricator, you may want
+to create a corresponding bot account so you can add keys and give it
+permissions.
+
+At this point you should be able to `ssh builder@automation001` to connect to
+the host, and get a normal shell. You should be able to `git clone ...` from
+**Repository X** and from **Staging Area Y**, and `git push` to **Repository
+X**. If you can't, configure things so you can.
+
+Now, create a host blueprint for the host. You can find a more detailed
+walkthrough in @{article:Drydock Blueprints: Hosts}. Briefly:
+
+  - Create an Almanac device for the host. This should have the IP address and
+    port for your host.
+  - Create an Almanac service bound to the device. This should be a Drydock
+    resource pool service and have a binding to the IP from the previous step.
+  - Create a Drydock host blueprint which uses the service from the previous
+    step. It should be configured with an SSH private key that can be used
+    to connect to `builder@automation001`.
+
+Then, create a new working copy blueprint which uses the host blueprint you
+just made. You can find a more detailed walkthrough in @{article:Drydock
+Blueprints: Working Copies}. Authorize the working copy blueprint to use the
+host blueprint.
+
+Finally, configure repository automation for **Repository X**:
+{nav Edit Repository > Edit Automation}. Provide the working copy blueprint
+from the previous step. Authorize the repository to use the working copy
+blueprint.
+
+After you save changes, click {nav Test Configuration} to test that things
+are working properly.
+
+The "Land Revision" action should now be available on revisions for this
+repository.
+
+Next Steps
+==========
+
+Continue by:
+
+  - returning to @{article:Drydock User Guide}.
diff --git a/src/docs/user/userguide/drydock_repository_automation.diviner b/src/docs/user/userguide/drydock_repository_automation.diviner
index a16e8d33e9..35ef932cc2 100644
--- a/src/docs/user/userguide/drydock_repository_automation.diviner
+++ b/src/docs/user/userguide/drydock_repository_automation.diviner
@@ -7,12 +7,19 @@ Configuring repository automation so Phabricator can push commits.
 Overview
 ========
 
-IMPORTANT: This feature is very new and most of the capabilities described
+IMPORTANT: This feature is very new and some of the capabilities described
 in this document are not yet available. This feature as a whole is a prototype.
 
 By configuring Drydock and Diffusion appropriately, you can enable **Repository
-Automation** for a repository. Once automation is set up, Phabricator will be
-able to make changes to the repository.
+Automation** for a repository. This will allow Phabricator to make changes
+to the repository.
+
+
+Limitations
+===========
+
+  - This feature is a prototype.
+  - Only Git is supported.
 
 
 Security
@@ -29,6 +36,45 @@ with automation. You can read more about this in
 @{article:Drydock User Guide: Security}.
 
 
+Configuring Automation
+======================
+
+To configure automation, use {nav Edit Repository > Edit Automation} from
+Diffusion.
+
+On the configuration screen, specify one or more working copy blueprints in
+Drydock (usually, you'll just use one). Repository automation will use working
+copies built by these blueprints to perform merges and push changes.
+
+For more details on configuring these blueprints, see
+@{article:Drydock Blueprints: Working Copies}.
+
+After selecting one or more blueprints, make sure you authorize the repository
+to use them. Automation operations won't be able to proceed until you do. The
+UI will remind you if you have unauthorized blueprints selected.
+
+
+Testing Configuration
+=====================
+
+Once the blueprints are configured and authorized, use {nav Test Configuration}
+to check that things are configured correctly. This will build a working copy
+in Drydock, connect to it, and run a trivial command (like `git status`) to
+make sure things work.
+
+If it's the first time you're doing this, it may take a few moments since it
+will need to clone a fresh working copy.
+
+If the test is successful, your configuration is generally in good shape. If
+not, it should give you more details about what went wrong.
+
+Since the test doesn't actually do a push, it's possible that you may have
+everything configured properly //except// write access. In this case, you'll
+run into a permission error when you try to actually perform a merge or other
+similar write. If you do, adjust permissions or credentials appropriately so
+the working copy can be pushed from.
+
+
 Next Steps
 ==========
 
diff --git a/src/docs/user/userguide/drydock_working_copies.diviner b/src/docs/user/userguide/drydock_working_copies.diviner
new file mode 100644
index 0000000000..2e8eb293c7
--- /dev/null
+++ b/src/docs/user/userguide/drydock_working_copies.diviner
@@ -0,0 +1,39 @@
+@title Drydock Blueprints: Working Copies
+@group userguide
+
+Guide to configuring Drydock working copy blueprints.
+
+
+Overview
+========
+
+IMPORTANT: Drydock is not a mature application and may be difficult to
+configure and use for now.
+
+To let Drydock build repository working copies in order to run unit tests and
+other similar operations, you'll configure **working copy blueprints**.
+
+Working Copies
+==============
+
+Working copy blueprints rely on host blueprints, so you'll need to configure
+a suitable host blueprint first. See @{article:Drydock Blueprints: Hosts}.
+
+To configure a working copy blueprint, choose the host blueprints it should
+use in **Use Blueprints**.
+
+You can optionally specify a **Limit**. If you do, the blueprint won't be
+allowed to create more than this many simultaneous resources. If you leave
+it empty, the blueprint will be able to create an unlimited number of
+resources.
+
+After you save the blueprint, make sure you authorize it to use the selected
+host blueprints. It won't be able to acquire host resources until you do.
+
+
+Next Steps
+==========
+
+Continue by:
+
+  - returning to @{article:Drydock Blueprints}.

From 1c7443f8f242a1f6b9fb10b5b90178fabf2f1e42 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 27 Oct 2015 18:51:59 +0000
Subject: [PATCH 24/39] Make "Land Revision" button state consistent, prevent
 non-accepted lands

Summary:
Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work.

Don't allow "land" to proceed unless the revision is accepted.

Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D14350
---
 ...ifferentialRevisionOperationController.php | 100 ++----------------
 ...erentialLandingActionMenuEventListener.php |  20 +++-
 .../DrydockLandRepositoryOperation.php        |  97 +++++++++++++++++
 3 files changed, 120 insertions(+), 97 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php
index 85f2a0b1b2..a5d5b5f98f 100644
--- a/src/applications/differential/controller/DifferentialRevisionOperationController.php
+++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php
@@ -18,86 +18,13 @@ final class DifferentialRevisionOperationController
 
     $detail_uri = "/D{$id}";
 
-    $repository = $revision->getRepository();
-    if (!$repository) {
-      return $this->rejectOperation(
-        $revision,
-        pht('No Repository'),
-        pht(
-          'This revision is not associated with a known repository. Only '.
-          'revisions associated with a tracked repository can be landed '.
-          'automatically.'));
-    }
-
-    if (!$repository->canPerformAutomation()) {
-      return $this->rejectOperation(
-        $revision,
-        pht('No Repository Automation'),
-        pht(
-          'The repository this revision is associated with ("%s") is not '.
-          'configured to support automation. Configure automation for the '.
-          'repository to enable revisions to be landed automatically.',
-          $repository->getMonogram()));
-    }
-
-    // TODO: At some point we should allow installs to give "land reviewed
-    // code" permission to more users than "push any commit", because it is
-    // a much less powerful operation. For now, just require push so this
-    // doesn't do anything users can't do on their own.
-    $can_push = PhabricatorPolicyFilter::hasCapability(
-      $viewer,
-      $repository,
-      DiffusionPushCapability::CAPABILITY);
-    if (!$can_push) {
-      return $this->rejectOperation(
-        $revision,
-        pht('Unable to Push'),
-        pht(
-          'You do not have permission to push to the repository this '.
-          'revision is associated with ("%s"), so you can not land it.',
-          $repository->getMonogram()));
-    }
-
     $op = new DrydockLandRepositoryOperation();
-
-    // Check for other operations. Eventually this should probably be more
-    // general (e.g., it's OK to land to multiple different branches
-    // simultaneously) but just put this in as a sanity check for now.
-    $other_operations = id(new DrydockRepositoryOperationQuery())
-      ->setViewer($viewer)
-      ->withObjectPHIDs(array($revision->getPHID()))
-      ->withOperationTypes(
-        array(
-          $op->getOperationConstant(),
-        ))
-      ->withOperationStates(
-        array(
-          DrydockRepositoryOperation::STATE_WAIT,
-          DrydockRepositoryOperation::STATE_WORK,
-          DrydockRepositoryOperation::STATE_DONE,
-        ))
-      ->execute();
-
-    if ($other_operations) {
-      $any_done = false;
-      foreach ($other_operations as $operation) {
-        if ($operation->isDone()) {
-          $any_done = true;
-          break;
-        }
-      }
-
-      if ($any_done) {
-        return $this->rejectOperation(
-          $revision,
-          pht('Already Complete'),
-          pht('This revision has already landed.'));
-      } else {
-        return $this->rejectOperation(
-          $revision,
-          pht('Already In Flight'),
-          pht('This revision is already landing.'));
-      }
+    $barrier = $op->getBarrierToLanding($viewer, $revision);
+    if ($barrier) {
+      return $this->newDialog()
+        ->setTitle($barrier['title'])
+        ->appendParagraph($barrier['body'])
+        ->addCancelButton($detail_uri);
     }
 
     if ($request->isFormPost()) {
@@ -106,6 +33,7 @@ final class DifferentialRevisionOperationController
       // occurs.
 
       $diff = $revision->getActiveDiff();
+      $repository = $revision->getRepository();
 
       $operation = DrydockRepositoryOperation::initializeNewOperation($op)
         ->setAuthorPHID($viewer->getPHID())
@@ -136,18 +64,4 @@ final class DifferentialRevisionOperationController
       ->addSubmitButton(pht('Mutate Repository Unpredictably'));
   }
 
-  private function rejectOperation(
-    DifferentialRevision $revision,
-    $title,
-    $body) {
-
-    $id = $revision->getID();
-    $detail_uri = "/D{$id}";
-
-    return $this->newDialog()
-      ->setTitle($title)
-      ->appendParagraph($body)
-      ->addCancelButton($detail_uri);
-  }
-
 }
diff --git a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php
index 7dcf3f462d..2cf3eaa4e5 100644
--- a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php
+++ b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php
@@ -26,7 +26,9 @@ final class DifferentialLandingActionMenuEventListener
   }
 
   private function renderRevisionAction(PhutilEvent $event) {
-    if (!$this->canUseApplication($event->getUser())) {
+    $viewer = $event->getUser();
+
+    if (!$this->canUseApplication($viewer)) {
       return null;
     }
 
@@ -40,11 +42,22 @@ final class DifferentialLandingActionMenuEventListener
     if ($repository->canPerformAutomation()) {
       $revision_id = $revision->getID();
 
+      $op = new DrydockLandRepositoryOperation();
+      $barrier = $op->getBarrierToLanding($viewer, $revision);
+
+      if ($barrier) {
+        $can_land = false;
+      } else {
+        $can_land = true;
+      }
+
       $action = id(new PhabricatorActionView())
-        ->setWorkflow(true)
         ->setName(pht('Land Revision'))
         ->setIcon('fa-fighter-jet')
-        ->setHref("/differential/revision/operation/{$revision_id}/");
+        ->setHref("/differential/revision/operation/{$revision_id}/")
+        ->setWorkflow(true)
+        ->setDisabled(!$can_land);
+
 
       $this->addActionMenuItems($event, $action);
     }
@@ -54,7 +67,6 @@ final class DifferentialLandingActionMenuEventListener
       ->execute();
 
     foreach ($strategies as $strategy) {
-      $viewer = $event->getUser();
       $action = $strategy->createMenuItem($viewer, $revision, $repository);
       if ($action == null) {
         continue;
diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
index 00b5c71181..6da4c2ae5d 100644
--- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
+++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
@@ -189,4 +189,101 @@ final class DrydockLandRepositoryOperation
     return $diff;
   }
 
+  public function getBarrierToLanding(
+    PhabricatorUser $viewer,
+    DifferentialRevision $revision) {
+
+    $repository = $revision->getRepository();
+    if (!$repository) {
+      return array(
+        'title' => pht('No Repository'),
+        'body' => pht(
+          'This revision is not associated with a known repository. Only '.
+          'revisions associated with a tracked repository can be landed '.
+          'automatically.'),
+      );
+    }
+
+    if (!$repository->canPerformAutomation()) {
+      return array(
+        'title' => pht('No Repository Automation'),
+        'body' => pht(
+          'The repository this revision is associated with ("%s") is not '.
+          'configured to support automation. Configure automation for the '.
+          'repository to enable revisions to be landed automatically.',
+          $repository->getMonogram()),
+      );
+    }
+
+    // TODO: At some point we should allow installs to give "land reviewed
+    // code" permission to more users than "push any commit", because it is
+    // a much less powerful operation. For now, just require push so this
+    // doesn't do anything users can't do on their own.
+    $can_push = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $repository,
+      DiffusionPushCapability::CAPABILITY);
+    if (!$can_push) {
+      return array(
+        'title' => pht('Unable to Push'),
+        'body' => pht(
+          'You do not have permission to push to the repository this '.
+          'revision is associated with ("%s"), so you can not land it.',
+          $repository->getMonogram()),
+      );
+    }
+
+    $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+    if ($revision->getStatus() !== $status_accepted) {
+      return array(
+        'title' => pht('Revision Not Accepted'),
+        'body' => pht(
+          'This revision is still under review. Only revisions which have '.
+          'been accepted may land.'),
+      );
+    }
+
+    // Check for other operations. Eventually this should probably be more
+    // general (e.g., it's OK to land to multiple different branches
+    // simultaneously) but just put this in as a sanity check for now.
+    $other_operations = id(new DrydockRepositoryOperationQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($revision->getPHID()))
+      ->withOperationTypes(
+        array(
+          $this->getOperationConstant(),
+        ))
+      ->withOperationStates(
+        array(
+          DrydockRepositoryOperation::STATE_WAIT,
+          DrydockRepositoryOperation::STATE_WORK,
+          DrydockRepositoryOperation::STATE_DONE,
+        ))
+      ->execute();
+
+    if ($other_operations) {
+      $any_done = false;
+      foreach ($other_operations as $operation) {
+        if ($operation->isDone()) {
+          $any_done = true;
+          break;
+        }
+      }
+
+      if ($any_done) {
+        return array(
+          'title' => pht('Already Complete'),
+          'body' => pht('This revision has already landed.'),
+        );
+      } else {
+        return array(
+          'title' => pht('Already In Flight'),
+          'body' => pht('This revision is already landing.'),
+        );
+      }
+    }
+
+    return null;
+  }
+
 }

From 198bf1198dafa819f7a96e226c145562f1fed97a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 27 Oct 2015 12:06:48 -0700
Subject: [PATCH 25/39] Fix "Accepted" status constant in landing

Summary: I didn't test the positive version of this -- the constant has value `2` but when we read it from the database it's `"2"` or whatever. Just do this for now and maybe someday we'll use strings.

Test Plan: will do production things

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14352
---
 .../drydock/operation/DrydockLandRepositoryOperation.php        | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
index 6da4c2ae5d..48a3e8075f 100644
--- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
+++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php
@@ -234,7 +234,7 @@ final class DrydockLandRepositoryOperation
     }
 
     $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
-    if ($revision->getStatus() !== $status_accepted) {
+    if ($revision->getStatus() != $status_accepted) {
       return array(
         'title' => pht('Revision Not Accepted'),
         'body' => pht(

From 4b5de5135c29821d52536045cafa58f46b7a78de Mon Sep 17 00:00:00 2001
From: Chad Little <chad@chadsdomain.com>
Date: Tue, 27 Oct 2015 12:07:28 -0700
Subject: [PATCH 26/39] Fix landing icon

Summary: This is //hilarious//.

Test Plan: Test icon on local install.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14351
---
 src/applications/drydock/storage/DrydockRepositoryOperation.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php
index c6fc383193..83e660ffa9 100644
--- a/src/applications/drydock/storage/DrydockRepositoryOperation.php
+++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php
@@ -99,7 +99,7 @@ final class DrydockRepositoryOperation extends DrydockDAO
   public static function getOperationStateIcon($state) {
     $map = array(
       self::STATE_WAIT => 'fa-clock-o',
-      self::STATE_WORK => 'fa-refresh blue',
+      self::STATE_WORK => 'fa-plane ph-spin blue',
       self::STATE_DONE => 'fa-check green',
       self::STATE_FAIL => 'fa-times red',
     );

From 218ab398b0ef1c47809836b420091688560d5716 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@chadsdomain.com>
Date: Tue, 27 Oct 2015 19:32:35 +0000
Subject: [PATCH 27/39] Allow ObjectLists to be set to Dialogs

Summary: Better formatting for object lists when in a dialog (like subscribers).

Test Plan:
Test a subscription list.

{F911522}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14353
---
 resources/celerity/map.php                    |  6 ++---
 .../view/SubscriptionListDialogBuilder.php    |  5 ++--
 src/view/AphrontDialogView.php                | 24 +++++++++++++++----
 webroot/rsrc/css/aphront/dialog-view.css      | 10 ++++++++
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index ee959fa58a..00498fa482 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => 'd80f5c3e',
+    'core.pkg.css' => 'd25d16e4',
     'core.pkg.js' => '47dc9ebb',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '2de124c9',
@@ -18,7 +18,7 @@ return array(
     'maniphest.pkg.js' => '3ec6a6d5',
     'rsrc/css/aphront/aphront-bars.css' => '231ac33c',
     'rsrc/css/aphront/dark-console.css' => '6378ef3d',
-    'rsrc/css/aphront/dialog-view.css' => 'fe58b18d',
+    'rsrc/css/aphront/dialog-view.css' => 'be0e3a46',
     'rsrc/css/aphront/lightbox-attachment.css' => '7acac05d',
     'rsrc/css/aphront/list-filter-view.css' => '5d6f0526',
     'rsrc/css/aphront/multi-column.css' => 'fd18389d',
@@ -489,7 +489,7 @@ return array(
     'almanac-css' => 'dbb9b3af',
     'aphront-bars' => '231ac33c',
     'aphront-dark-console-css' => '6378ef3d',
-    'aphront-dialog-view-css' => 'fe58b18d',
+    'aphront-dialog-view-css' => 'be0e3a46',
     'aphront-list-filter-view-css' => '5d6f0526',
     'aphront-multi-column-view-css' => 'fd18389d',
     'aphront-panel-view-css' => '8427b78d',
diff --git a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
index a4d382e170..7d1b8d799c 100644
--- a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
+++ b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php
@@ -54,15 +54,14 @@ final class SubscriptionListDialogBuilder extends Phobject {
       ->setUser($this->getViewer())
       ->setWidth(AphrontDialogView::WIDTH_FORM)
       ->setTitle($this->getTitle())
-      ->appendChild($this->buildBody($this->getViewer(), $handles))
+      ->setObjectList($this->buildBody($this->getViewer(), $handles))
       ->addCancelButton($object_handle->getURI(), pht('Close'));
   }
 
   private function buildBody(PhabricatorUser $viewer, array $handles) {
 
     $list = id(new PHUIObjectItemListView())
-      ->setUser($viewer)
-      ->setFlush(true);
+      ->setUser($viewer);
     foreach ($handles as $handle) {
       $item = id(new PHUIObjectItemView())
         ->setHeader($handle->getFullName())
diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php
index a336e990a2..f8bc3fa060 100644
--- a/src/view/AphrontDialogView.php
+++ b/src/view/AphrontDialogView.php
@@ -23,6 +23,7 @@ final class AphrontDialogView
   private $errors = array();
   private $flush;
   private $validationException;
+  private $objectList;
 
 
   const WIDTH_DEFAULT = 'default';
@@ -132,6 +133,13 @@ final class AphrontDialogView
     return $this;
   }
 
+  public function setObjectList(PHUIObjectItemListView $list) {
+    $this->objectList = true;
+    $box = id(new PHUIObjectBoxView())
+      ->setObjectList($list);
+    return $this->appendChild($box);
+  }
+
   public function appendParagraph($paragraph) {
     return $this->appendChild(
       phutil_tag(
@@ -236,15 +244,17 @@ final class AphrontDialogView
           __CLASS__));
     }
 
-    $more = $this->class;
+    $classes = array();
+    $classes[] = 'aphront-dialog-view';
+    $classes[] = $this->class;
     if ($this->flush) {
-      $more .= ' aphront-dialog-flush';
+      $classes[] = 'aphront-dialog-flush';
     }
 
     switch ($this->width) {
       case self::WIDTH_FORM:
       case self::WIDTH_FULL:
-        $more .= ' aphront-dialog-view-width-'.$this->width;
+        $classes[] = 'aphront-dialog-view-width-'.$this->width;
         break;
       case self::WIDTH_DEFAULT:
         break;
@@ -256,11 +266,15 @@ final class AphrontDialogView
     }
 
     if ($this->isStandalone) {
-      $more .= ' aphront-dialog-view-standalone';
+      $classes[] = 'aphront-dialog-view-standalone';
+    }
+
+    if ($this->objectList) {
+      $classes[] = 'aphront-dialog-object-list';
     }
 
     $attributes = array(
-      'class'   => 'aphront-dialog-view '.$more,
+      'class'   => implode(' ', $classes),
       'sigil'   => 'jx-dialog',
     );
 
diff --git a/webroot/rsrc/css/aphront/dialog-view.css b/webroot/rsrc/css/aphront/dialog-view.css
index 88da74e8e1..3043400357 100644
--- a/webroot/rsrc/css/aphront/dialog-view.css
+++ b/webroot/rsrc/css/aphront/dialog-view.css
@@ -128,3 +128,13 @@
   border: 0;
   border-top: 1px solid {$thinblueborder};
 }
+
+.aphront-dialog-object-list .phui-object-box {
+  border: none;
+  padding: 0;
+  margin: 0;
+}
+
+.aphront-dialog-object-list .aphront-dialog-body {
+  padding: 0 12px;
+}

From d92f7a14739ad8e21fffd2afa33e95626a4d53f9 Mon Sep 17 00:00:00 2001
From: Chad Little <chad@chadsdomain.com>
Date: Tue, 27 Oct 2015 14:02:00 -0700
Subject: [PATCH 28/39] Add CSS to wrap long tables in a scrollable area

Summary: Fixes T9609, applies style to a wrapping div for long tables in Remarkup

Test Plan: Build a long table in Phriction, get scrollbar.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9609

Differential Revision: https://secure.phabricator.com/D14355
---
 resources/celerity/map.php         | 6 +++---
 webroot/rsrc/css/core/remarkup.css | 4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 00498fa482..8747ad2a66 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => 'd25d16e4',
+    'core.pkg.css' => 'b394b5ce',
     'core.pkg.js' => '47dc9ebb',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '2de124c9',
@@ -104,7 +104,7 @@ return array(
     'rsrc/css/application/tokens/tokens.css' => '3d0f239e',
     'rsrc/css/application/uiexample/example.css' => '528b19de',
     'rsrc/css/core/core.css' => 'a76cefc9',
-    'rsrc/css/core/remarkup.css' => 'fa3a8225',
+    'rsrc/css/core/remarkup.css' => '8d341238',
     'rsrc/css/core/syntax.css' => '9fd11da8',
     'rsrc/css/core/z-index.css' => '57ddcaa2',
     'rsrc/css/diviner/diviner-shared.css' => '5a337049',
@@ -735,7 +735,7 @@ return array(
     'phabricator-object-selector-css' => '85ee8ce6',
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => '6920d200',
-    'phabricator-remarkup-css' => 'fa3a8225',
+    'phabricator-remarkup-css' => '8d341238',
     'phabricator-search-results-css' => '7dea472c',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-side-menu-view-css' => 'bec2458e',
diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css
index 5c6cfd8c89..3f4516f135 100644
--- a/webroot/rsrc/css/core/remarkup.css
+++ b/webroot/rsrc/css/core/remarkup.css
@@ -334,6 +334,10 @@
   max-width: 100%;
 }
 
+.phabricator-remarkup .remarkup-table-wrap {
+  overflow-x: auto;
+}
+
 .phabricator-remarkup table.remarkup-table {
   border-collapse: separate;
   border-spacing: 1px;

From 526aa48f8b481fd7dc4efc1e253f8d4ed0598b9d Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Wed, 28 Oct 2015 11:15:18 -0700
Subject: [PATCH 29/39] Widen PHUIPropertyListView when ActionList isn't
 attached

Summary: This makes PHUIPropertyList display wider when an ActionList isn't present.

Test Plan: Review Diff Details in a Diff. Test mobile and desktop layouts.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13568
---
 resources/celerity/map.php                        |  6 +++---
 src/view/phui/PHUIPropertyListView.php            |  1 +
 webroot/rsrc/css/phui/phui-property-list-view.css | 14 +++++++++++---
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 8747ad2a66..fe97bdc236 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => 'b394b5ce',
+    'core.pkg.css' => '15e557bc',
     'core.pkg.js' => '47dc9ebb',
     'darkconsole.pkg.js' => 'e7393ebb',
     'differential.pkg.css' => '2de124c9',
@@ -141,7 +141,7 @@ return array(
     'rsrc/css/phui/phui-object-item-list-view.css' => '26c30d3f',
     'rsrc/css/phui/phui-pager.css' => 'bea33d23',
     'rsrc/css/phui/phui-pinboard-view.css' => '2495140e',
-    'rsrc/css/phui/phui-property-list-view.css' => '03904f6b',
+    'rsrc/css/phui/phui-property-list-view.css' => '27b2849e',
     'rsrc/css/phui/phui-remarkup-preview.css' => '867f85b3',
     'rsrc/css/phui/phui-spacing.css' => '042804d6',
     'rsrc/css/phui/phui-status.css' => '888cedb8',
@@ -792,7 +792,7 @@ return array(
     'phui-object-item-list-view-css' => '26c30d3f',
     'phui-pager-css' => 'bea33d23',
     'phui-pinboard-view-css' => '2495140e',
-    'phui-property-list-view-css' => '03904f6b',
+    'phui-property-list-view-css' => '27b2849e',
     'phui-remarkup-preview-css' => '867f85b3',
     'phui-spacing-css' => '042804d6',
     'phui-status-list-view-css' => '888cedb8',
diff --git a/src/view/phui/PHUIPropertyListView.php b/src/view/phui/PHUIPropertyListView.php
index 8a44ecd941..b505bde09f 100644
--- a/src/view/phui/PHUIPropertyListView.php
+++ b/src/view/phui/PHUIPropertyListView.php
@@ -127,6 +127,7 @@ final class PHUIPropertyListView extends AphrontView {
     // If we have an action list, make sure we render a property part, even
     // if there are no properties. Otherwise, the action list won't render.
     if ($this->actionList) {
+      $this->classes[] = 'phui-property-list-has-actions';
       $have_property_part = false;
       foreach ($this->parts as $part) {
         if ($part['type'] == 'property') {
diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css
index 803d572421..3558bf7a9e 100644
--- a/webroot/rsrc/css/phui/phui-property-list-view.css
+++ b/webroot/rsrc/css/phui/phui-property-list-view.css
@@ -43,7 +43,7 @@
 }
 
 .device-desktop .phui-property-list-key {
-  width: 18%;
+  width: 12%;
   margin-left: 1%;
   text-align: right;
   float: left;
@@ -51,6 +51,10 @@
   margin-bottom: 4px;
 }
 
+.device-desktop .phui-property-list-has-actions .phui-property-list-key {
+  width: 18%;
+}
+
 .phui-property-list-properties-wrap.phui-property-list-stacked {
   width: auto;
   float: none;
@@ -71,12 +75,16 @@
 }
 
 .device-desktop .phui-property-list-value {
-  width: 78%;
+  width: 84%;
   margin-left: 1%;
   float: left;
   margin-bottom: 4px;
 }
 
+.device-desktop .phui-property-list-has-actions .phui-property-list-value {
+  width: 78%;
+}
+
 .device .phui-property-list-value,
 .phui-property-list-stacked .phui-property-list-properties
   .phui-property-list-value {
@@ -158,7 +166,7 @@
   margin: 2px 0;
 }
 
-.phui-property-list-properties-wrap {
+.phui-property-list-has-actions .phui-property-list-properties-wrap {
   float: left;
   width: 78%;
 }

From 724f6ddda58d27edcee3a755f238179ebf0a2d7b Mon Sep 17 00:00:00 2001
From: Aviv Eyal <avivey@gmail.com>
Date: Wed, 28 Oct 2015 23:25:41 +0000
Subject: [PATCH 30/39] return this in DiffusionCommitQuery

Test Plan: chain another call after this

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D14364
---
 src/applications/diffusion/query/DiffusionCommitQuery.php | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 5d6b2623b5..e8f499955a 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -72,6 +72,7 @@ final class DiffusionCommitQuery
    */
   public function withRepositoryPHIDs(array $phids) {
     $this->repositoryPHIDs = $phids;
+    return $this;
   }
 
   /**

From 1b8337871b8894a6265d44af529d776d9005d7f1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2015 15:54:10 +0000
Subject: [PATCH 31/39] Correct the handle URI for build steps

Summary: Fixes T9674. This was wrong to start with (URI is `/edit/X/`, not `/X/edit/`) but we have a new view page anyway.

Test Plan:
  - Visited an exmaple URI in my browser.
  - Followed a build step link from "Authorized By: ..." in Drydock.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T9674

Differential Revision: https://secure.phabricator.com/D14366
---
 .../harbormaster/phid/HarbormasterBuildStepPHIDType.php         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php
index 92e1980d8c..c3427fe46f 100644
--- a/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php
+++ b/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php
@@ -34,7 +34,7 @@ final class HarbormasterBuildStepPHIDType extends PhabricatorPHIDType {
       $handle
         ->setName($name)
         ->setFullName(pht('Build Step %d: %s', $id, $name))
-        ->setURI("/harbormaster/step/{$id}/edit/");
+        ->setURI("/harbormaster/step/view/{$id}/");
     }
   }
 

From 2c3dbc48ee9502f93e28beccd779b218f06579b6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2015 16:00:53 +0000
Subject: [PATCH 32/39] Move "Next Step" to a custom field in Differential

Summary:
Fixes T9672. This was never turned into a custom field, for no particular reason. Convert it into one.

This is substantially similar to the existing "Apply Patch" field, which does the same thing (only shows a command).

We might rethink or remove this eventually (e.g., in a post-"Land Revision" world) but this makes it easier, at the very least.

Test Plan:
  - Viewed a non-accepted revision (no hint).
  - Viewed an accepted revision from a raw diff source (no hint).
  - Viewed an accepted revision from Git (`arc land` hint).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9672

Differential Revision: https://secure.phabricator.com/D14367
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorDifferentialConfigOptions.php  |  2 +
 .../customfield/DifferentialNextStepField.php | 65 +++++++++++++++++++
 .../view/DifferentialRevisionDetailView.php   | 30 ---------
 4 files changed, 69 insertions(+), 30 deletions(-)
 create mode 100644 src/applications/differential/customfield/DifferentialNextStepField.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index df013da0bf..81b8d553a2 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -416,6 +416,7 @@ phutil_register_library_map(array(
     'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php',
     'DifferentialManiphestTasksField' => 'applications/differential/customfield/DifferentialManiphestTasksField.php',
     'DifferentialModernHunk' => 'applications/differential/storage/DifferentialModernHunk.php',
+    'DifferentialNextStepField' => 'applications/differential/customfield/DifferentialNextStepField.php',
     'DifferentialParseCacheGarbageCollector' => 'applications/differential/garbagecollector/DifferentialParseCacheGarbageCollector.php',
     'DifferentialParseCommitMessageConduitAPIMethod' => 'applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php',
     'DifferentialParseRenderTestCase' => 'applications/differential/__tests__/DifferentialParseRenderTestCase.php',
@@ -4160,6 +4161,7 @@ phutil_register_library_map(array(
     'DifferentialLocalCommitsView' => 'AphrontView',
     'DifferentialManiphestTasksField' => 'DifferentialCoreCustomField',
     'DifferentialModernHunk' => 'DifferentialHunk',
+    'DifferentialNextStepField' => 'DifferentialCustomField',
     'DifferentialParseCacheGarbageCollector' => 'PhabricatorGarbageCollector',
     'DifferentialParseCommitMessageConduitAPIMethod' => 'DifferentialConduitAPIMethod',
     'DifferentialParseRenderTestCase' => 'PhabricatorTestCase',
diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
index a2bb63a2a2..dd864d523f 100644
--- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
+++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
@@ -25,6 +25,8 @@ final class PhabricatorDifferentialConfigOptions
     $custom_field_type = 'custom:PhabricatorCustomFieldConfigOptionType';
 
     $fields = array(
+      new DifferentialNextStepField(),
+
       new DifferentialTitleField(),
       new DifferentialSummaryField(),
       new DifferentialTestPlanField(),
diff --git a/src/applications/differential/customfield/DifferentialNextStepField.php b/src/applications/differential/customfield/DifferentialNextStepField.php
new file mode 100644
index 0000000000..bf0ed5972d
--- /dev/null
+++ b/src/applications/differential/customfield/DifferentialNextStepField.php
@@ -0,0 +1,65 @@
+<?php
+
+final class DifferentialNextStepField
+  extends DifferentialCustomField {
+
+  public function getFieldKey() {
+    return 'differential:next-step';
+  }
+
+  public function getFieldName() {
+    return pht('Next Step');
+  }
+
+  public function getFieldDescription() {
+    return pht('Provides a hint for the next step to take.');
+  }
+
+  public function shouldAppearInPropertyView() {
+    return true;
+  }
+
+  public function renderPropertyViewLabel() {
+    return $this->getFieldName();
+  }
+
+  public function renderPropertyViewValue(array $handles) {
+    $revision = $this->getObject();
+    $diff = $revision->getActiveDiff();
+
+    $status = $revision->getStatus();
+    if ($status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
+      return null;
+    }
+
+    $local_vcs = $diff->getSourceControlSystem();
+    switch ($local_vcs) {
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+        $bookmark = $diff->getBookmark();
+        if (strlen($bookmark)) {
+          $next_step = csprintf('arc land %R', $bookmark);
+        } else {
+          $next_step = csprintf('arc land');
+        }
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+        $branch = $diff->getBranch();
+        if (strlen($branch)) {
+          $next_step = csprintf('arc land %R', $branch);
+        } else {
+          $next_step = csprintf('arc land');
+        }
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        $next_step = csprintf('arc commit');
+        break;
+      default:
+        return null;
+    }
+
+    $next_step = phutil_tag('tt', array(), (string)$next_step);
+
+    return $next_step;
+  }
+
+}
diff --git a/src/applications/differential/view/DifferentialRevisionDetailView.php b/src/applications/differential/view/DifferentialRevisionDetailView.php
index cf4222dbdb..493691d21a 100644
--- a/src/applications/differential/view/DifferentialRevisionDetailView.php
+++ b/src/applications/differential/view/DifferentialRevisionDetailView.php
@@ -73,36 +73,6 @@ final class DifferentialRevisionDetailView extends AphrontView {
       ->setUser($user)
       ->setObject($revision);
 
-    $status = $revision->getStatus();
-    $local_vcs = $this->getDiff()->getSourceControlSystem();
-
-    $next_step = null;
-    if ($status == ArcanistDifferentialRevisionStatus::ACCEPTED) {
-      switch ($local_vcs) {
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-          $bookmark = $this->getDiff()->getBookmark();
-          $next_step = ($bookmark != ''
-            ? csprintf('arc land %s', $bookmark)
-            : 'arc land');
-          break;
-
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-          $branch = $this->getDiff()->getBranch();
-          $next_step = ($branch != ''
-            ? csprintf('arc land %s', $branch)
-            : 'arc land');
-          break;
-
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-          $next_step = 'arc commit';
-          break;
-      }
-    }
-    if ($next_step) {
-      $next_step = phutil_tag('tt', array(), $next_step);
-      $properties->addProperty(pht('Next Step'), $next_step);
-    }
-
     $properties->setHasKeyboardShortcuts(true);
     $properties->setActionList($actions);
     $this->setActionList($actions);

From 096117aacded31f1dec459c189211a3d1ee75dcb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2015 16:02:19 +0000
Subject: [PATCH 33/39] Allow any {icon} to spin

Summary: We are greedily hoarding this for ourselves, when we could enrich the world.

Test Plan: Used `{icon cog spin}`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14369
---
 .../macro/markup/PhabricatorIconRemarkupRule.php      | 11 ++++++++++-
 src/docs/user/userguide/remarkup.diviner              |  6 ++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/applications/macro/markup/PhabricatorIconRemarkupRule.php b/src/applications/macro/markup/PhabricatorIconRemarkupRule.php
index ce1212b0fc..acd87d4136 100644
--- a/src/applications/macro/markup/PhabricatorIconRemarkupRule.php
+++ b/src/applications/macro/markup/PhabricatorIconRemarkupRule.php
@@ -41,6 +41,7 @@ final class PhabricatorIconRemarkupRule extends PhutilRemarkupRule {
 
     $defaults = array(
       'color' => null,
+      'spin' => false,
     );
 
     $options = idx($extra, 1, '');
@@ -70,8 +71,16 @@ final class PhabricatorIconRemarkupRule extends PhutilRemarkupRule {
       $color = null;
     }
 
+    $classes = array();
+    $classes[] = $color;
+
+    $spin = $options['spin'];
+    if ($spin) {
+      $classes[] = 'ph-spin';
+    }
+
     $icon_view = id(new PHUIIconView())
-      ->setIconFont('fa-'.$icon, $color);
+      ->setIconFont('fa-'.$icon, implode(' ', $classes));
 
     return $this->getEngine()->storeText($icon_view);
   }
diff --git a/src/docs/user/userguide/remarkup.diviner b/src/docs/user/userguide/remarkup.diviner
index ef38552489..ca20751c47 100644
--- a/src/docs/user/userguide/remarkup.diviner
+++ b/src/docs/user/userguide/remarkup.diviner
@@ -496,6 +496,12 @@ For a list of available icons and colors, check the UIExamples application.
 [[ http://fortawesome.github.io/Font-Awesome/ | FontAwesome ]], so you can also
 browse the collection there.)
 
+You can add `spin` to make the icon spin:
+
+  {icon cog spin}
+
+This renders: {icon cog spin}
+
 
 = Phriction Documents =
 

From f48a8337048d617d840aba1a08fe9f845bc8751a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2015 16:02:35 +0000
Subject: [PATCH 34/39] Fix an issue with incorrect authorization handling in
 Working Copy build steps

Summary:
Fixes T9669. Two issues:

  - We were using `repositoryPHIDs` instead of `blueprintPHIDs` for the list of allowed blueprints. Use the correct value.
  - We weren't enforcing `allowedBlueprintPHIDs` fully correctly. We //did// require an authorization, so the net effect was correct in nearly all cases, but we could have selected from too large a pool in the case where the application itself was doing the authorization (e.g., from the command line).

Test Plan: Ran a build through Drydock/Harbormaster locally.

Reviewers: chad, tycho.tatitscheff

Reviewed By: chad, tycho.tatitscheff

Subscribers: tycho.tatitscheff

Maniphest Tasks: T9669

Differential Revision: https://secure.phabricator.com/D14368
---
 .../drydock/worker/DrydockLeaseUpdateWorker.php       | 11 ++++++-----
 ...rmasterLeaseWorkingCopyBuildStepImplementation.php |  5 ++++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
index 0c93647b89..8f34a88342 100644
--- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
+++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
@@ -309,17 +309,18 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker {
       return array();
     }
 
-    $query = id(new DrydockBlueprintQuery())
-      ->setViewer($viewer)
-      ->withBlueprintClasses(array_keys($impls))
-      ->withDisabled(false);
-
     $blueprint_phids = $lease->getAllowedBlueprintPHIDs();
     if (!$blueprint_phids) {
       $lease->logEvent(DrydockLeaseNoBlueprintsLogType::LOGCONST);
       return array();
     }
 
+    $query = id(new DrydockBlueprintQuery())
+      ->setViewer($viewer)
+      ->withPHIDs($blueprint_phids)
+      ->withBlueprintClasses(array_keys($impls))
+      ->withDisabled(false);
+
     // The Drydock application itself is allowed to authorize anything. This
     // is primarily used for leases generated by CLI administrative tools.
     $drydock_phid = id(new PhabricatorDrydockApplication())->getPHID();
diff --git a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
index 3c4d18f4b4..82c041f423 100644
--- a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
+++ b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
@@ -41,7 +41,10 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
       $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation())
         ->getType();
 
-      $allowed_phids = $build_target->getFieldValue('repositoryPHIDs');
+      $allowed_phids = $build_target->getFieldValue('blueprintPHIDs');
+      if (!is_array($allowed_phids)) {
+        $allowed_phids = array();
+      }
       $authorizing_phid = $build_target->getBuildStep()->getPHID();
 
       $lease = DrydockLease::initializeNewLease()

From 4d13b6c6a8c07ee4e5c7469cbb9e76836a205eeb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2015 12:06:47 -0700
Subject: [PATCH 35/39] Add a setup warning for major clock skew issues

Summary: See IRC. A user had a database set to 8 hours ahead of their web host. Try to catch and warn about these issues.

Test Plan: Artificially adjusted skew, saw setup warning.

Reviewers: avivey, chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14371
---
 .../check/PhabricatorMySQLSetupCheck.php      | 26 ++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php
index 75ecc06239..cc792358ad 100644
--- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php
+++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php
@@ -319,9 +319,11 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck {
         ->addMySQLConfig('innodb_buffer_pool_size');
     }
 
+    $conn_w = id(new PhabricatorUser())->establishConnection('w');
+
     $ok = PhabricatorStorageManagementAPI::isCharacterSetAvailableOnConnection(
       'utf8mb4',
-      id(new PhabricatorUser())->establishConnection('w'));
+      $conn_w);
     if (!$ok) {
       $summary = pht(
         'You are using an old version of MySQL, and should upgrade.');
@@ -339,6 +341,28 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck {
         ->setMessage($message);
     }
 
+    $info = queryfx_one(
+      $conn_w,
+      'SELECT UNIX_TIMESTAMP() epoch');
+
+    $epoch = (int)$info['epoch'];
+    $local = PhabricatorTime::getNow();
+    $delta = (int)abs($local - $epoch);
+    if ($delta > 60) {
+      $this->newIssue('mysql.clock')
+        ->setName(pht('Major Web/Database Clock Skew'))
+        ->setSummary(
+          pht(
+            'This host is set to a very different time than the database.'))
+        ->setMessage(
+          pht(
+            'The database host and this host ("%s") disagree on the current '.
+            'time by more than 60 seconds (absolute skew is %s seconds). '.
+            'Check that the current time is set correctly everywhere.',
+            php_uname('n'),
+            new PhutilNumber($delta)));
+    }
+
   }
 
   protected function shouldUseMySQLSearchEngine() {

From 98a301a59b1df7eb5c0512a054a38a6f99d7c303 Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@freelancer.com>
Date: Sat, 31 Oct 2015 04:54:16 +0000
Subject: [PATCH 36/39] Set `$can_edit` for Harbormaster steps

Summary: Sets the `$can_edit` value correctly (previously it was hardcoded to `true`).

Test Plan: Went to http://phabricator.local/harbormaster/step/view/1/ and saw "Edit Step" disabled.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14373
---
 .../controller/HarbormasterStepViewController.php            | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/applications/harbormaster/controller/HarbormasterStepViewController.php b/src/applications/harbormaster/controller/HarbormasterStepViewController.php
index 1af7f2fe16..d8d1d3f0b0 100644
--- a/src/applications/harbormaster/controller/HarbormasterStepViewController.php
+++ b/src/applications/harbormaster/controller/HarbormasterStepViewController.php
@@ -97,7 +97,10 @@ final class HarbormasterStepViewController extends HarbormasterController {
       ->setUser($viewer)
       ->setObject($step);
 
-    $can_edit = true;
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $step,
+      PhabricatorPolicyCapability::CAN_EDIT);
 
     $list->addAction(
       id(new PhabricatorActionView())

From 4626fb4ef00f8da35493d480d8535032f76d9e76 Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@freelancer.com>
Date: Sat, 31 Oct 2015 11:20:23 +0000
Subject: [PATCH 37/39] Update "should not run as root" message

Summary: These should be fine to land whenever.

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14066
---
 .../management/PhabricatorAphlictManagementWorkflow.php      | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php b/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
index 7747a73866..659fb80edb 100644
--- a/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
+++ b/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
@@ -132,10 +132,7 @@ abstract class PhabricatorAphlictManagementWorkflow
 
     if (posix_getuid() == 0) {
       throw new PhutilArgumentUsageException(
-        pht(
-          // TODO: Update this message after a while.
-          'The notification server should not be run as root. It no '.
-          'longer requires access to privileged ports.'));
+        pht('The notification server should not be run as root.'));
     }
 
     // Make sure we can write to the PID file.

From 3a046384e97e86e37a8a1f78b2db47c6950cbfcf Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@freelancer.com>
Date: Sat, 31 Oct 2015 11:20:59 +0000
Subject: [PATCH 38/39] Drop the `metamta_mailinglist` table

Summary: We haven't seen any issues here, remove the table and schema spec.

Test Plan: Not yet tested.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14067
---
 .../sql/autopatches/20150906.mailinglist.sql  |  1 +
 .../storage/PhabricatorMetaMTAMailingList.php | 40 -------------------
 2 files changed, 1 insertion(+), 40 deletions(-)
 create mode 100644 resources/sql/autopatches/20150906.mailinglist.sql
 delete mode 100644 src/applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php

diff --git a/resources/sql/autopatches/20150906.mailinglist.sql b/resources/sql/autopatches/20150906.mailinglist.sql
new file mode 100644
index 0000000000..4f349eadd4
--- /dev/null
+++ b/resources/sql/autopatches/20150906.mailinglist.sql
@@ -0,0 +1 @@
+DROP TABLE {$NAMESPACE}_metamta.metamta_mailinglist;
diff --git a/src/applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php b/src/applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php
deleted file mode 100644
index 68d2499ed6..0000000000
--- a/src/applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php
+++ /dev/null
@@ -1,40 +0,0 @@
-<?php
-
-/**
- * TODO: This class is just here to keep `storage adjust` happy until we
- * destroy the table.
- */
-final class PhabricatorMetaMTAMailingList extends PhabricatorMetaMTADAO {
-
-  protected $name;
-  protected $email;
-  protected $uri;
-
-
-  protected function getConfiguration() {
-    return array(
-      self::CONFIG_AUX_PHID => true,
-      self::CONFIG_COLUMN_SCHEMA => array(
-        'name' => 'text128',
-        'email' => 'text128',
-        'uri' => 'text255?',
-      ),
-      self::CONFIG_KEY_SCHEMA => array(
-        'key_phid' => null,
-        'phid' => array(
-          'columns' => array('phid'),
-          'unique' => true,
-        ),
-        'email' => array(
-          'columns' => array('email'),
-          'unique' => true,
-        ),
-        'name' => array(
-          'columns' => array('name'),
-          'unique' => true,
-        ),
-      ),
-    ) + parent::getConfiguration();
-  }
-
-}

From 2c2d1d13e378a8d0ebab5bf1f507a9aea4f945f4 Mon Sep 17 00:00:00 2001
From: Aviv Eyal <avivey@gmail.com>
Date: Sat, 31 Oct 2015 00:13:13 -0700
Subject: [PATCH 39/39] arc liberate

---
 src/__phutil_library_map__.php | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 81b8d553a2..398d212913 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2410,7 +2410,6 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAMailableDatasource' => 'applications/metamta/typeahead/PhabricatorMetaMTAMailableDatasource.php',
     'PhabricatorMetaMTAMailableFunctionDatasource' => 'applications/metamta/typeahead/PhabricatorMetaMTAMailableFunctionDatasource.php',
     'PhabricatorMetaMTAMailgunReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php',
-    'PhabricatorMetaMTAMailingList' => 'applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php',
     'PhabricatorMetaMTAMemberQuery' => 'applications/metamta/query/PhabricatorMetaMTAMemberQuery.php',
     'PhabricatorMetaMTAPermanentFailureException' => 'applications/metamta/exception/PhabricatorMetaMTAPermanentFailureException.php',
     'PhabricatorMetaMTAReceivedMail' => 'applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php',
@@ -6470,7 +6469,6 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAMailableDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorMetaMTAMailableFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorMetaMTAMailgunReceiveController' => 'PhabricatorMetaMTAController',
-    'PhabricatorMetaMTAMailingList' => 'PhabricatorMetaMTADAO',
     'PhabricatorMetaMTAMemberQuery' => 'PhabricatorQuery',
     'PhabricatorMetaMTAPermanentFailureException' => 'Exception',
     'PhabricatorMetaMTAReceivedMail' => 'PhabricatorMetaMTADAO',