diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c6e80dcb..66f53df0 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/parser/bundle/ArcanistBundle.php b/src/parser/bundle/ArcanistBundle.php index 0b75ec85..f306cf10 100644 --- a/src/parser/bundle/ArcanistBundle.php +++ b/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(); diff --git a/src/parser/bundle/__tests__/ArcanistBundleTestCase.php b/src/parser/bundle/__tests__/ArcanistBundleTestCase.php new file mode 100644 index 00000000..9b486949 --- /dev/null +++ b/src/parser/bundle/__tests__/ArcanistBundleTestCase.php @@ -0,0 +1,70 @@ +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()); + } + + + + +} diff --git a/src/parser/bundle/__tests__/__init__.php b/src/parser/bundle/__tests__/__init__.php new file mode 100644 index 00000000..39f7b7b7 --- /dev/null +++ b/src/parser/bundle/__tests__/__init__.php @@ -0,0 +1,16 @@ +