Port comments through time and space in the common/best case

Summary:
Ref T7447. This ports comments forward and backward in the best case:

  - The old comment is on a changeset with the same filename.
  - The old and new files are pretty much the same, line-for-line.

This will fail to port a lot of comments around and probably port a lot of comments into goofy places. We can see how bad it is in practice.

Errata:

  - Design is me cobbling something together, probably worth tweaking.
  - "Old Comment" should, at a minimum, say "Newer Comment" sometimes, or we should come up with some better name for this stuff.

Test Plan: {F377214}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: johnny-bit, yelirekim, epriestley

Maniphest Tasks: T7447

Differential Revision: https://secure.phabricator.com/D12484

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -5,6 +5,7 @@ final class PhabricatorAuditInlineComment
 
   private $proxy;
   private $syntheticAuthor;
+  private $isGhost;
 
   public function __construct() {
     $this->proxy = new PhabricatorAuditTransactionComment();
@@ -308,6 +309,15 @@ final class PhabricatorAuditInlineComment
     return $this->proxy->getFixedState();
   }
 
+  public function setIsGhost($is_ghost) {
+    $this->isGhost = $is_ghost;
+    return $this;
+  }
+
+  public function getIsGhost() {
+    return $this->isGhost;
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -9,8 +9,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $author_phid = $viewer->getPHID();
-
     $rendering_reference = $request->getStr('ref');
     $parts = explode('/', $rendering_reference);
     if (count($parts) == 2) {
@@ -161,10 +159,33 @@ final class DifferentialChangesetViewController extends DifferentialController {
       $parser->setOriginals($left, $right);
     }
 
+    $diff = $changeset->getDiff();
+    $revision_id = $diff->getRevisionID();
+
+    $can_mark = false;
+    $object_owner_phid = null;
+    if ($revision_id) {
+      $revision = id(new DifferentialRevisionQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($revision_id))
+        ->executeOne();
+      if ($revision) {
+        $can_mark = ($revision->getAuthorPHID() == $viewer->getPHID());
+        $object_owner_phid = $revision->getAuthorPHID();
+      }
+    }
+
     // Load both left-side and right-side inline comments.
-    $inlines = $this->loadInlineComments(
-      array($left_source, $right_source),
-      $author_phid);
+    if ($revision) {
+      $inlines = $this->loadInlineComments(
+        $revision,
+        nonempty($left, $right),
+        $left_new,
+        $right,
+        $right_new);
+    } else {
+      $inlines = array();
+    }
 
     if ($left_new) {
       $inlines = array_merge(
@@ -201,22 +222,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
 
     $engine->process();
 
-    $diff = $changeset->getDiff();
-    $revision_id = $diff->getRevisionID();
-
-    $can_mark = false;
-    $object_owner_phid = null;
-    if ($revision_id) {
-      $revision = id(new DifferentialRevisionQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($revision_id))
-        ->executeOne();
-      if ($revision) {
-        $can_mark = ($revision->getAuthorPHID() == $viewer->getPHID());
-        $object_owner_phid = $revision->getAuthorPHID();
-      }
-    }
-
     $parser
       ->setUser($viewer)
       ->setMarkupEngine($engine)
@@ -280,16 +285,82 @@ final class DifferentialChangesetViewController extends DifferentialController {
       ));
   }
 
-  private function loadInlineComments(array $changeset_ids, $author_phid) {
-    $changeset_ids = array_unique(array_filter($changeset_ids));
-    if (!$changeset_ids) {
-      return;
+  private function loadInlineComments(
+    DifferentialRevision $revision,
+    DifferentialChangeset $left,
+    $left_new,
+    DifferentialChangeset $right,
+    $right_new) {
+
+    $viewer = $this->getViewer();
+    $all = array($left, $right);
+
+    $inlines = id(new DifferentialInlineCommentQuery())
+      ->setViewer($viewer)
+      ->withRevisionPHIDs(array($revision->getPHID()))
+      ->execute();
+
+    $changeset_ids = mpull($inlines, 'getChangesetID');
+    $changeset_ids = array_unique($changeset_ids);
+    if ($changeset_ids) {
+      $changesets = id(new DifferentialChangesetQuery())
+        ->setViewer($viewer)
+        ->withIDs($changeset_ids)
+        ->execute();
+      $changesets = mpull($changesets, null, 'getID');
+    } else {
+      $changesets = array();
+    }
+    $changesets += mpull($all, null, 'getID');
+
+    $id_map = array(
+      $left->getID() => $left->getID(),
+      $right->getID() => $right->getID(),
+    );
+
+    $name_map = array(
+      $left->getFilename() => $left->getID(),
+      $right->getFilename() => $right->getID(),
+    );
+
+    $results = array();
+    foreach ($inlines as $inline) {
+      $changeset_id = $inline->getChangesetID();
+      if (isset($id_map[$changeset_id])) {
+        // This inline is legitimately on one of the current changesets, so
+        // we can include it in the result set unmodified.
+        $results[] = $inline;
+        continue;
+      }
+
+      $changeset = idx($changesets, $changeset_id);
+      if (!$changeset) {
+        // Just discard this inline with bogus data.
+        continue;
+      }
+
+      $target_id = null;
+
+      $filename = $changeset->getFilename();
+      if (isset($name_map[$filename])) {
+        // This changeset is on a file with the same name as the current
+        // changeset, so we're going to port it forward or backward.
+        $target_id = $name_map[$filename];
+      }
+
+      // If we found a changeset to port this comment to, bring it forward
+      // or backward and mark it.
+      if ($target_id) {
+        $inline
+          ->makeEphemeral(true)
+          ->setChangesetID($target_id)
+          ->setIsGhost(true);
+
+        $results[] = $inline;
+      }
     }
 
-    return id(new DifferentialInlineCommentQuery())
-      ->setViewer($this->getViewer())
-      ->withChangesetIDs($changeset_ids)
-      ->execute();
+    return $results;
   }
 
   private function buildRawFileResponse(

src/applications/differential/query/DifferentialInlineCommentQuery.php

@@ -13,7 +13,6 @@ final class DifferentialInlineCommentQuery
   private $phids;
   private $drafts;
   private $authorPHIDs;
-  private $changesetIDs;
   private $revisionPHIDs;
   private $deletedDrafts;
 
@@ -46,11 +45,6 @@ final class DifferentialInlineCommentQuery
     return $this;
   }
 
-  public function withChangesetIDs(array $ids) {
-    $this->changesetIDs = $ids;
-    return $this;
-  }
-
   public function withRevisionPHIDs(array $revision_phids) {
     $this->revisionPHIDs = $revision_phids;
     return $this;
@@ -116,13 +110,6 @@ final class DifferentialInlineCommentQuery
         $this->revisionPHIDs);
     }
 
-    if ($this->changesetIDs !== null) {
-      $where[] = qsprintf(
-        $conn_r,
-        'changesetID IN (%Ld)',
-        $this->changesetIDs);
-    }
-
     if ($this->drafts === null) {
       if ($this->deletedDrafts) {
         $where[] = qsprintf(

src/applications/differential/storage/DifferentialInlineComment.php

@@ -5,6 +5,7 @@ final class DifferentialInlineComment
 
   private $proxy;
   private $syntheticAuthor;
+  private $isGhost;
 
   public function __construct() {
     $this->proxy = new DifferentialTransactionComment();
@@ -225,6 +226,20 @@ final class DifferentialInlineComment
     return $this->proxy->getFixedState();
   }
 
+  public function setIsGhost($is_ghost) {
+    $this->isGhost = $is_ghost;
+    return $this;
+  }
+
+  public function getIsGhost() {
+    return $this->isGhost;
+  }
+
+  public function makeEphemeral() {
+    $this->proxy->makeEphemeral();
+    return $this;
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 

src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php

@@ -54,4 +54,7 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface {
   public function save();
   public function delete();
 
+  public function setIsGhost($is_ghost);
+  public function getIsGhost();
+
 }

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -122,6 +122,19 @@ final class PHUIDiffInlineCommentDetailView
         ->addClass('mml inline-draft-text');
     }
 
+    $ghost_tag = null;
+    if ($inline->getIsGhost()) {
+      // TODO: This could also be a newer comment, not necessarily an older
+      // comment.
+      $ghost_tag = id(new PHUITagView())
+        ->setType(PHUITagView::TYPE_SHADE)
+        ->setName(pht('Old Comment'))
+        ->setSlimShady(true)
+        ->setShade(PHUITagView::COLOR_BLUE)
+        ->addClass('mml');
+      $classes[] = 'inline-comment-ghost';
+    }
+
     // I think this is unused
     if ($inline->getHasReplies()) {
       $classes[] = 'inline-comment-has-reply';
@@ -351,6 +364,7 @@ final class PHUIDiffInlineCommentDetailView
         $author,
         $author_owner,
         $draft_text,
+        $ghost_tag,
       ));
 
     $group_right = phutil_tag(

webroot/rsrc/css/application/differential/phui-inline-comment.css

@@ -132,6 +132,32 @@
     padding-bottom: 4px;
 }
 
+/* - Ghost Comment ------------------------------------------------------------
+
+  Comments from older or newer versions of the changeset.
+
+*/
+
+.differential-inline-comment.inline-comment-ghost {
+  border: 1px solid {$lightgreyborder};
+  opacity: 0.75;
+}
+
+.differential-inline-comment.inline-comment-ghost
+  .differential-inline-comment-head {
+    border-bottom: 1px solid {$lightgreyborder};
+    background-color: {$lightgreybackground};
+}
+
+.differential-inline-comment.inline-comment-ghost
+  .button.simple {
+  border-color: {$lightgreyborder};
+}
+
+.differential-inline-comment.inline-comment-ghost
+  .button.simple .phui-icon-view {
+  color: {$lightgreytext};
+}
 
 /* - New/Edit Inline Comment --------------------------------------------------
 
@@ -350,7 +376,7 @@
 }
 
 .differential-inline-comment.inline-state-is-draft .inline-draft-text {
-    display: inline-block;
+  display: inline-block;
 }
 
 .inline-state-is-draft .differential-inline-done-label {