Merge pull request #180 from r4nt/differential-unified-comments

Adds an option to allow sending unified diff contexts in differential mails

conf/default.conf.php

@@ -382,6 +382,13 @@ return array(
   // patches are sent in. Valid options are 'unified' (like diff -u) or 'git'.
   'metamta.differential.patch-format'   => 'unified',
 
+  // Enables a different format for comments in differential emails.
+  // Differential will create unified diffs around the comment, which
+  // will give enough context for people who are only viewing the
+  // reviews in email to understand what is going on. The context will
+  // be created based on the range of the comment.
+  'metamta.differential.unified-comment-context' => true,
+
   // Prefix prepended to mail sent by Diffusion.
   'metamta.diffusion.subject-prefix' => '[Diffusion]',
 

src/__phutil_library_map__.php

@@ -216,6 +216,7 @@ phutil_register_library_map(array(
     'DifferentialBranchFieldSpecification' => 'applications/differential/field/specification/DifferentialBranchFieldSpecification.php',
     'DifferentialCCWelcomeMail' => 'applications/differential/mail/DifferentialCCWelcomeMail.php',
     'DifferentialCCsFieldSpecification' => 'applications/differential/field/specification/DifferentialCCsFieldSpecification.php',
+    'DifferentialChangeSetTestCase' => 'applications/differential/storage/__tests__/DifferentialChangesetTestCase.php',
     'DifferentialChangeType' => 'applications/differential/constants/DifferentialChangeType.php',
     'DifferentialChangeset' => 'applications/differential/storage/DifferentialChangeset.php',
     'DifferentialChangesetDetailView' => 'applications/differential/view/DifferentialChangesetDetailView.php',
@@ -1385,6 +1386,7 @@ phutil_register_library_map(array(
     'DifferentialBranchFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialCCWelcomeMail' => 'DifferentialReviewRequestMail',
     'DifferentialCCsFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialChangeSetTestCase' => 'PhabricatorTestCase',
     'DifferentialChangeset' => 'DifferentialDAO',
     'DifferentialChangesetDetailView' => 'AphrontView',
     'DifferentialChangesetListView' => 'AphrontView',

src/applications/differential/mail/DifferentialCommentMail.php

@@ -152,6 +152,13 @@ final class DifferentialCommentMail extends DifferentialMail {
     if ($inlines) {
       $body[] = 'INLINE COMMENTS';
       $changesets = $this->getChangesets();
+
+      if (PhabricatorEnv::getEnvConfig(
+            'metamta.differential.unified-comment-context', false)) {
+        foreach ($changesets as $changeset) {
+          $changeset->attachHunks($changeset->loadHunks());
+        }
+      }
       foreach ($inlines as $inline) {
         $changeset = $changesets[$inline->getChangesetID()];
         if (!$changeset) {
@@ -165,8 +172,20 @@ final class DifferentialCommentMail extends DifferentialMail {
         } else {
           $range = $start;
         }
-        $content = $inline->getContent();
+
+        if (!PhabricatorEnv::getEnvConfig(
+              'metamta.differential.unified-comment-context', false)) {
           $body[] = $this->formatText("{$file}:{$range} {$content}");
+        } else {
+          $body[] = "================";
+          $body[] = "Comment at: " . $file . ":" . $range;
+          $body[] = $changeset->makeContextDiff($inline, 1);
+          $body[] = "----------------";
+
+          $content = $inline->getContent();
+          $body[] = $content;
+          $body[] = null;
+        }
       }
       $body[] = null;
     }

src/applications/differential/storage/DifferentialChangeset.php

@@ -215,4 +215,90 @@ final class DifferentialChangeset extends DifferentialDAO {
     return false;
   }
 
+  public function makeContextDiff($inline, $add_context) {
+    $context = array();
+    $debug = false;
+    if ($debug) {
+      $context[] = 'Inline: '.$inline->getIsNewFile().' '.
+        $inline->getLineNumber().' '.$inline->getLineLength();
+      foreach ($this->getHunks() as $hunk) {
+        $context[] = 'hunk: '.$hunk->getOldOffset().'-'.
+          $hunk->getOldLen().'; '.$hunk->getNewOffset().'-'.$hunk->getNewLen();
+        $context[] = $hunk->getChanges();
+      }
+    }
+
+    if ($inline->getIsNewFile()) {
+      $prefix = '+';
+    } else {
+      $prefix = '-';
+    }
+    foreach ($this->getHunks() as $hunk) {
+      if ($inline->getIsNewFile()) {
+        $offset = $hunk->getNewOffset();
+        $length = $hunk->getNewLen();
+      } else {
+        $offset = $hunk->getOldOffset();
+        $length = $hunk->getOldLen();
+      }
+      $start = $inline->getLineNumber() - $offset;
+      $end = $start + $inline->getLineLength();
+      // We need to go in if $start == $length, because the last line
+      // might be a "\No newline at end of file" marker, which we want
+      // to show if the additional context is > 0.
+      if ($start <= $length && $end >= 0) {
+        $start = $start - $add_context;
+        $end = $end + $add_context;
+        $hunk_content = array();
+        $hunk_pos = array( "-" => 0, "+" => 0 );
+        $hunk_offset = array( "-" => NULL, "+" => NULL );
+        $hunk_last = array( "-" => NULL, "+" => NULL );
+        foreach (explode("\n", $hunk->getChanges()) as $line) {
+          $in_common = strncmp($line, " ", 1) === 0;
+          $in_old = strncmp($line, "-", 1) === 0 || $in_common;
+          $in_new = strncmp($line, "+", 1) === 0 || $in_common;
+          $in_selected = strncmp($line, $prefix, 1) === 0;
+          $skip = !$in_selected && !$in_common;
+          if ($hunk_pos[$prefix] <= $end) {
+            if ($start <= $hunk_pos[$prefix]) {
+              if (!$skip || ($hunk_pos[$prefix] != $start &&
+                             $hunk_pos[$prefix] != $end)) {
+                if ($in_old) {
+                  if ($hunk_offset["-"] === NULL) {
+                    $hunk_offset["-"] = $hunk_pos["-"];
+                  }
+                  $hunk_last["-"] = $hunk_pos["-"];
+                }
+                if ($in_new) {
+                  if ($hunk_offset["+"] === NULL) {
+                    $hunk_offset["+"] = $hunk_pos["+"];
+                  }
+                  $hunk_last["+"] = $hunk_pos["+"];
+                }
+
+                $hunk_content[] = $line;
+              }
+            }
+            if ($in_old) { ++$hunk_pos["-"]; }
+            if ($in_new) { ++$hunk_pos["+"]; }
+          }
+        }
+        if ($hunk_offset["-"] !== NULL || $hunk_offset["+"] !== NULL) {
+          $header = "@@";
+          if ($hunk_offset["-"] !== NULL) {
+            $header .= " -" . ($hunk->getOldOffset() + $hunk_offset["-"]) .
+              "," . ($hunk_last["-"]-$hunk_offset["-"]+1);
+          }
+          if ($hunk_offset["+"] !== NULL) {
+            $header .= " +" . ($hunk->getNewOffset() + $hunk_offset["+"]) .
+              "," . ($hunk_last["+"]-$hunk_offset["+"]+1);
+          }
+          $header .= " @@";
+          $context[] = $header;
+          $context[] = implode("\n", $hunk_content);
+        }
+      }
+    }
+    return implode("\n", $context);
+  }
 }

src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php

@@ -0,0 +1,193 @@
+<?php
+
+final class DifferentialChangeSetTestCase extends PhabricatorTestCase {
+  private function createComment() {
+    $comment = new DifferentialInlineComment();
+    return $comment;
+  }
+  // $line: 1 based
+  // $length: 0 based (0 meaning 1 line)
+  private function createNewComment($line, $length) {
+    $comment = $this->createComment();
+    $comment->setIsNewFile(True);
+    $comment->setLineNumber($line);
+    $comment->setLineLength($length);
+    return $comment;
+  }
+  // $line: 1 based
+  // $length: 0 based (0 meaning 1 line)
+  private function createOldComment($line, $length) {
+    $comment = $this->createComment();
+    $comment->setIsNewFile(False);
+    $comment->setLineNumber($line);
+    $comment->setLineLength($length);
+    return $comment;
+  }
+  private function createHunk($oldOffset, $oldLen, $newOffset, $newLen, $changes) {
+    $hunk = new DifferentialHunk();
+    $hunk->setOldOffset($oldOffset);
+    $hunk->setOldLen($oldLen);
+    $hunk->setNewOffset($newOffset);
+    $hunk->setNewLen($newLen);
+    $hunk->setChanges($changes);
+    return $hunk;
+  }
+  private function createChange($hunks) {
+    $change = new DifferentialChangeset();
+    $change->attachHunks($hunks);
+    return $change;
+  }
+  // Returns a change that consists of a single hunk, starting at line 1.
+  private function createSingleChange($old_lines, $new_lines, $changes) {
+    return $this->createChange(array(
+      0 => $this->createHunk(1, $old_lines, 1, $new_lines, $changes),
+    ));
+  }
+
+  public function testOneLineOldComment() {
+    $change = $this->createSingleChange(1, 0, "-a");
+    $context = $change->makeContextDiff($this->createOldComment(1, 0), 0); 
+    $this->assertEqual("@@ -1,1 @@\n-a", $context);
+  }
+
+  public function testOneLineNewComment() {
+    $change = $this->createSingleChange(0, 1, "+a");
+    $context = $change->makeContextDiff($this->createNewComment(1, 0), 0); 
+    $this->assertEqual("@@ +1,1 @@\n+a", $context);
+  }
+
+  public function testCannotFindContext() {
+    $change = $this->createSingleChange(0, 1, "+a");
+    $context = $change->makeContextDiff($this->createNewComment(2, 0), 0); 
+    $this->assertEqual("", $context);
+  }
+  
+  public function testOverlapFromStartOfHunk() {
+    $change = $this->createChange(array(
+      0 => $this->createHunk(23, 2, 42, 2, " 1\n 2"),
+    ));
+    $context = $change->makeContextDiff($this->createNewComment(41, 1), 0); 
+    $this->assertEqual("@@ -23,1 +42,1 @@\n 1", $context);
+  }
+
+  public function testOverlapAfterEndOfHunk() {
+    $change = $this->createChange(array(
+      0 => $this->createHunk(23, 2, 42, 2, " 1\n 2"),
+    ));
+    $context = $change->makeContextDiff($this->createNewComment(43, 1), 0); 
+    $this->assertEqual("@@ -24,1 +43,1 @@\n 2", $context);
+  }
+
+  public function testInclusionOfNewFileInOldCommentFromStart() {
+    $change = $this->createSingleChange(2, 3,
+         "+n1\n".
+         " e1/2\n".
+         "-o2\n".
+         "+n3\n");
+    $context = $change->makeContextDiff($this->createOldComment(1, 1), 0); 
+    $this->assertEqual(
+        "@@ -1,2 +2,1 @@\n".
+        " e1/2\n".
+        "-o2", $context);
+  }
+
+  public function testInclusionOfOldFileInNewCommentFromStart() {
+    $change = $this->createSingleChange(2, 2,
+         "-o1\n".
+         " e2/1\n".
+         "-o3\n".
+         "+n2\n");
+    $context = $change->makeContextDiff($this->createNewComment(1, 1), 0); 
+    $this->assertEqual(
+        "@@ -2,1 +1,2 @@\n".
+        " e2/1\n".
+        "+n2", $context);
+  }
+
+  public function testNoNewlineAtEndOfFile() {
+    $change = $this->createSingleChange(0, 1, 
+        "+a\n".
+        "\\No newline at end of file");
+    // Note that this only works with additional context.
+    $context = $change->makeContextDiff($this->createNewComment(2, 0), 1); 
+    $this->assertEqual(
+        "@@ +1,1 @@\n".
+        "+a\n".
+        "\\No newline at end of file", $context);
+  }
+
+  public function testMultiLineNewComment() {
+    $change = $this->createSingleChange(7, 7,
+        " e1\n".
+        " e2\n".
+        "-o3\n".
+        "-o4\n".
+        "+n3\n".
+        " e5/4\n".
+        " e6/5\n".
+        "+n6\n".
+        " e7\n");
+    $context = $change->makeContextDiff($this->createNewComment(2, 4), 0);
+    $this->assertEqual(
+        "@@ -2,5 +2,5 @@\n".
+        " e2\n".
+        "-o3\n".
+        "-o4\n".
+        "+n3\n".
+        " e5/4\n".
+        " e6/5\n".
+        "+n6", $context);
+  }
+
+  public function testMultiLineOldComment() {
+    $change = $this->createSingleChange(7, 7,
+        " e1\n".
+        " e2\n".
+        "-o3\n".
+        "-o4\n".
+        "+n3\n".
+        " e5/4\n".
+        " e6/5\n".
+        "+n6\n".
+        " e7\n");
+    $context = $change->makeContextDiff($this->createOldComment(2, 4), 0);
+    $this->assertEqual(
+        "@@ -2,5 +2,4 @@\n".
+        " e2\n".
+        "-o3\n".
+        "-o4\n".
+        "+n3\n".
+        " e5/4\n".
+        " e6/5", $context);
+  }
+
+  public function testInclusionOfNewFileInOldCommentFromStartWithContext() {
+    $change = $this->createSingleChange(2, 3,
+         "+n1\n".
+         " e1/2\n".
+         "-o2\n".
+         "+n3\n");
+    $context = $change->makeContextDiff($this->createOldComment(1, 1), 1); 
+    $this->assertEqual(
+        "@@ -1,2 +1,2 @@\n".
+        "+n1\n".
+        " e1/2\n".
+        "-o2", $context);
+  }
+
+  public function testInclusionOfOldFileInNewCommentFromStartWithContext() {
+    $change = $this->createSingleChange(2, 2,
+         "-o1\n".
+         " e2/1\n".
+         "-o3\n".
+         "+n2\n");
+    $context = $change->makeContextDiff($this->createNewComment(1, 1), 1); 
+    $this->assertEqual(
+        "@@ -1,3 +1,2 @@\n".
+        "-o1\n".
+        " e2/1\n".
+        "-o3\n".
+        "+n2", $context);
+  }
+}
+