From 1edb875978d5a1d823c5a6e94e771f483229c4fb Mon Sep 17 00:00:00 2001 From: Neal Poole Date: Fri, 4 Oct 2013 06:37:32 -0700 Subject: [PATCH] Adding support for 'adds' and 'removes' in diff content. Summary: Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'. - There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant. - The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally. Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations. Reviewers: epriestley Reviewed By: epriestley CC: Korvin, aran Maniphest Tasks: T3829 Differential Revision: https://secure.phabricator.com/D7214 --- .../differential/storage/DifferentialHunk.php | 4 ++ .../herald/adapter/HeraldAdapter.php | 6 ++ .../herald/adapter/HeraldCommitAdapter.php | 46 ++++++++++++--- .../HeraldDifferentialRevisionAdapter.php | 56 +++++++++++++++++++ .../herald/storage/HeraldRule.php | 2 +- 5 files changed, 105 insertions(+), 9 deletions(-) diff --git a/src/applications/differential/storage/DifferentialHunk.php b/src/applications/differential/storage/DifferentialHunk.php index 77484cdc01..8efc8da499 100644 --- a/src/applications/differential/storage/DifferentialHunk.php +++ b/src/applications/differential/storage/DifferentialHunk.php @@ -13,6 +13,10 @@ final class DifferentialHunk extends DifferentialDAO { return $this->makeContent($include = '+'); } + public function getRemovedLines() { + return $this->makeContent($include = '-'); + } + public function makeNewFile() { return implode('', $this->makeContent($include = ' +')); } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index ad857424c4..76876ef728 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -15,6 +15,8 @@ abstract class HeraldAdapter { const FIELD_TAGS = 'tags'; const FIELD_DIFF_FILE = 'diff-file'; const FIELD_DIFF_CONTENT = 'diff-content'; + const FIELD_DIFF_ADDED_CONTENT = 'diff-added-content'; + const FIELD_DIFF_REMOVED_CONTENT = 'diff-removed-content'; const FIELD_REPOSITORY = 'repository'; const FIELD_RULE = 'rule'; const FIELD_AFFECTED_PACKAGE = 'affected-package'; @@ -127,6 +129,8 @@ abstract class HeraldAdapter { self::FIELD_TAGS => pht('Tags'), self::FIELD_DIFF_FILE => pht('Any changed filename'), self::FIELD_DIFF_CONTENT => pht('Any changed file content'), + self::FIELD_DIFF_ADDED_CONTENT => pht('Any added file content'), + self::FIELD_DIFF_REMOVED_CONTENT => pht('Any removed file content'), self::FIELD_REPOSITORY => pht('Repository'), self::FIELD_RULE => pht('Another Herald rule'), self::FIELD_AFFECTED_PACKAGE => pht('Any affected package'), @@ -197,6 +201,8 @@ abstract class HeraldAdapter { self::CONDITION_REGEXP, ); case self::FIELD_DIFF_CONTENT: + case self::FIELD_DIFF_ADDED_CONTENT: + case self::FIELD_DIFF_REMOVED_CONTENT: return array( self::CONDITION_CONTAINS, self::CONDITION_REGEXP, diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 94f6ae7fb8..a3ec223d6c 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -61,6 +61,8 @@ final class HeraldCommitAdapter extends HeraldAdapter { self::FIELD_REPOSITORY, self::FIELD_DIFF_FILE, self::FIELD_DIFF_CONTENT, + self::FIELD_DIFF_ADDED_CONTENT, + self::FIELD_DIFF_REMOVED_CONTENT, self::FIELD_RULE, self::FIELD_AFFECTED_PACKAGE, self::FIELD_AFFECTED_PACKAGE_OWNER, @@ -255,6 +257,17 @@ final class HeraldCommitAdapter extends HeraldAdapter { return $diff; } + private function loadChangesets() { + try { + $diff = $this->loadCommitDiff(); + } catch (Exception $ex) { + return array( + '<<< Failed to load diff, this may mean the change was '. + 'unimaginably enormous. >>>'); + } + return $diff->getChangesets(); + } + public function getHeraldField($field) { $data = $this->commitData; switch ($field) { @@ -271,16 +284,9 @@ final class HeraldCommitAdapter extends HeraldAdapter { case self::FIELD_REPOSITORY: return $this->repository->getPHID(); case self::FIELD_DIFF_CONTENT: - try { - $diff = $this->loadCommitDiff(); - } catch (Exception $ex) { - return array( - '<<< Failed to load diff, this may mean the change was '. - 'unimaginably enormous. >>>'); - } $dict = array(); $lines = array(); - $changes = $diff->getChangesets(); + $changes = $this->loadChangesets(); foreach ($changes as $change) { $lines = array(); foreach ($change->getHunks() as $hunk) { @@ -289,6 +295,30 @@ final class HeraldCommitAdapter extends HeraldAdapter { $dict[$change->getFilename()] = implode("\n", $lines); } return $dict; + case self::FIELD_DIFF_ADDED_CONTENT: + $dict = array(); + $lines = array(); + $changes = $this->loadChangesets(); + foreach ($changes as $change) { + $lines = array(); + foreach ($change->getHunks() as $hunk) { + $lines[] = implode('', $hunk->getAddedLines()); + } + $dict[$change->getFilename()] = implode("\n", $lines); + } + return $dict; + case self::FIELD_DIFF_REMOVED_CONTENT: + $dict = array(); + $lines = array(); + $changes = $this->loadChangesets(); + foreach ($changes as $change) { + $lines = array(); + foreach ($change->getHunks() as $hunk) { + $lines[] = implode('', $hunk->getRemovedLines()); + } + $dict[$change->getFilename()] = implode("\n", $lines); + } + return $dict; case self::FIELD_AFFECTED_PACKAGE: $packages = $this->loadAffectedPackages(); return mpull($packages, 'getPHID'); diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 76bcf26791..5559d8cdc5 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -44,6 +44,8 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::FIELD_REPOSITORY, self::FIELD_DIFF_FILE, self::FIELD_DIFF_CONTENT, + self::FIELD_DIFF_ADDED_CONTENT, + self::FIELD_DIFF_REMOVED_CONTENT, self::FIELD_RULE, self::FIELD_AFFECTED_PACKAGE, self::FIELD_AFFECTED_PACKAGE_OWNER, @@ -194,6 +196,56 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return $dict; } + protected function loadAddedContentDictionary() { + $changesets = $this->loadChangesets(); + + $hunks = array(); + if ($changesets) { + $hunks = id(new DifferentialHunk())->loadAllWhere( + 'changesetID in (%Ld)', + mpull($changesets, 'getID')); + } + + $dict = array(); + $hunks = mgroup($hunks, 'getChangesetID'); + $changesets = mpull($changesets, null, 'getID'); + foreach ($changesets as $id => $changeset) { + $path = $this->getAbsoluteRepositoryPathForChangeset($changeset); + $content = array(); + foreach (idx($hunks, $id, array()) as $hunk) { + $content[] = implode('', $hunk->getAddedLines()); + } + $dict[$path] = implode("\n", $content); + } + + return $dict; + } + + protected function loadRemovedContentDictionary() { + $changesets = $this->loadChangesets(); + + $hunks = array(); + if ($changesets) { + $hunks = id(new DifferentialHunk())->loadAllWhere( + 'changesetID in (%Ld)', + mpull($changesets, 'getID')); + } + + $dict = array(); + $hunks = mgroup($hunks, 'getChangesetID'); + $changesets = mpull($changesets, null, 'getID'); + foreach ($changesets as $id => $changeset) { + $path = $this->getAbsoluteRepositoryPathForChangeset($changeset); + $content = array(); + foreach (idx($hunks, $id, array()) as $hunk) { + $content[] = implode('', $hunk->getRemovedLines()); + } + $dict[$path] = implode("\n", $content); + } + + return $dict; + } + public function loadAffectedPackages() { if ($this->affectedPackages === null) { $this->affectedPackages = array(); @@ -243,6 +295,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return $repository->getPHID(); case self::FIELD_DIFF_CONTENT: return $this->loadContentDictionary(); + case self::FIELD_DIFF_ADDED_CONTENT: + return $this->loadAddedContentDictionary(); + case self::FIELD_DIFF_REMOVED_CONTENT: + return $this->loadRemovedContentDictionary(); case self::FIELD_AFFECTED_PACKAGE: $packages = $this->loadAffectedPackages(); return mpull($packages, 'getPHID'); diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 78f3908604..2f97d154c6 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -13,7 +13,7 @@ final class HeraldRule extends HeraldDAO protected $repetitionPolicy; protected $ruleType; - protected $configVersion = 11; + protected $configVersion = 12; private $ruleApplied = self::ATTACHABLE; // phids for which this rule has been applied private $validAuthor = self::ATTACHABLE;