Fix various newline problems in the difference engines

Summary: I'll mark this one up inline since it's all separate bugs.

Test Plan:
  - Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
  - Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
  - All 32 results seemed sensible.
  - Really wish this stuff was better factored and testable. Need to fix it. :(

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T1030

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

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

@@ -228,7 +228,7 @@ final class DifferentialChangesetParser {
         'text'  => (string)substr($lines[$cursor], 1),
         'line'  => $new_line,
       );
-      if ($type == '\\' && $cursor > 1) {
+      if ($type == '\\') {
         $type = $types[$cursor - 1];
         $data['text'] = ltrim($data['text']);
       }
@@ -338,9 +338,14 @@ final class DifferentialChangesetParser {
             break;
         }
         if ($similar) {
-          $o_desc['type'] = null;
+          if ($o_desc['type'] == '\\') {
-          $n_desc['type'] = null;
+            // These are similar because they're "No newline at end of file"
-          $skip_intra[count($old)] = true;
+            // comments.
+          } else {
+            $o_desc['type'] = null;
+            $n_desc['type'] = null;
+            $skip_intra[count($old)] = true;
+          }
         } else {
           $changed = true;
         }

src/applications/differential/storage/hunk/DifferentialHunk.php

@@ -40,13 +40,49 @@ final class DifferentialHunk extends DifferentialDAO {
   final private function makeContent($exclude) {
     $results = array();
     $lines = explode("\n", $this->changes);
+
+    // NOTE: To determine whether the recomposed file should have a trailing
+    // newline, we look for a "\ No newline at end of file" line which appears
+    // after a line which we don't exclude. For example, if we're constructing
+    // the "new" side of a diff (excluding "-"), we want to ignore this one:
+    //
+    //    - x
+    //    \ No newline at end of file
+    //    + x
+    //
+    // ...since it's talking about the "old" side of the diff, but interpret
+    // this as meaning we should omit the newline:
+    //
+    //    - x
+    //    + x
+    //    \ No newline at end of file
+
+
+    $use_next_newline = false;
+    $has_newline = true;
     foreach ($lines as $line) {
-      if (isset($line[0]) && $line[0] == $exclude) {
+      if (isset($line[0])) {
-        continue;
+        if ($line[0] == $exclude) {
+          $use_next_newline = false;
+          continue;
+        }
+        if ($line[0] == '\\') {
+          if ($use_next_newline) {
+            $has_newline = false;
+          }
+          continue;
+        }
       }
+      $use_next_newline = true;
       $results[] = substr($line, 1);
     }
-    return implode("\n", $results);
+
+    $possible_newline = '';
+    if ($has_newline) {
+      $possible_newline = "\n";
+    }
+
+    return implode("\n", $results).$possible_newline;
   }
 
 }

src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -127,14 +127,38 @@ final class PhabricatorDifferenceEngine {
       foreach ($entire_file as $k => $line) {
         $entire_file[$k] = ' '.$line;
       }
+
       $len = count($entire_file);
       $entire_file = implode("\n", $entire_file);
 
+      // TODO: If both files were identical but missing newlines, we probably
+      // get this wrong. Unclear if it ever matters.
+
       // This is a bit hacky but the diff parser can handle it.
       $diff = "--- {$old_name}\n".
               "+++ {$new_name}\n".
               "@@ -1,{$len} +1,{$len} @@\n".
               $entire_file."\n";
+    } else {
+      if ($this->ignoreWhitespace) {
+
+        // Under "-bw", `diff` is inconsistent about emitting "\ No newline
+        // at end of file". For instance, a long file with a change in the
+        // middle will emit a contextless "\ No newline..." at the end if a
+        // newline is removed, but not if one is added. A file with a change
+        // at the end will emit the "old" "\ No newline..." block only, even
+        // if the newline was not removed. Since we're ostensibly ignoring
+        // whitespace changes, just drop these lines if they appear anywhere
+        // in the diff.
+
+        $lines = explode("\n", rtrim($diff));
+        foreach ($lines as $key => $line) {
+          if (isset($line[0]) && $line[0] == '\\') {
+            unset($lines[$key]);
+          }
+        }
+        $diff = implode("\n", $lines);
+      }
     }
 
     return $diff;