Fix ArcanistBundle to generate disjoint, minimal hunks

Summary: 'patch' chokes on hunks with too much trailing stuff. 'git apply'
chokes on overlapping hunks. Make them both happy. Write some test cases so I
stop breaking this stuff.

Test Plan:
  - Applied a previously-failing patch via SVN.
  - Applied a previously-failing-then-succeeding patch via Git.
  - Ran unit tests.

Reviewers: jungejason, btrahan, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 1099

src/__phutil_library_map__.php

@@ -15,6 +15,7 @@ phutil_register_library_map(array(
     'ArcanistBaseWorkflow' => 'workflow/base',
     'ArcanistBranchWorkflow' => 'workflow/branch',
     'ArcanistBundle' => 'parser/bundle',
+    'ArcanistBundleTestCase' => 'parser/bundle/__tests__',
     'ArcanistCallConduitWorkflow' => 'workflow/call-conduit',
     'ArcanistCapabilityNotSupportedException' => 'workflow/exception/notsupported',
     'ArcanistChooseInvalidRevisionException' => 'exception',
@@ -104,6 +105,7 @@ phutil_register_library_map(array(
     'ArcanistApacheLicenseLinter' => 'ArcanistLicenseLinter',
     'ArcanistApacheLicenseLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistBranchWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistBundleTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistCallConduitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCommitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCoverWorkflow' => 'ArcanistBaseWorkflow',

src/parser/bundle/ArcanistBundle.php

@@ -296,22 +296,39 @@ class ArcanistBundle {
 
       $hunk_start = max($jj - $context, 0);
 
+
+      // NOTE: There are two tricky considerations here.
+      // We can not generate a patch with overlapping hunks, or 'git apply'
+      // rejects it after 1.7.3.4.
+      // We can not generate a patch with too much trailing context, or
+      // 'patch' rejects it.
+      // So we need to ensure that we generate disjoint hunks, but don't
+      // generate any hunks with too much context.
+
       $old_lines = 0;
       $new_lines = 0;
       $last_change = $jj;
+      $break_here = null;
       for (; $jj < $n; ++$jj) {
         if ($lines[$jj][0] == ' ') {
-          // NOTE: We must look past the context size or we may generate
-          // overlapping hunks. For instance, if we have "context = 3" and four
-          // unchanged lines between hunks, we'll include unchanged lines 1, 2,
-          // 3 in the first hunk and 2, 3, and 4 in the second hunk -- that is,
-          // lines 2 and 3 will appear twice in the patch. Some time after
-          // 1.7.3.4, Git stopped cleanly applying patches with overlapping
-          // hunks, so be careful to avoid generating them.
+
+          if ($jj - $last_change > $context) {
+            if ($break_here === null) {
+              // We haven't seen a change in $context lines, so this is a
+              // potential place to break the hunk. However, we need to keep
+              // looking in case there is another change fewer than $context
+              // lines away, in which case we have to merge the hunks.
+              $break_here = $jj;
+            }
+          }
+
           if ($jj - $last_change > (($context + 1) * 2)) {
+            // We definitely aren't going to merge this with the next hunk, so
+            // break out of the loop. We'll end the hunk at $break_here.
             break;
           }
         } else {
+          $break_here = null;
           $last_change = $jj;
           if ($lines[$jj][0] == '-') {
             ++$old_lines;
@@ -321,6 +338,10 @@ class ArcanistBundle {
         }
       }
 
+      if ($break_here !== null) {
+        $jj = $break_here;
+      }
+
       $hunk_length = min($jj, $n) - $hunk_start;
 
       $hunk = new ArcanistDiffHunk();

src/parser/bundle/__tests__/ArcanistBundleTestCase.php

@@ -0,0 +1,70 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class ArcanistBundleTestCase extends ArcanistPhutilTestCase {
+
+  private function loadResource($name) {
+    return Filesystem::readFile($this->getResourcePath($name));
+  }
+
+  private function getResourcePath($name) {
+    return dirname(__FILE__).'/data/'.$name;
+  }
+
+  private function loadDiff($old, $new) {
+    list($err, $stdout) = exec_manual(
+      'diff --unified=65535 --label %s --label %s -- %s %s',
+      'file 9999-99-99',
+      'file 9999-99-99',
+      $this->getResourcePath($old),
+      $this->getResourcePath($new));
+    $this->assertEqual(
+      1,
+      $err,
+      "Expect `diff` to find changes between '{$old}' and '{$new}'.");
+    return $stdout;
+  }
+
+  private function loadOneChangeBundle($old, $new) {
+    $diff = $this->loadDiff($old, $new);
+    return ArcanistBundle::newFromDiff($diff);
+  }
+
+  public function testTrailingContext() {
+    // Diffs need to generate without extra trailing context, or 'patch' will
+    // choke on them.
+    $this->assertEqual(
+      $this->loadResource('trailing-context.diff'),
+      $this->loadOneChangeBundle(
+        'trailing-context.old',
+        'trailing-context.new')->toUnifiedDiff());
+  }
+
+  public function testDisjointHunks() {
+    // Diffs need to generate without overlapping hunks.
+    $this->assertEqual(
+      $this->loadResource('disjoint-hunks.diff'),
+      $this->loadOneChangeBundle(
+        'disjoint-hunks.old',
+        'disjoint-hunks.new')->toUnifiedDiff());
+  }
+
+
+
+
+}

src/parser/bundle/__tests__/__init__.php

@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'parser/bundle');
+phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
+
+phutil_require_module('phutil', 'filesystem');
+phutil_require_module('phutil', 'future/exec');
+
+
+phutil_require_source('ArcanistBundleTestCase.php');

src/parser/bundle/__tests__/data/disjoint-hunks.diff

@@ -0,0 +1,22 @@
+Index: file
+===================================================================
+--- file
++++ file
+@@ -7,14 +7,17 @@
+ g
+ h
+ i
++MMMM
+ j
+ k
+ l
+ m
++MMMM
+ n
+ o
+ p
+ q
++MMMM
+ r
+ s
+ t

src/parser/bundle/__tests__/data/disjoint-hunks.new

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

src/parser/bundle/__tests__/data/disjoint-hunks.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

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

@@ -0,0 +1,12 @@
+Index: file
+===================================================================
+--- file
++++ file
+@@ -7,6 +7,7 @@
+ g
+ h
+ i
++MMMM
+ j
+ k
+ l

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

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

src/parser/bundle/__tests__/data/trailing-context.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