Make DifferentialChangesetParser explicitly map display to storage for comments

Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.

One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.

Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.

This has the nice side-effect of fixing T132 and several other bugs.

Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.

Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.

Repeated these steps for a previous diff in the same revision; verified
comments.

Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.

NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237

src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php

@@ -154,8 +154,12 @@ class DifferentialChangesetViewController extends DifferentialController {
     $parser->setLeftSideCommentMapping($left_source, $left_new);
     $parser->setWhitespaceMode($request->getStr('whitespace'));
 
+    // Load both left-side and right-side inline comments.
+    $inlines = $this->loadInlineComments(
+      array($left_source, $right_source),
+      $author_phid);
+
     $phids = array();
-    $inlines = $this->loadInlineComments($id, $author_phid);
     foreach ($inlines as $inline) {
       $parser->parseInlineComment($inline);
       $phids[$inline->getAuthorPHID()] = true;
@@ -228,10 +232,15 @@ class DifferentialChangesetViewController extends DifferentialController {
       ));
   }
 
-  private function loadInlineComments($changeset_id, $author_phid) {
+  private function loadInlineComments(array $changeset_ids, $author_phid) {
+    $changeset_ids = array_unique(array_filter($changeset_ids));
+    if (!$changeset_ids) {
+      return;
+    }
+
     return id(new DifferentialInlineComment())->loadAllWhere(
-      'changesetID = %d AND (commentID IS NOT NULL OR authorPHID = %s)',
+      'changesetID IN (%Ld) AND (commentID IS NOT NULL OR authorPHID = %s)',
-      $changeset_id,
+      $changeset_ids,
       $author_phid);
   }
 

src/applications/differential/parser/changeset/DifferentialChangesetParser.php

@@ -39,7 +39,6 @@ class DifferentialChangesetParser {
   protected $whitespaceMode = null;
 
   protected $subparser;
-  protected $oldChangesetID = null;
   protected $noHighlight;
 
   protected $renderCacheKey = null;
@@ -47,6 +46,12 @@ class DifferentialChangesetParser {
   private $handles;
   private $user;
 
+  private $leftSideChangesetID;
+  private $leftSideAttachesToNewFile;
+
+  private $rightSideChangesetID;
+  private $rightSideAttachesToNewFile;
+
   const CACHE_VERSION = 4;
 
   const ATTR_GENERATED  = 'attr:generated';
@@ -60,12 +65,38 @@ class DifferentialChangesetParser {
   const WHITESPACE_IGNORE_TRAILING  = 'ignore-trailing';
   const WHITESPACE_IGNORE_ALL       = 'ignore-all';
 
+  /**
+   * Configure which Changeset comments added to the right side of the visible
+   * diff will be attached to. The ID must be the ID of a real Differential
+   * Changeset.
+   *
+   * The complexity here is that we may show an arbitrary side of an arbitrary
+   * changeset as either the left or right part of a diff. This method allows
+   * the left and right halves of the displayed diff to be correctly mapped to
+   * storage changesets.
+   *
+   * @param id    The Differential Changeset ID that comments added to the right
+   *              side of the visible diff should be attached to.
+   * @param bool  If true, attach new comments to the right side of the storage
+   *              changeset. Note that this may be false, if the left side of
+   *              some storage changeset is being shown as the right side of
+   *              a display diff.
+   * @return this
+   */
   public function setRightSideCommentMapping($id, $is_new) {
-
+    $this->rightSideChangesetID = $id;
+    $this->rightSideAttachesToNewFile = $is_new;
+    return $this;
   }
 
+  /**
+   * See setRightSideCommentMapping(), but this sets information for the left
+   * side of the display diff.
+   */
   public function setLeftSideCommentMapping($id, $is_new) {
-
+    $this->leftSideChangesetID = $id;
+    $this->leftSideAttachesToNewFile = $is_new;
+    return $this;
   }
 
   /**
@@ -104,11 +135,6 @@ class DifferentialChangesetParser {
     return $this;
   }
 
-  public function setOldChangesetID($old_changeset_id) {
-    $this->oldChangesetID = $old_changeset_id;
-    return $this;
-  }
-
   public function setChangesetID($changeset_id) {
     $this->changesetID = $changeset_id;
     return $this;
@@ -236,7 +262,10 @@ class DifferentialChangesetParser {
   }
 
   public function parseInlineComment(DifferentialInlineComment $comment) {
+    // Parse only comments which are actually visible.
+    if ($this->isCommentVisibleOnRenderedDiff($comment)) {
       $this->comments[] = $comment;
+    }
     return $this;
   }
 
@@ -837,7 +866,7 @@ EOSYNTHETIC;
         $end = $comment->getLineNumber() +
                $comment->getLineLength() +
                self::LINES_CONTEXT;
-        $new = $this->isCommentInNewFile($comment);
+        $new = $this->isCommentOnRightSideWhenDisplayed($comment);
         for ($ii = $start; $ii <= $end; $ii++) {
           if ($new) {
             $new_mask[$ii] = true;
@@ -862,7 +891,7 @@ EOSYNTHETIC;
       foreach ($this->comments as $comment) {
         $final = $comment->getLineNumber() +
                  $comment->getLineLength();
-        if ($this->isCommentInNewFile($comment)) {
+        if ($this->isCommentOnRightSideWhenDisplayed($comment)) {
           $new_comments[$final][] = $comment;
         } else {
           $old_comments[$final][] = $comment;
@@ -881,12 +910,58 @@ EOSYNTHETIC;
     return $this->renderChangesetTable($this->changeset, $html);
   }
 
-  private function isCommentInNewFile(DifferentialInlineComment $comment) {
+  /**
-    if ($this->oldChangesetID) {
+   * Determine if an inline comment will appear on the rendered diff,
-      return ($comment->getChangesetID() != $this->oldChangesetID);
+   * taking into consideration which halves of which changesets will actually
-    } else {
+   * be shown.
-      return $comment->getIsNewFile();
+   *
+   * @param DifferentialInlineComment Comment to test for visibility.
+   * @return bool True if the comment is visible on the rendered diff.
+   */
+  private function isCommentVisibleOnRenderedDiff(
+    DifferentialInlineComment $comment) {
+
+    $changeset_id = $comment->getChangesetID();
+    $is_new = $comment->getIsNewFile();
+
+    if ($changeset_id == $this->rightSideChangesetID &&
+        $is_new == $this->rightSideAttachesToNewFile) {
+      return true;
     }
+
+    if ($changeset_id == $this->leftSideChangesetID &&
+        $is_new == $this->leftSideAttachesToNewFile) {
+      return true;
+    }
+
+    return false;
+  }
+
+
+  /**
+   * Determine if a comment will appear on the right side of the display diff.
+   * Note that the comment must appear somewhere on the rendered changeset, as
+   * per isCommentVisibleOnRenderedDiff().
+   *
+   * @param DifferentialInlineComment Comment to test for display location.
+   * @return bool True for right, false for left.
+   */
+  private function isCommentOnRightSideWhenDisplayed(
+    DifferentialInlineComment $comment) {
+
+    if (!$this->isCommentVisibleOnRenderedDiff($comment)) {
+      throw new Exception("Comment is not visible on changeset!");
+    }
+
+    $changeset_id = $comment->getChangesetID();
+    $is_new = $comment->getIsNewFile();
+
+    if ($changeset_id == $this->rightSideChangesetID &&
+        $is_new == $this->rightSideAttachesToNewFile) {
+      return true;
+    }
+
+    return false;
   }
 
   protected function renderShield($message, $more) {
@@ -961,6 +1036,15 @@ EOSYNTHETIC;
 
     $range_len = min($range_len, $rows - $range_start);
 
+    // Gaps - compute gaps in the visible display diff, where we will render
+    // "Show more context" spacers. This builds an aggregate $mask of all the
+    // lines we must show (because they are near changed lines, near inline
+    // comments, or the request has explicitly asked for them, i.e. resulting
+    // from the user clicking "show more") and then finds all the gaps between
+    // visible lines. If a gap is smaller than the context size, we just
+    // display it. Otherwise, we record it into $gaps and will render a
+    // "show more context" element instead of diff text below.
+
     $gaps = array();
     $gap_start = 0;
     $in_gap = false;
@@ -989,11 +1073,29 @@ EOSYNTHETIC;
 
     $gaps = array_reverse($gaps);
 
-    $changeset = $this->changesetID;
     $reference = $this->getChangeset()->getRenderingReference();
 
+    $left_id = $this->leftSideChangesetID;
+    $right_id = $this->rightSideChangesetID;
+
+    // "N" stands for 'new' and means the comment should attach to the new file
+    // when stored, i.e. DifferentialInlineComment->setIsNewFile().
+    // "O" stands for 'old' and means the comment should attach to the old file.
+
+    $left_char = $this->leftSideAttachesToNewFile
+      ? 'N'
+      : 'O';
+    $right_char = $this->rightSideAttachesToNewFile
+      ? 'N'
+      : 'O';
+
     for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) {
       if (empty($mask[$ii])) {
+        // If we aren't going to show this line, we've just entered a gap.
+        // Pop information about the next gap off the $gaps stack and render
+        // an appropriate "Show more context" element. This branch eventually
+        // increments $ii by the entire size of the gap and then continues
+        // the loop.
         $gap = array_pop($gaps);
         $top = $gap[0];
         $len = $gap[1];
@@ -1100,14 +1202,14 @@ EOSYNTHETIC;
         $html[] = $context_not_available;
       }
 
-      if ($o_num && $changeset) {
+      if ($o_num && $left_id) {
-        $o_id = ' id="C'.$changeset.'OL'.$o_num.'"';
+        $o_id = ' id="C'.$left_id.$left_char.'L'.$o_num.'"';
       } else {
         $o_id = null;
       }
 
-      if ($n_num && $changeset) {
+      if ($n_num && $right_id) {
-        $n_id = ' id="C'.$changeset.'NL'.$n_num.'"';
+        $n_id = ' id="C'.$right_id.$right_char.'L'.$n_num.'"';
       } else {
         $n_id = null;
       }
@@ -1156,7 +1258,7 @@ EOSYNTHETIC;
             ($comment->getAuthorPHID() == $user->getPHID()) &&
             (!$comment->getCommentID());
 
-    $on_right = $this->isCommentInNewFile($comment);
+    $on_right = $this->isCommentOnRightSideWhenDisplayed($comment);
 
     return id(new DifferentialInlineCommentView())
       ->setInlineComment($comment)