From 2dfe79cfc7134940f0be1e97b74b02c4c0544080 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jan 2017 11:39:17 -0800 Subject: [PATCH] When updating revisions in response to commits, reuse previously generated diffs Summary: Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail. When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too. Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it. Test Plan: - Used `bin/differential attach-commit ` to hit this code. - Simulated a filesystem write failure, saw the diff get reused. - Also did a normal update, which worked properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10968 Differential Revision: https://secure.phabricator.com/D17164 --- .../autopatches/20170109.diff.01.commit.sql | 2 + .../DifferentialDiffExtractionEngine.php | 14 +++++++ .../query/DifferentialDiffQuery.php | 37 +++++++++++++++++-- .../differential/storage/DifferentialDiff.php | 5 +++ 4 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 resources/sql/autopatches/20170109.diff.01.commit.sql diff --git a/resources/sql/autopatches/20170109.diff.01.commit.sql b/resources/sql/autopatches/20170109.diff.01.commit.sql new file mode 100644 index 0000000000..2a28900272 --- /dev/null +++ b/resources/sql/autopatches/20170109.diff.01.commit.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_diff + ADD commitPHID VARBINARY(64); diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index 05e19bbe71..952c76c63f 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -26,6 +26,19 @@ final class DifferentialDiffExtractionEngine extends Phobject { public function newDiffFromCommit(PhabricatorRepositoryCommit $commit) { $viewer = $this->getViewer(); + // If we already have an unattached diff for this commit, just reuse it. + // This stops us from repeatedly generating diffs if something goes wrong + // later in the process. See T10968 for context. + $existing_diffs = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withCommitPHIDs(array($commit->getPHID())) + ->withHasRevision(false) + ->needChangesets(true) + ->execute(); + if ($existing_diffs) { + return head($existing_diffs); + } + $repository = $commit->getRepository(); $identifier = $commit->getCommitIdentifier(); $monogram = $commit->getMonogram(); @@ -73,6 +86,7 @@ final class DifferentialDiffExtractionEngine extends Phobject { $diff = DifferentialDiff::newFromRawChanges($viewer, $changes) ->setRepositoryPHID($repository->getPHID()) + ->setCommitPHID($commit->getPHID()) ->setCreationMethod('commit') ->setSourceControlSystem($repository->getVersionControlSystem()) ->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP) diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index 23c016446c..f779e40c26 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -6,6 +6,8 @@ final class DifferentialDiffQuery private $ids; private $phids; private $revisionIDs; + private $commitPHIDs; + private $hasRevision; private $needChangesets = false; private $needProperties; @@ -25,6 +27,16 @@ final class DifferentialDiffQuery return $this; } + public function withCommitPHIDs(array $phids) { + $this->commitPHIDs = $phids; + return $this; + } + + public function withHasRevision($has_revision) { + $this->hasRevision = $has_revision; + return $this; + } + public function needChangesets($bool) { $this->needChangesets = $bool; return $this; @@ -108,27 +120,46 @@ final class DifferentialDiffQuery protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { $where = parent::buildWhereClauseParts($conn); - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->revisionIDs) { + if ($this->revisionIDs !== null) { $where[] = qsprintf( $conn, 'revisionID IN (%Ld)', $this->revisionIDs); } + if ($this->commitPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'commitPHID IN (%Ls)', + $this->commitPHIDs); + } + + if ($this->hasRevision !== null) { + if ($this->hasRevision) { + $where[] = qsprintf( + $conn, + 'revisionID IS NOT NULL'); + } else { + $where[] = qsprintf( + $conn, + 'revisionID IS NULL'); + } + } + return $where; } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 891af35982..d8c2ea9786 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -13,6 +13,7 @@ final class DifferentialDiff protected $revisionID; protected $authorPHID; protected $repositoryPHID; + protected $commitPHID; protected $sourceMachine; protected $sourcePath; @@ -62,6 +63,7 @@ final class DifferentialDiff 'branch' => 'text255?', 'bookmark' => 'text255?', 'repositoryUUID' => 'text64?', + 'commitPHID' => 'phid?', // T6203/NULLABILITY // These should be non-null; all diffs should have a creation method @@ -73,6 +75,9 @@ final class DifferentialDiff 'revisionID' => array( 'columns' => array('revisionID'), ), + 'key_commit' => array( + 'columns' => array('commitPHID'), + ), ), ) + parent::getConfiguration(); }