Fix "arc lint" exploding on new directories

Summary:
D1061 introduced a 'text file' check, but it fails under SVN for new
directories.

  - Revert D1061 (This reverts commit b2cd18252701be2093b52652fb3d1d94c5df571e.)
  - Make getChangedLines() return null to indicate that the operation doesn't
make sense. I think this was the intent of the code in the lint engine.
  - Fix a bug where running "arc lint" on a change in an SVN working copy from a
subdirectory would fail.
  - Fix a bug where warnings with no line information were incorrectly
discarded.

Test Plan:
  - Ran "arc lint" in an SVN working copy with a new directory (no failure).
  - Forced FilenameLinter to always raise a warning. Added a binary file and ran
"arc lint". The warning was reported for the new binary file, a new text file,
and a new directory.

Reviewers: jungejason, andrewjcg, nh, tuomaspelkonen, aran

Reviewed By: andrewjcg

CC: aran, andrewjcg, epriestley

Differential Revision: 1076

src/lint/engine/base/ArcanistLintEngine.php

@@ -68,7 +68,6 @@ abstract class ArcanistLintEngine {
   private $minimumSeverity = ArcanistLintSeverity::SEVERITY_DISABLED;
 
   private $changedLines = array();
-  private $textChanges = array();
   private $commitHookMode = false;
 
   public function __construct() {
@@ -93,8 +92,12 @@ abstract class ArcanistLintEngine {
     return $this->paths;
   }
 
-  public function setPathChangedLines($path, array $changed) {
-    $this->changedLines[$path] = array_fill_keys($changed, true);
+  public function setPathChangedLines($path, $changed) {
+    if ($changed === null) {
+      $this->changedLines[$path] = null;
+    } else {
+      $this->changedLines[$path] = array_fill_keys($changed, true);
+    }
     return $this;
   }
 
@@ -102,15 +105,6 @@ abstract class ArcanistLintEngine {
     return idx($this->changedLines, $path);
   }
 
-  public function setTextChange($path) {
-    $this->textChanges[$path] = true;
-    return $this;
-  }
-
-  public function isTextChange($path) {
-    return idx($this->textChanges, $path, false);
-  }
-
   public function setFileData($data) {
     $this->fileData = $data + $this->fileData;
     return $this;
@@ -205,12 +199,11 @@ abstract class ArcanistLintEngine {
         }
         // When a user runs "arc lint", we default to raising only warnings on
         // lines they have changed (errors are still raised anywhere in the
-        // file).
-        $text_change = $this->isTextChange($message->getPath());
+        // file). The list of $changed lines may be null, to indicate that the
+        // path is a directory or a binary file so we should not exclude
+        // warnings.
         $changed = $this->getPathChangedLines($message->getPath());
-        if ($text_change === true &&
-            $changed !== null &&
-            !$message->isError()) {
+        if ($changed !== null && !$message->isError() && $message->getLine()) {
           if (empty($changed[$message->getLine()])) {
             continue;
           }

src/repository/api/subversion/ArcanistSubversionAPI.php

@@ -230,7 +230,13 @@ class ArcanistSubversionAPI extends ArcanistRepositoryAPI {
 
   public function buildDiffFuture($path) {
     // The "--depth empty" flag prevents us from picking up changes in
-    // children when we run 'diff' against a directory.
+    // children when we run 'diff' against a directory. Specifically, when a
+    // user has added or modified some directory "example/", we want to return
+    // ONLY changes to that directory when given it as a path. If we run
+    // without "--depth empty", svn will give us changes to the directory
+    // itself (such as property changes) and also give us changes to any
+    // files within the directory (basically, implicit recursion). We don't
+    // want that, so prevent recursive diffing.
     return new ExecFuture(
       '(cd %s; svn diff --depth empty --diff-cmd diff -x -U%d %s)',
       $this->getPath(),

src/workflow/base/ArcanistBaseWorkflow.php

@@ -754,17 +754,29 @@ class ArcanistBaseWorkflow {
     return $bundle;
   }
 
-  protected function isTextChange($path) {
-    $change = $this->getChange($path);
-    return $change->getFileType() == ArcanistDiffChangeType::FILE_TEXT;
-  }
-
+  /**
+   * Return a list of lines changed by the current diff, or ##null## if the
+   * change list is meaningless (for example, because the path is a directory
+   * or binary file).
+   *
+   * @param string      Path within the repository.
+   * @param string      Change selection mode (see ArcanistDiffHunk).
+   * @return list|null  List of changed line numbers, or null to indicate that
+   *                    the path is not a line-oriented text file.
+   */
   protected function getChangedLines($path, $mode) {
-    if (is_dir($path)) {
-      return array();
+    $repository_api = $this->getRepositoryAPI();
+    $full_path = $repository_api->getPath($path);
+    if (is_dir($full_path)) {
+      return null;
     }
 
     $change = $this->getChange($path);
+
+    if ($change->getFileType() !== ArcanistDiffChangeType::FILE_TEXT) {
+      return null;
+    }
+
     $lines = $change->getChangedLines($mode);
     return array_keys($lines);
   }

src/workflow/lint/ArcanistLintWorkflow.php

@@ -170,19 +170,16 @@ EOTEXT
     }
 
     // Propagate information about which lines changed to the lint engine.
-    // This is used so that the lint engine can drop messages concerning
-    // lines that weren't in the change.
+    // This is used so that the lint engine can drop warning messages
+    // concerning lines that weren't in the change.
     $engine->setPaths($paths);
     if (!$should_lint_all) {
       foreach ($paths as $path) {
-        // Explicitly flag text changes, as line information doesn't apply
-        // to non-text files.
-        if ($this->isTextChange($path)) {
-          $engine->setTextChange($path);
-          $engine->setPathChangedLines(
-            $path,
-            $this->getChangedLines($path, 'new'));
-        }
+        // Note that getChangedLines() returns null to indicate that a file
+        // is binary or a directory (i.e., changed lines are not relevant).
+        $engine->setPathChangedLines(
+          $path,
+          $this->getChangedLines($path, 'new'));
       }
     }