From af6238ca4ae01024f3817f6b11b025c496cf01db Mon Sep 17 00:00:00 2001 From: vrana Date: Tue, 22 May 2012 16:09:49 -0700 Subject: [PATCH] Inform about changes made between last revision and commit Summary: This adds a link to [Closed] e-mail if it detects some changes. It compares added and removed lines with 3 lines context. The subtle form of informing is permissive to false negatives and positives. I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit. Test Plan: Reparse a diff with a change after last update. Reparse a diff without a change after last update. Reviewers: jungejason, epriestley Reviewed By: jungejason CC: aran, Koolvin, btrahan Maniphest Tasks: T201 Differential Revision: https://secure.phabricator.com/D2540 --- .../DifferentialRevisionViewController.php | 40 +++---- .../mail/comment/DifferentialCommentMail.php | 2 +- .../changeset/DifferentialChangeset.php | 21 +++- .../DifferentialDiffTableOfContentsView.php | 5 - .../base/DiffusionFileContentQuery.php | 2 +- ...torRepositoryCommitMessageParserWorker.php | 109 +++++++++++++++++- .../commitmessageparser/base/__init__.php | 5 + 7 files changed, 149 insertions(+), 35 deletions(-) diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index bd56a473d0..9e717afebd 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -50,13 +50,8 @@ final class DifferentialRevisionViewController extends DifferentialController { $diff_vs = $request->getInt('vs'); - $target = end($diffs); $target_id = $request->getInt('id'); - if ($target_id) { - if (isset($diffs[$target_id])) { - $target = $diffs[$target_id]; - } - } + $target = idx($diffs, $target_id, end($diffs)); $target_manual = $target; if (!$target_id) { @@ -67,7 +62,6 @@ final class DifferentialRevisionViewController extends DifferentialController { } } - $diffs = mpull($diffs, null, 'getID'); if (empty($diffs[$diff_vs])) { $diff_vs = null; } @@ -80,8 +74,14 @@ final class DifferentialRevisionViewController extends DifferentialController { 'arc:unit', )); + $arc_project = $target->loadArcanistProject(); + $repository = ($arc_project ? $arc_project->loadRepository() : null); + list($changesets, $vs_map, $rendering_references) = - $this->loadChangesetsAndVsMap($diffs, $diff_vs, $target); + $this->loadChangesetsAndVsMap( + $target, + idx($diffs, $diff_vs), + $repository); $comments = $revision->loadComments(); $comments = array_merge( @@ -221,16 +221,12 @@ final class DifferentialRevisionViewController extends DifferentialController { 'whitespace', DifferentialChangesetParser::WHITESPACE_IGNORE_ALL); - $arc_project = $target_manual->loadArcanistProject(); - if ($arc_project) { $symbol_indexes = $this->buildSymbolIndexes( $arc_project, $visible_changesets); - $repository = $arc_project->loadRepository(); } else { $symbol_indexes = array(); - $repository = null; } $revision_detail->setActions($actions); @@ -304,7 +300,6 @@ final class DifferentialRevisionViewController extends DifferentialController { } $toc_view->setDiff($target); $toc_view->setUser($user); - $toc_view->setVsMap($vs_map); $toc_view->setRevisionID($revision->getID()); $toc_view->setWhitespace($whitespace); @@ -601,14 +596,13 @@ final class DifferentialRevisionViewController extends DifferentialController { } private function loadChangesetsAndVsMap( - array $diffs, - $diff_vs, - DifferentialDiff $target) { - assert_instances_of($diffs, 'DifferentialDiff'); + DifferentialDiff $target, + DifferentialDiff $diff_vs = null, + PhabricatorRepository $repository = null) { $load_ids = array(); if ($diff_vs) { - $load_ids[] = $diff_vs; + $load_ids[] = $diff_vs->getID(); } $load_ids[] = $target->getID(); @@ -628,10 +622,14 @@ final class DifferentialRevisionViewController extends DifferentialController { $vs_map = array(); if ($diff_vs) { - $vs_changesets = idx($changeset_groups, $diff_vs, array()); - $vs_changesets = mpull($vs_changesets, null, 'getFilename'); + $vs_changesets = array(); + $vs_id = $diff_vs->getID(); + foreach (idx($changeset_groups, $vs_id, array()) as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $diff_vs); + $vs_changesets[$path] = $changeset; + } foreach ($changesets as $key => $changeset) { - $file = $changeset->getFilename(); + $file = $changeset->getAbsoluteRepositoryPath($repository, $target); if (isset($vs_changesets[$file])) { $vs_map[$changeset->getID()] = $vs_changesets[$file]->getID(); $refs[$changeset->getID()] = diff --git a/src/applications/differential/mail/comment/DifferentialCommentMail.php b/src/applications/differential/mail/comment/DifferentialCommentMail.php index 0164c06d56..478e7b635c 100644 --- a/src/applications/differential/mail/comment/DifferentialCommentMail.php +++ b/src/applications/differential/mail/comment/DifferentialCommentMail.php @@ -134,7 +134,7 @@ final class DifferentialCommentMail extends DifferentialMail { if ($this->getChangedByCommit()) { $body[] = 'CHANGED PRIOR TO COMMIT'; - $body[] = ' This revision was updated prior to commit.'; + $body[] = ' '.$this->getChangedByCommit(); $body[] = null; } diff --git a/src/applications/differential/storage/changeset/DifferentialChangeset.php b/src/applications/differential/storage/changeset/DifferentialChangeset.php index 395c81d692..9175ca48b1 100644 --- a/src/applications/differential/storage/changeset/DifferentialChangeset.php +++ b/src/applications/differential/storage/changeset/DifferentialChangeset.php @@ -135,12 +135,27 @@ final class DifferentialChangeset extends DifferentialDAO { return implode("\n", $file); } + public function makeChangesWithContext($num_lines = 3) { + $with_context = array(); + foreach ($this->getHunks() as $hunk) { + $context = array(); + $changes = explode("\n", $hunk->getChanges()); + foreach ($changes as $l => $line) { + if ($line[0] == '+' || $line[0] == '-') { + $context += array_fill($l - $num_lines, $l + $num_lines, true); + } + } + $with_context[] = array_intersect_key($changes, $context); + } + return call_user_func('array_merge', $with_context); + } + public function getAnchorName() { return substr(md5($this->getFilename()), 0, 8); } public function getAbsoluteRepositoryPath( - PhabricatorRepository $repository, + PhabricatorRepository $repository = null, DifferentialDiff $diff = null) { $base = '/'; @@ -151,8 +166,8 @@ final class DifferentialChangeset extends DifferentialDAO { $path = $this->getFilename(); $path = rtrim($base, '/').'/'.ltrim($path, '/'); - $vcs = $repository->getVersionControlSystem(); - if ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_SVN) { + $svn = PhabricatorRepositoryType::REPOSITORY_TYPE_SVN; + if ($repository && $repository->getVersionControlSystem() == $svn) { $prefix = $repository->getDetail('remote-uri'); $prefix = id(new PhutilURI($prefix))->getPath(); if (!strncmp($path, $prefix, strlen($prefix))) { diff --git a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php index 1f83884de2..03176b9122 100644 --- a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php +++ b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php @@ -64,11 +64,6 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { return $this; } - public function setVsMap(array $vs_map) { - $this->vsMap = $vs_map; - return $this; - } - public function setRevisionID($revision_id) { $this->revisionID = $revision_id; return $this; diff --git a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php index 43374370d6..ae1bb2c99f 100644 --- a/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/base/DiffusionFileContentQuery.php @@ -27,7 +27,7 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { } final public function loadFileContent() { - $this->fileContent = $this->executeQuery(); + return $this->fileContent = $this->executeQuery(); } final public function getRawData() { diff --git a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php index 27f064efbc..f6724641ed 100644 --- a/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php @@ -109,6 +109,10 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $message = 'Closed by '.$data->getAuthorName().'.'; } + if ($commit_is_new) { + $diff = $this->attachToRevision($revision, $committer); + } + $status_closed = ArcanistDifferentialRevisionStatus::CLOSED; $should_close = ($revision->getStatus() != $status_closed) && (!$repository->getDetail('disable-autoclose', false)); @@ -120,12 +124,22 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $committer, DifferentialAction::ACTION_CLOSE); $editor->setIsDaemonWorkflow(true); + + if ($commit_is_new) { + $vs_diff = $this->loadChangedByCommit($diff); + if ($vs_diff) { + $changed_by_commit = PhabricatorEnv::getProductionURI( + '/D'.$revision->getID(). + '?vs='.$vs_diff->getID(). + '&id='.$diff->getID(). + '#differential-review-toc'); + $editor->setChangedByCommit($changed_by_commit); + } + } + $editor->setMessage($message)->save(); } - if ($commit_is_new) { - $this->attachToRevision($revision, $committer); - } } } } @@ -156,13 +170,100 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $this->repository->getCallsign(). $this->commit->getCommitIdentifier()); + // TODO: This is not correct in SVN where one repository can have multiple + // Arcanist projects. + $arcanist_project = id(new PhabricatorRepositoryArcanistProject()) + ->loadOneWhere('repositoryID = %d LIMIT 1', $this->repository->getID()); + if ($arcanist_project) { + $diff->setArcanistProjectPHID($arcanist_project->getPHID()); + } + $parents = DiffusionCommitParentsQuery::newFromDiffusionRequest($drequest) ->loadParents(); if ($parents) { $diff->setSourceControlBaseRevision(head_key($parents)); } - $diff->save(); + // TODO: Attach binary files. + + return $diff->save(); + } + + private function loadChangedByCommit(DifferentialDiff $diff) { + $repository = $this->repository; + + $vs_changesets = array(); + $vs_diff = id(new DifferentialDiff())->loadOneWhere( + 'revisionID = %d AND creationMethod != %s ORDER BY id DESC LIMIT 1', + $diff->getRevisionID(), + 'commit'); + foreach ($vs_diff->loadChangesets() as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $vs_diff); + $vs_changesets[$path] = $changeset; + } + + $changesets = array(); + foreach ($diff->getChangesets() as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $diff); + $changesets[$path] = $changeset; + } + + if (array_fill_keys(array_keys($changesets), true) != + array_fill_keys(array_keys($vs_changesets), true)) { + return $vs_diff; + } + + $hunks = id(new DifferentialHunk())->loadAllWhere( + 'changesetID IN (%Ld)', + mpull($vs_changesets, 'getID')); + $hunks = mgroup($hunks, 'getChangesetID'); + foreach ($vs_changesets as $changeset) { + $changeset->attachHunks(idx($hunks, $changeset->getID(), array())); + } + + $file_phids = array(); + foreach ($vs_changesets as $changeset) { + $metadata = $changeset->getMetadata(); + $file_phid = idx($metadata, 'new:binary-phid'); + if ($file_phid) { + $file_phids[$file_phid] = $file_phid; + } + } + + $files = array(); + if ($file_phids) { + $files = id(new PhabricatorFile())->loadAllWhere( + 'phid IN (%Ls)', + $file_phids); + $files = mpull($files, null, 'getPHID'); + } + + foreach ($changesets as $path => $changeset) { + $vs_changeset = $vs_changesets[$path]; + + $file_phid = idx($vs_changeset->getMetadata(), 'new:binary-phid'); + if ($file_phid) { + if (!isset($files[$file_phid])) { + return $vs_diff; + } + $drequest = DiffusionRequest::newFromDictionary(array( + 'repository' => $this->repository, + 'commit' => $this->commit->getCommitIdentifier(), + 'path' => $path, + )); + $corpus = DiffusionFileContentQuery::newFromDiffusionRequest($drequest) + ->loadFileContent() + ->getCorpus(); + if ($files[$file_phid]->loadFileData() != $corpus) { + return $vs_diff; + } + } else if ($changeset->makeChangesWithContext() != + $vs_changeset->makeChangesWithContext()) { + return $vs_diff; + } + } + + return null; } /** diff --git a/src/applications/repository/worker/commitmessageparser/base/__init__.php b/src/applications/repository/worker/commitmessageparser/base/__init__.php index aa3e46001a..a8955947ae 100644 --- a/src/applications/repository/worker/commitmessageparser/base/__init__.php +++ b/src/applications/repository/worker/commitmessageparser/base/__init__.php @@ -15,12 +15,17 @@ phutil_require_module('phabricator', 'applications/differential/constants/unitst phutil_require_module('phabricator', 'applications/differential/editor/comment'); phutil_require_module('phabricator', 'applications/differential/query/revision'); phutil_require_module('phabricator', 'applications/differential/storage/diff'); +phutil_require_module('phabricator', 'applications/differential/storage/hunk'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); +phutil_require_module('phabricator', 'applications/diffusion/query/filecontent/base'); phutil_require_module('phabricator', 'applications/diffusion/query/parents/base'); phutil_require_module('phabricator', 'applications/diffusion/query/rawdiff/base'); phutil_require_module('phabricator', 'applications/diffusion/request/base'); +phutil_require_module('phabricator', 'applications/files/storage/file'); +phutil_require_module('phabricator', 'applications/repository/storage/arcanistproject'); phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); phutil_require_module('phabricator', 'applications/repository/worker/base'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phutil', 'symbols');