From 2ddd78647b20d615191da1f07313cb8b81f13216 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Mar 2016 08:46:11 -0800 Subject: [PATCH] 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 --- .../query/DifferentialDiffQuery.php | 54 +++++---- .../DrydockLandRepositoryOperation.php | 104 ++++++++++++++++++ .../storage/build/HarbormasterBuildLog.php | 2 + 3 files changed, 141 insertions(+), 19 deletions(-) diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index 1616d58ac8..23c016446c 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/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(); - $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); + return $this->loadStandardPage($this->newResultObject()); } 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) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $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 $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index 77de219115..4e306772a3 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/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), + ); + } + } + } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 40090d0ac3..290b288c6c 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/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() {