From 17217fda2864dad025d13e8996b910c0ad2e7dd9 Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Thu, 2 Aug 2012 13:00:18 +0000 Subject: [PATCH 1/3] Adds an option to allow sending unified diff contexts in differential mails. --- conf/default.conf.php | 7 + src/__phutil_library_map__.php | 2 + .../mail/DifferentialCommentMail.php | 17 +- .../storage/DifferentialChangeset.php | 84 ++++++++ .../DifferentialChangesetTestCase.php | 193 ++++++++++++++++++ 5 files changed, 301 insertions(+), 2 deletions(-) create mode 100644 src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 8130bb85dc..0d76da634c 100644 --- a/conf/default.conf.php +++ b/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]', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cf538cb35b..27418300b2 100644 --- a/src/__phutil_library_map__.php +++ b/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', @@ -1381,6 +1382,7 @@ phutil_register_library_map(array( 'DifferentialBranchFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialCCWelcomeMail' => 'DifferentialReviewRequestMail', 'DifferentialCCsFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialChangeSetTestCase' => 'PhabricatorTestCase', 'DifferentialChangeset' => 'DifferentialDAO', 'DifferentialChangesetDetailView' => 'AphrontView', 'DifferentialChangesetListView' => 'AphrontView', diff --git a/src/applications/differential/mail/DifferentialCommentMail.php b/src/applications/differential/mail/DifferentialCommentMail.php index 412dcd8b63..4f11596419 100644 --- a/src/applications/differential/mail/DifferentialCommentMail.php +++ b/src/applications/differential/mail/DifferentialCommentMail.php @@ -165,8 +165,21 @@ final class DifferentialCommentMail extends DifferentialMail { } else { $range = $start; } - $content = $inline->getContent(); - $body[] = $this->formatText("{$file}:{$range} {$content}"); + + if (!PhabricatorEnv::getEnvConfig( + 'metamta.differential.unified-comment-context', false)) { + $body[] = $this->formatText("{$file}:{$range} {$content}"); + } else { + $body[] = "Comment at: " . $file . ":" . $range; + + $changeset->attachHunks($changeset->loadHunks()); + $body[] = $changeset->makeContextDiff($inline, 1); + $body[] = null; + + $content = $inline->getContent(); + $body[] = $content; + $body[] = null; + } } $body[] = null; } diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 199dce99ee..f566a403b4 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -215,4 +215,88 @@ 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) { + $inCommon = strncmp($line, " ", 1) === 0; + $inOld = strncmp($line, "-", 1) === 0 || $inCommon; + $inNew = strncmp($line, "+", 1) === 0 || $inCommon; + $inSelected = strncmp($line, $prefix, 1) === 0; + $skip = !$inSelected && !$inCommon; + if ($hunk_pos[$prefix] <= $end) { + if ($start <= $hunk_pos[$prefix]) { + if (!$skip || ($hunk_pos[$prefix] != $start && + $hunk_pos[$prefix] != $end)) { + if ($inOld) { + if ($hunk_offset["-"] === NULL) + $hunk_offset["-"] = $hunk_pos["-"]; + $hunk_last["-"] = $hunk_pos["-"]; + } + if ($inNew) { + if ($hunk_offset["+"] === NULL) + $hunk_offset["+"] = $hunk_pos["+"]; + $hunk_last["+"] = $hunk_pos["+"]; + } + + $hunk_content[] = $line; + } + } + if ($inOld) ++$hunk_pos["-"]; + if ($inNew) ++$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); + } } diff --git a/src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php b/src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php new file mode 100644 index 0000000000..a8932dc91d --- /dev/null +++ b/src/applications/differential/storage/__tests__/DifferentialChangesetTestCase.php @@ -0,0 +1,193 @@ +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); + } +} + From 0ce886121c0f137c8fbf36df76503306af38f306 Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Mon, 13 Aug 2012 08:08:30 +0000 Subject: [PATCH 2/3] Make comments easier to read. --- src/applications/differential/mail/DifferentialCommentMail.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/mail/DifferentialCommentMail.php b/src/applications/differential/mail/DifferentialCommentMail.php index 4f11596419..4d6c983f63 100644 --- a/src/applications/differential/mail/DifferentialCommentMail.php +++ b/src/applications/differential/mail/DifferentialCommentMail.php @@ -170,11 +170,12 @@ final class DifferentialCommentMail extends DifferentialMail { 'metamta.differential.unified-comment-context', false)) { $body[] = $this->formatText("{$file}:{$range} {$content}"); } else { + $body[] = "================"; $body[] = "Comment at: " . $file . ":" . $range; $changeset->attachHunks($changeset->loadHunks()); $body[] = $changeset->makeContextDiff($inline, 1); - $body[] = null; + $body[] = "----------------"; $content = $inline->getContent(); $body[] = $content; From 10773c5cb63ddc754cfc1e1355b1f53d6aaffabb Mon Sep 17 00:00:00 2001 From: Manuel Klimek Date: Mon, 13 Aug 2012 19:32:19 +0000 Subject: [PATCH 3/3] Adapt to style changes; fix performance bug when loading hunks for changesets. --- .../mail/DifferentialCommentMail.php | 9 +++++-- .../storage/DifferentialChangeset.php | 26 ++++++++++--------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/applications/differential/mail/DifferentialCommentMail.php b/src/applications/differential/mail/DifferentialCommentMail.php index 4d6c983f63..e9c962de3b 100644 --- a/src/applications/differential/mail/DifferentialCommentMail.php +++ b/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) { @@ -172,8 +179,6 @@ final class DifferentialCommentMail extends DifferentialMail { } else { $body[] = "================"; $body[] = "Comment at: " . $file . ":" . $range; - - $changeset->attachHunks($changeset->loadHunks()); $body[] = $changeset->makeContextDiff($inline, 1); $body[] = "----------------"; diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index f566a403b4..ef36c09891 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -217,7 +217,7 @@ final class DifferentialChangeset extends DifferentialDAO { public function makeContextDiff($inline, $add_context) { $context = array(); - $debug = False; + $debug = false; if ($debug) { $context[] = 'Inline: '.$inline->getIsNewFile().' '. $inline->getLineNumber().' '.$inline->getLineLength(); @@ -254,31 +254,33 @@ final class DifferentialChangeset extends DifferentialDAO { $hunk_offset = array( "-" => NULL, "+" => NULL ); $hunk_last = array( "-" => NULL, "+" => NULL ); foreach (explode("\n", $hunk->getChanges()) as $line) { - $inCommon = strncmp($line, " ", 1) === 0; - $inOld = strncmp($line, "-", 1) === 0 || $inCommon; - $inNew = strncmp($line, "+", 1) === 0 || $inCommon; - $inSelected = strncmp($line, $prefix, 1) === 0; - $skip = !$inSelected && !$inCommon; + $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 ($inOld) { - if ($hunk_offset["-"] === NULL) + if ($in_old) { + if ($hunk_offset["-"] === NULL) { $hunk_offset["-"] = $hunk_pos["-"]; + } $hunk_last["-"] = $hunk_pos["-"]; } - if ($inNew) { - if ($hunk_offset["+"] === NULL) + if ($in_new) { + if ($hunk_offset["+"] === NULL) { $hunk_offset["+"] = $hunk_pos["+"]; + } $hunk_last["+"] = $hunk_pos["+"]; } $hunk_content[] = $line; } } - if ($inOld) ++$hunk_pos["-"]; - if ($inNew) ++$hunk_pos["+"]; + if ($in_old) { ++$hunk_pos["-"]; } + if ($in_new) { ++$hunk_pos["+"]; } } } if ($hunk_offset["-"] !== NULL || $hunk_offset["+"] !== NULL) {