From d57ea379c6cb8de17ca118561aaca702b524301e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 3 Nov 2011 13:42:19 -0700
Subject: [PATCH] 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 | 27 +++++++------------
.../api/subversion/ArcanistSubversionAPI.php | 8 +++++-
src/workflow/base/ArcanistBaseWorkflow.php | 26 +++++++++++++-----
src/workflow/lint/ArcanistLintWorkflow.php | 17 +++++-------
4 files changed, 43 insertions(+), 35 deletions(-)
diff --git a/src/lint/engine/base/ArcanistLintEngine.php b/src/lint/engine/base/ArcanistLintEngine.php
index 0cc6c043..308cdd50 100644
--- a/src/lint/engine/base/ArcanistLintEngine.php
+++ b/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;
}
diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php
index 522ed172..e73a1e37 100644
--- a/src/repository/api/subversion/ArcanistSubversionAPI.php
+++ b/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(),
diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php
index d0008fec..ad198f29 100644
--- a/src/workflow/base/ArcanistBaseWorkflow.php
+++ b/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);
}
diff --git a/src/workflow/lint/ArcanistLintWorkflow.php b/src/workflow/lint/ArcanistLintWorkflow.php
index ccb1c3ed..a4831304 100644
--- a/src/workflow/lint/ArcanistLintWorkflow.php
+++ b/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'));
}
}