Fix an issue with unified patch generation for terminal newline changes

Summary:
We currently detect the "\" as a change, and may generate a hunk like this, without changes.

  @@ -97,4 +98,4 @@
   mmm
   mmm
   mmm
  \ No newline at end of file

While "git apply" is OK with this, "patch" is not. Instead, don't detect this as a change (it is always accompanied by - / + if it's a real change).

Test Plan: Successfully applied a previously-failing SVN patch to a file without a terminal newline that had nonlocal changes.

Reviewers: 20after4, nh, btrahan

Reviewed By: 20after4

CC: aran, epriestley

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

src/parser/bundle/ArcanistBundle.php

@@ -426,8 +426,18 @@ final class ArcanistBundle {
     $ii = 0;
     $jj = 0;
     while ($ii < $n) {
-      for ($jj = $ii; $jj < $n && $lines[$jj][0] == ' '; ++$jj) {
+      // Skip lines until we find the next line with changes. Note: this skips
-        // Skip lines until we find the first line with changes.
+      // both ' ' (no changes) and '\' (no newline at end of file) lines. If we
+      // don't skip the latter, we may incorrectly generate a terminal hunk
+      // that has no actual change information when a file doesn't have a
+      // terminal newline and not changed near the end of the file. 'patch' will
+      // fail to apply the diff if we generate a hunk that does not actually
+      // contain changes.
+      for ($jj = $ii; $jj < $n; ++$jj) {
+        $char = $lines[$jj][0];
+        if ($char == '-' || $char == '+') {
+          break;
+        }
       }
       if ($jj >= $n) {
         break;

src/parser/bundle/__tests__/ArcanistBundleTestCase.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.
@@ -64,7 +64,14 @@ final class ArcanistBundleTestCase extends ArcanistPhutilTestCase {
         'disjoint-hunks.new')->toUnifiedDiff());
   }
 
-
+  public function testNonlocalTrailingNewline() {
-
+    // Diffs without changes near the end of the file should not generate a
+    // bogus, change-free hunk if the file has no trailing newline.
+    $this->assertEqual(
+      $this->loadResource('trailing-newline.diff'),
+      $this->loadOneChangeBundle(
+        'trailing-newline.old',
+        'trailing-newline.new')->toUnifiedDiff());
+  }
 
 }

src/parser/bundle/__tests__/data/trailing-newline.diff

@@ -0,0 +1,13 @@
+Index: file
+===================================================================
+--- file
++++ file
+@@ -3,7 +3,7 @@
+ c
+ d
+ e
+-f
++QUACK
+ g
+ h
+ i

src/parser/bundle/__tests__/data/trailing-newline.new

@@ -0,0 +1,26 @@
+a
+b
+c
+d
+e
+QUACK
+g
+h
+i
+j
+k
+l
+m
+n
+o
+p
+q
+r
+s
+t
+u
+v
+w
+x
+y
+z

src/parser/bundle/__tests__/data/trailing-newline.old

@@ -0,0 +1,26 @@
+a
+b
+c
+d
+e
+f
+g
+h
+i
+j
+k
+l
+m
+n
+o
+p
+q
+r
+s
+t
+u
+v
+w
+x
+y
+z