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

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) {
+    $target = idx($diffs, $target_id, end($diffs));
-      if (isset($diffs[$target_id])) {
-        $target = $diffs[$target_id];
-      }
-    }
 
     $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,
+    DifferentialDiff $target,
-    $diff_vs,
+    DifferentialDiff $diff_vs = null,
-    DifferentialDiff $target) {
+    PhabricatorRepository $repository = null) {
-    assert_instances_of($diffs, 'DifferentialDiff');
 
     $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 = array();
-      $vs_changesets = mpull($vs_changesets, null, 'getFilename');
+      $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()] =

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;
     }
 

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();
+    $svn = PhabricatorRepositoryType::REPOSITORY_TYPE_SVN;
-    if ($vcs == 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))) {

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;

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() {

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;
   }
 
   /**

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');