From 4d7b44183425af3f4494b1c80a88b955d42e2d04 Mon Sep 17 00:00:00 2001
From: vrana <jakubv@fb.com>
Date: Wed, 21 Nov 2012 12:30:38 -0800
Subject: [PATCH] Don't skip even lines in copied code detector

Summary: PHP 3 basics: `each()` advances internal pointer so calling `next()` is wrong.

Test Plan: New test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4011
---
 src/__phutil_library_map__.php                |  2 +
 .../differential/storage/DifferentialDiff.php |  2 +-
 .../__tests__/DifferentialDiffTestCase.php    | 18 +++++++
 .../storage/__tests__/diff/lint_engine.diff   | 50 +++++++++++++++++++
 4 files changed, 71 insertions(+), 1 deletion(-)
 create mode 100644 src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php
 create mode 100644 src/applications/differential/storage/__tests__/diff/lint_engine.diff

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 8dc76976b5..0fef1c616f 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -245,6 +245,7 @@ phutil_register_library_map(array(
     'DifferentialDiffCreateController' => 'applications/differential/controller/DifferentialDiffCreateController.php',
     'DifferentialDiffProperty' => 'applications/differential/storage/DifferentialDiffProperty.php',
     'DifferentialDiffTableOfContentsView' => 'applications/differential/view/DifferentialDiffTableOfContentsView.php',
+    'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php',
     'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php',
     'DifferentialException' => 'applications/differential/exception/DifferentialException.php',
     'DifferentialExceptionMail' => 'applications/differential/mail/DifferentialExceptionMail.php',
@@ -1502,6 +1503,7 @@ phutil_register_library_map(array(
     'DifferentialDiffCreateController' => 'DifferentialController',
     'DifferentialDiffProperty' => 'DifferentialDAO',
     'DifferentialDiffTableOfContentsView' => 'AphrontView',
+    'DifferentialDiffTestCase' => 'ArcanistPhutilTestCase',
     'DifferentialDiffViewController' => 'DifferentialController',
     'DifferentialException' => 'Exception',
     'DifferentialExceptionMail' => 'DifferentialMail',
diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php
index 38f64733c3..3c43233e49 100644
--- a/src/applications/differential/storage/DifferentialDiff.php
+++ b/src/applications/differential/storage/DifferentialDiff.php
@@ -204,7 +204,7 @@ final class DifferentialDiff extends DifferentialDAO {
       $copies = array();
       foreach ($changeset->getHunks() as $hunk) {
         $added = array_map('trim', $hunk->getAddedLines());
-        for (reset($added); list($line, $code) = each($added); next($added)) {
+        for (reset($added); list($line, $code) = each($added); ) {
           if (isset($map[$code])) { // We found a long matching line.
             $best_length = 0;
             foreach ($map[$code] as $val) { // Explore all candidates.
diff --git a/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php b/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php
new file mode 100644
index 0000000000..c287b5e0f0
--- /dev/null
+++ b/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php
@@ -0,0 +1,18 @@
+<?php
+
+final class DifferentialDiffTestCase extends ArcanistPhutilTestCase {
+
+  public function testDetectCopiedCode() {
+    $root = dirname(__FILE__).'/diff/';
+    $parser = new ArcanistDiffParser();
+
+    $diff = DifferentialDiff::newFromRawChanges(
+      $parser->parseDiff(Filesystem::readFile($root.'lint_engine.diff')));
+    $copies = idx(head($diff->getChangesets())->getMetadata(), 'copy:lines');
+
+    $this->assertEqual(
+      array_combine(range(237, 252), range(167, 182)),
+      ipull($copies, 1));
+  }
+
+}
diff --git a/src/applications/differential/storage/__tests__/diff/lint_engine.diff b/src/applications/differential/storage/__tests__/diff/lint_engine.diff
new file mode 100644
index 0000000000..5b5262caad
--- /dev/null
+++ b/src/applications/differential/storage/__tests__/diff/lint_engine.diff
@@ -0,0 +1,50 @@
+diff --git a/ArcanistLintEngine.php b/ArcanistLintEngine.php
+index 41e1153..255b049 100644
+--- a/ArcanistLintEngine.php
++++ b/ArcanistLintEngine.php
+@@ -165,22 +165,6 @@ abstract class ArcanistLintEngine {
+     $stopped = array();
+     $linters = $this->buildLinters();
+
+-    if (!$linters) {
+-      throw new ArcanistNoEffectException("No linters to run.");
+-    }
+-
+-    $have_paths = false;
+-    foreach ($linters as $linter) {
+-      if ($linter->getPaths()) {
+-        $have_paths = true;
+-        break;
+-      }
+-    }
+-
+-    if (!$have_paths) {
+-      throw new ArcanistNoEffectException("No paths are lintable.");
+-    }
+-
+     $exceptions = array();
+     foreach ($linters as $linter_name => $linter) {
+       try {
+@@ -251,6 +235,22 @@ abstract class ArcanistLintEngine {
+       }
+     }
+
++    if (!$linters) {
++      throw new ArcanistNoEffectException("No linters to run.");
++    }
++
++    $have_paths = false;
++    foreach ($linters as $linter) {
++      if ($linter->getPaths()) {
++        $have_paths = true;
++        break;
++      }
++    }
++
++    if (!$have_paths) {
++      throw new ArcanistNoEffectException("No paths are lintable.");
++    }
++
+     if ($exceptions) {
+       throw new PhutilAggregateException('Some linters failed:', $exceptions);
+     }