Improve "Land Revision" errors for issues related to staging areas

Summary: Ref T10093. Changes must be pushed to staging before they can be landed from the web.

Test Plan:
Changes must be pushed to staging before they can be landed from the web.

{F1161909}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10093

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

src/applications/differential/query/DifferentialDiffQuery.php

@@ -6,7 +6,9 @@ final class DifferentialDiffQuery
   private $ids;
   private $phids;
   private $revisionIDs;
+
   private $needChangesets = false;
+  private $needProperties;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -28,19 +30,17 @@ final class DifferentialDiffQuery
     return $this;
   }
 
+  public function needProperties($need_properties) {
+    $this->needProperties = $need_properties;
+    return $this;
+  }
+
+  public function newResultObject() {
+    return new DifferentialDiff();
+  }
+
   protected function loadPage() {
-    $table = new DifferentialDiff();
+    return $this->loadStandardPage($this->newResultObject());
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
   }
 
   protected function willFilterPage(array $diffs) {
@@ -76,6 +76,23 @@ final class DifferentialDiffQuery
     return $diffs;
   }
 
+  protected function didFilterPage(array $diffs) {
+    if ($this->needProperties) {
+      $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+        'diffID IN (%Ld)',
+        mpull($diffs, 'getID'));
+
+      $properties = mgroup($properties, 'getDiffID');
+      foreach ($diffs as $diff) {
+        $map = idx($properties, $diff->getID(), array());
+        $map = mpull($map, 'getData', 'getName');
+        $diff->attachDiffProperties($map);
+      }
+    }
+
+    return $diffs;
+  }
+
   private function loadChangesets(array $diffs) {
     id(new DifferentialChangesetQuery())
       ->setViewer($this->getViewer())
@@ -88,32 +105,31 @@ final class DifferentialDiffQuery
     return $diffs;
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
-    $where = array();
+    $where = parent::buildWhereClauseParts($conn);
 
     if ($this->ids) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'id IN (%Ld)',
         $this->ids);
     }
 
     if ($this->phids) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'phid IN (%Ls)',
         $this->phids);
     }
 
     if ($this->revisionIDs) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'revisionID IN (%Ld)',
         $this->revisionIDs);
     }
 
-    $where[] = $this->buildPagingClause($conn_r);
+    return $where;
-    return $this->formatWhereClause($where);
   }
 
   public function getQueryApplicationClass() {

src/applications/drydock/operation/DrydockLandRepositoryOperation.php

@@ -248,6 +248,29 @@ final class DrydockLandRepositoryOperation
       );
     }
 
+    // Check if this diff was pushed to a staging area.
+    $diff = id(new DifferentialDiffQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($revision->getActiveDiff()->getID()))
+      ->needProperties(true)
+      ->executeOne();
+
+    // Older diffs won't have this property. They may still have been pushed.
+    // At least for now, assume staging changes are present if the property
+    // is missing. This should smooth the transition to the more formal
+    // approach.
+    $has_staging = $diff->hasDiffProperty('arc.staging');
+    if ($has_staging) {
+      $staging = $diff->getProperty('arc.staging');
+      if (!is_array($staging)) {
+        $staging = array();
+      }
+      $status = idx($staging, 'status');
+      if ($status != ArcanistDiffWorkflow::STAGING_PUSHED) {
+        return $this->getBarrierToLandingFromStagingStatus($status);
+      }
+    }
+
     // 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
@@ -336,4 +359,85 @@ final class DrydockLandRepositoryOperation
     return null;
   }
 
+  private function getBarrierToLandingFromStagingStatus($status) {
+    switch ($status) {
+      case ArcanistDiffWorkflow::STAGING_USER_SKIP:
+        return array(
+          'title' => pht('Staging Area Skipped'),
+          'body' => pht(
+            'The diff author used the %s flag to skip pushing this change to '.
+            'staging. Changes must be pushed to staging before they can be '.
+            'landed from the web.',
+            phutil_tag('tt', array(), '--skip-staging')),
+        );
+      case ArcanistDiffWorkflow::STAGING_DIFF_RAW:
+        return array(
+          'title' => pht('Raw Diff Source'),
+          'body' => pht(
+            'The diff was generated from a raw input source, so the change '.
+            'could not be pushed to staging. Changes must be pushed to '.
+            'staging before they can be landed from the web.'),
+        );
+      case ArcanistDiffWorkflow::STAGING_REPOSITORY_UNKNOWN:
+        return array(
+          'title' => pht('Unknown Repository'),
+          'body' => pht(
+            'When the diff was generated, the client was not able to '.
+            'determine which repository it belonged to, so the change '.
+            'was not pushed to staging. Changes must be pushed to staging '.
+            'before they can be landed from the web.'),
+        );
+      case ArcanistDiffWorkflow::STAGING_REPOSITORY_UNAVAILABLE:
+        return array(
+          'title' => pht('Staging Unavailable'),
+          'body' => pht(
+            'When this diff was generated, the server was running an older '.
+            'version of Phabricator which did not support staging areas, so '.
+            'the change was not pushed to staging. Changes must be pushed '.
+            'to staging before they can be landed from the web.'),
+        );
+      case ArcanistDiffWorkflow::STAGING_REPOSITORY_UNSUPPORTED:
+        return array(
+          'title' => pht('Repository Unsupported'),
+          'body' => pht(
+            'When this diff was generated, the server was running an older '.
+            'version of Phabricator which did not support staging areas for '.
+            'this version control system, so the chagne was not pushed to '.
+            'staging. Changes must be pushed to staging before they can be '.
+            'landed from the web.'),
+        );
+
+      case ArcanistDiffWorkflow::STAGING_REPOSITORY_UNCONFIGURED:
+        return array(
+          'title' => pht('Repository Unconfigured'),
+          'body' => pht(
+            'When this diff was generated, the repository was not configured '.
+            'with a staging area, so the change was not pushed to staging. '.
+            'Changes must be pushed to staging before they can be landed '.
+            'from the web.'),
+        );
+      case ArcanistDiffWorkflow::STAGING_CLIENT_UNSUPPORTED:
+        return array(
+          'title' => pht('Client Support Unavailable'),
+          'body' => pht(
+            'When this diff was generated, the client did not support '.
+            'staging areas for this version control system, so the change '.
+            'was not pushed to staging. Changes must be pushed to staging '.
+            'before they can be landed from the web. Updating the client '.
+            'may resolve this issue.'),
+        );
+      default:
+        return array(
+          'title' => pht('Unknown Error'),
+          'body' => pht(
+            'When this diff was generated, it was not pushed to staging for '.
+            'an unknown reason (the status code was "%s"). Changes must be '.
+            'pushed to staging before they can be landed from the web. '.
+            'The server may be running an out-of-date version of Phabricator, '.
+            'and updating may provide more information about this error.',
+            $status),
+        );
+    }
+  }
+
 }

src/applications/harbormaster/storage/build/HarbormasterBuildLog.php

@@ -114,6 +114,8 @@ final class HarbormasterBuildLog
 
     $this->rope->append($content);
     $this->flush();
+
+    return $this;
   }
 
   private function flush() {