Put some whitespace behaviors back, but only for "diff alignment", not display

Summary:
Depends on D20185. Ref T13161. Fixes T6791.

See some discusison in T13161. I want to move to a world where:

  - whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
  - the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.

D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.

However, a second aspect to this is how the diff is "aligned". If this file:

```
A
```

..is changed to this file:

```
X
A
Y
Z
```

...diff tools will generally produce this diff:

```
+ X
  A
+ Y
+ Z
```

This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:

```
       1 X
1 A    2 A
       3 Y
       4 Z
```

However, if the new file looks like this instead:

```
X
A'
Y
Z
```

...we get a diff like this:

```
- A
+ X
+ A'
+ Y
+ Z
```

This one aligns like this:

```
1 A
        1 X
        2 A'
        3 Y
        4 Z
```

This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:

```
        1 X
1 A     2 A'
        3 Y
        4 Z
```

Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.

Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.

This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:

  - Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
  - Normalize the files (for example, by removing whitespace from each line).
  - Diff the normalized files to produce a second diff.
  - Parse that diff.
  - Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.

Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).

Test Plan:
{F6217133}

(Also, fix a unit test.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T6791

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

Author: epriestley

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php

@@ -14,6 +14,10 @@ public function testParseRender() {
       }
       $data = Filesystem::readFile($dir.$file);
 
+      // Strip trailing "~" characters from inputs so they may contain
+      // trailing whitespace.
+      $data = preg_replace('/~$/m', '', $data);
+
       $opt_file = $dir.$file.'.options';
       if (Filesystem::pathExists($opt_file)) {
         $options = Filesystem::readFile($opt_file);

src/applications/differential/__tests__/data/generated.diff

@@ -4,7 +4,7 @@ index 5dcff7f..eff82ef 100644
 +++ b/GENERATED
 @@ -1,4 +1,4 @@
  @generated
-
+ ~
 -This is a generated file.
 +This is a generated file, full of generated code.
 

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -643,6 +643,9 @@ private function process() {
 
     $hunk_parser = new DifferentialHunkParser();
     $hunk_parser->parseHunksForLineData($changeset->getHunks());
+
+    $this->realignDiff($changeset, $hunk_parser);
+
     $hunk_parser->reparseHunksForSpecialAttributes();
 
     $unchanged = false;
@@ -1366,4 +1369,51 @@ private function getOffset(array $map, $line) {
     return $key;
   }
 
+  private function realignDiff(
+    DifferentialChangeset $changeset,
+    DifferentialHunkParser $hunk_parser) {
+    // Normalizing and realigning the diff depends on rediffing the files, and
+    // we currently need complete representations of both files to do anything
+    // reasonable. If we only have parts of the files, skip realignment.
+
+    // We have more than one hunk, so we're definitely missing part of the file.
+    $hunks = $changeset->getHunks();
+    if (count($hunks) !== 1) {
+      return null;
+    }
+
+    // The first hunk doesn't start at the beginning of the file, so we're
+    // missing some context.
+    $first_hunk = head($hunks);
+    if ($first_hunk->getOldOffset() != 1 || $first_hunk->getNewOffset() != 1) {
+      return null;
+    }
+
+    $old_file = $changeset->makeOldFile();
+    $new_file = $changeset->makeNewFile();
+    if ($old_file === $new_file) {
+      // If the old and new files are exactly identical, the synthetic
+      // diff below will give us nonsense and whitespace modes are
+      // irrelevant anyway. This occurs when you, e.g., copy a file onto
+      // itself in Subversion (see T271).
+      return null;
+    }
+
+
+    $engine = id(new PhabricatorDifferenceEngine())
+      ->setNormalize(true);
+
+    $normalized_changeset = $engine->generateChangesetFromFileContent(
+      $old_file,
+      $new_file);
+
+    $type_parser = new DifferentialHunkParser();
+    $type_parser->parseHunksForLineData($normalized_changeset->getHunks());
+
+    $hunk_parser->setNormalized(true);
+    $hunk_parser->setOldLineTypeMap($type_parser->getOldLineTypeMap());
+    $hunk_parser->setNewLineTypeMap($type_parser->getNewLineTypeMap());
+  }
+
+
 }

src/applications/differential/parser/DifferentialHunkParser.php

@@ -7,6 +7,7 @@ final class DifferentialHunkParser extends Phobject {
   private $intraLineDiffs;
   private $depthOnlyLines;
   private $visibleLinesMask;
+  private $normalized;
 
   /**
    * Get a map of lines on which hunks start, other than line 1. This
@@ -124,6 +125,15 @@ public function getDepthOnlyLines() {
     return $this->depthOnlyLines;
   }
 
+  public function setNormalized($normalized) {
+    $this->normalized = $normalized;
+    return $this;
+  }
+
+  public function getNormalized() {
+    return $this->normalized;
+  }
+
   public function getIsDeleted() {
     foreach ($this->getNewLines() as $line) {
       if ($line) {
@@ -252,6 +262,8 @@ public function reparseHunksForSpecialAttributes() {
     $this->setOldLines($rebuild_old);
     $this->setNewLines($rebuild_new);
 
+    $this->updateChangeTypesForNormalization();
+
     return $this;
   }
 
@@ -753,4 +765,55 @@ private function getCharacterCountForVisualWhitespace(
     return $character_depth;
   }
 
+  private function updateChangeTypesForNormalization() {
+    if (!$this->getNormalized()) {
+      return;
+    }
+
+    // If we've parsed based on a normalized diff alignment, we may currently
+    // believe some lines are unchanged when they have actually changed. This
+    // happens when:
+    //
+    //   - a line changes;
+    //   - the change is a kind of change we normalize away when aligning the
+    //     diff, like an indentation change;
+    //   - we normalize the change away to align the diff; and so
+    //   - the old and new copies of the line are now aligned in the new
+    //     normalized diff.
+    //
+    // Then we end up with an alignment where the two lines that differ only
+    // in some some trivial way are aligned. This is great, and exactly what
+    // we're trying to accomplish by doing all this alignment stuff in the
+    // first place.
+    //
+    // However, in this case the correctly-aligned lines will be incorrectly
+    // marked as unchanged because the diff alorithm was fed normalized copies
+    // of the lines, and these copies truly weren't any different.
+    //
+    // When lines are aligned and marked identical, but they're not actually
+    // identcal, we now mark them as changed. The rest of the processing will
+    // figure out how to render them appropritely.
+
+    $new = $this->getNewLines();
+    $old = $this->getOldLines();
+    foreach ($old as $key => $o) {
+      $n = $new[$key];
+
+      if (!$o || !$n) {
+        continue;
+      }
+
+      if ($o['type'] === null && $n['type'] === null) {
+        if ($o['text'] !== $n['text']) {
+          $old[$key]['type'] = '-';
+          $new[$key]['type'] = '+';
+        }
+      }
+    }
+
+    $this->setOldLines($old);
+    $this->setNewLines($new);
+  }
+
+
 }

src/infrastructure/diff/PhabricatorDifferenceEngine.php

@@ -12,6 +12,7 @@ final class PhabricatorDifferenceEngine extends Phobject {
 
   private $oldName;
   private $newName;
+  private $normalize;
 
 
 /* -(  Configuring the Engine  )--------------------------------------------- */
@@ -43,6 +44,16 @@ public function setNewName($new_name) {
   }
 
 
+  public function setNormalize($normalize) {
+    $this->normalize = $normalize;
+    return $this;
+  }
+
+  public function getNormalize() {
+    return $this->normalize;
+  }
+
+
 /* -(  Generating Diffs  )--------------------------------------------------- */
 
 
@@ -71,6 +82,12 @@ public function generateRawDiffFromFileContent($old, $new) {
     $options[] = '-L';
     $options[] = $new_name;
 
+    $normalize = $this->getNormalize();
+    if ($normalize) {
+      $old = $this->normalizeFile($old);
+      $new = $this->normalizeFile($new);
+    }
+
     $old_tmp = new TempFile();
     $new_tmp = new TempFile();
 
@@ -129,4 +146,27 @@ public function generateChangesetFromFileContent($old, $new) {
     return head($diff->getChangesets());
   }
 
+  private function normalizeFile($corpus) {
+    // We can freely apply any other transformations we want to here: we have
+    // no constraints on what we need to preserve. If we normalize every line
+    // to "cat", the diff will still work, the alignment of the "-" / "+"
+    // lines will just be very hard to read.
+
+    // In general, we'll make the diff better if we normalize two lines that
+    // humans think are the same.
+
+    // We'll make the diff worse if we normalize two lines that humans think
+    // are different.
+
+
+    // Strip all whitespace present anywhere in the diff, since humans never
+    // consider whitespace changes to alter the line into a "different line"
+    // even when they're semantic (e.g., in a string constant). This covers
+    // indentation changes, trailing whitepspace, and formatting changes
+    // like "+/-".
+    $corpus = preg_replace('/[ \t]/', '', $corpus);
+
+    return $corpus;
+  }
+
 }