From c243cbbd9fc701ca9555672ea5c1666ea154898f Mon Sep 17 00:00:00 2001
From: Chris Burroughs <chris.burroughs@gmail.com>
Date: Wed, 28 Dec 2016 17:43:18 -0500
Subject: [PATCH 01/34] tighten remote URI error handling with idiosyncratic
 remote names

Summary:
`git ls-remote` has an unusual way to indicate a URL was not
found: echoing back user input
```
$ git ls-remote --get-url does_not_exist
does_not_exist
$ echo $?
0

```

`getRemoteURI` handles checking for remotes other than 'origin', but
the error handling always matched against the string 'origin'
regardless of remote name.

Test Plan:
With a git config along the lines of:
```
[remote "my_special_name"]
        url = ssh://secure@secure.phabricator.com/diffusion/ARC/arcanist.git
        fetch = +refs/heads/*:refs/remotes/my_special_name/*
[branch "master"]
        remote = github
        merge = refs/heads/master
[remote "github"]
         # url = git@github.com:phacility/arcanist.git
         fetch = +refs/heads/*:refs/remotes/github/*
```

and running in a branch tracking `master` (github).  `arc which` would
(without this diff) show:
```
The remote URI for this working copy is "github".
```
With this diff, `arc which` correctly shows:
```
Unable to determine the remote URI for this repository.
```

When diffing against a tracking branch with a propertly configured
remote (the happy path), `arc which` still correctly identifies the
remote URI:
```
The remote URI for this working copy is
"ssh://secure@secure.phabricator.com/diffusion/ARC/arcanist.git".
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, chad, epriestley

Differential Revision: https://secure.phabricator.com/D17110
---
 src/repository/api/ArcanistGitAPI.php | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index 57078dab..abe0a220 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -544,8 +544,10 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     }
 
     $uri = rtrim($stdout);
-    // 'origin' is what ls-remote outputs if no origin remote URI exists
-    if (!$uri || $uri === 'origin') {
+    // ls-remote echos the remote name (ie 'origin') if no remote URI is found
+    // TODO: In 2.7.0 (circa 2016) git introduced `git remote get-url`
+    // with saner error handling.
+    if (!$uri || $uri === $remote) {
       return null;
     }
 

From ade25facfdf22aed1c1e20fed3e58e60c0be3c2b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 5 Jan 2017 13:55:34 -0800
Subject: [PATCH 02/34] Fix a bad interaction between "arc diff --reviewers"
 and "the first line of a message is always a title"

Summary:
Fixes T12069. We implement "arc diff --reviewers" (and "--cc") by parsing a faux message with "Reviewers: ...".

After D17122, the first line of the message is always interpreted as a title, so the text ends up in the message body.

Instead, use a placeholder title so these fields are never initial fields.

Test Plan: Ran `arc diff --reviewers dog`, got only one "Reviewers" field.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12069

Differential Revision: https://secure.phabricator.com/D17147
---
 src/workflow/ArcanistDiffWorkflow.php | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index 7834f4b8..c7f2ff0a 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -1962,7 +1962,13 @@ EOTEXT
       $faux_message[] = pht('CC: %s', $this->getArgument('cc'));
     }
 
+    // See T12069. After T10312, the first line of a message is always parsed
+    // as a title. Add a placeholder so "Reviewers" and "CC" are never the
+    // first line.
+    $placeholder_title = pht('<placeholder>');
+
     if ($faux_message) {
+      array_unshift($faux_message, $placeholder_title);
       $faux_message = implode("\n\n", $faux_message);
       $local = array(
         '(Flags)     ' => array(
@@ -2034,6 +2040,10 @@ EOTEXT
         continue;
       }
 
+      if ($title === $placeholder_title) {
+        continue;
+      }
+
       if (!isset($result['title'])) {
         // We don't have a title yet, so use this one.
         $result['title'] = $title;

From 224986af634e1dfc40916e5baee76897db4c907f Mon Sep 17 00:00:00 2001
From: Josh Cox <joshcox@uberatc.com>
Date: Tue, 7 Feb 2017 16:03:48 -0500
Subject: [PATCH 03/34] Provide a better error message when an invalid ID is
 given to arc patch

Summary:
Fixes T8937. Previously when running `arc patch D9999999999` or `arc export --revision 99999999` with a non-existent diff or revision ID you would get a rather unhelpful error message. Now you'll get a slightly more helpful error message:
```
$ arc patch D99999999
Exception
Couldn't find a revision or diff that matches the given ID
(Run with `--trace` for a full exception trace.)
```

Test Plan: Ran arc patch with a valid revision and saw it patch successfully. Ran again with an invalid revision, saw the error message.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley, yelirekim

Maniphest Tasks: T8937

Differential Revision: https://secure.phabricator.com/D17325
---
 src/workflow/ArcanistWorkflow.php | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
index b4b105d6..2365083a 100644
--- a/src/workflow/ArcanistWorkflow.php
+++ b/src/workflow/ArcanistWorkflow.php
@@ -1196,6 +1196,14 @@ abstract class ArcanistWorkflow extends Phobject {
     $future = $conduit->callMethod('differential.querydiffs', $params);
     $diff = head($future->resolve());
 
+    if ($diff == null) {
+      throw new Exception(
+        phutil_console_wrap(
+          pht("The diff or revision you specified is either invalid or you ".
+          "don't have permission to view it."))
+      );
+    }
+
     $changes = array();
     foreach ($diff['changes'] as $changedict) {
       $changes[] = ArcanistDiffChange::newFromDictionary($changedict);

From f3037bf216e52e14f01e5c93158c6c767b94696d Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@dropbox.com>
Date: Wed, 8 Feb 2017 16:24:20 -0800
Subject: [PATCH 04/34] [git] Override `diff.submodule` so `git diff` output is
 always parseable

Test Plan:
Removed a submodule with `diff.submodule` set to `log`, saw
`arc diff` error; with this change, it no longer does.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T10881

Differential Revision: https://secure.phabricator.com/D17327
---
 src/repository/api/ArcanistGitAPI.php | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index abe0a220..8efe1bce 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -446,6 +446,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       // would ship up the binaries for 'arc patch' but display the textconv
       // output in the visual diff.
       '--no-textconv',
+      // Provide a standard view of submodule changes; the 'log' and 'diff'
+      // values do not parse by the diff parser.
+      '--submodule=short',
     );
     return implode(' ', $options);
   }

From bc9b70508edf224a15a3bdb45998f7fd75847765 Mon Sep 17 00:00:00 2001
From: Aviv Eyal <avivey@gmail.com>
Date: Wed, 15 Feb 2017 13:38:57 +0000
Subject: [PATCH 05/34] arc set-config: warn about unknown keys

Summary: See T12266. Warn when the user tries to set a value we will probably not read.

Test Plan: `arc set-config foo bar`, see fancy warning.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17357
---
 src/workflow/ArcanistSetConfigWorkflow.php | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/workflow/ArcanistSetConfigWorkflow.php b/src/workflow/ArcanistSetConfigWorkflow.php
index 9d9b099d..7bad38c1 100644
--- a/src/workflow/ArcanistSetConfigWorkflow.php
+++ b/src/workflow/ArcanistSetConfigWorkflow.php
@@ -69,6 +69,16 @@ EOTEXT
 
     $settings = new ArcanistSettings();
 
+    $console = PhutilConsole::getConsole();
+
+    if (!$settings->getHelp($key)) {
+      $warn = pht(
+        'The configuration key \'%s\' is not recognized by arc. It may '.
+        'be misspelled or out of date.',
+        $key);
+      $console->writeErr("**%s:** %s\n", pht('Warning'), $warn);
+    }
+
     $old = null;
     if (array_key_exists($key, $config)) {
       $old = $config[$key];
@@ -85,12 +95,12 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        echo pht(
+        $console->writeOut(
           "Deleted key '%s' from %s config.\n",
           $key,
           $which);
       } else {
-        echo pht(
+        $console->writeOut(
           "Deleted key '%s' from %s config (was %s).\n",
           $key,
           $which,
@@ -110,13 +120,13 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        echo pht(
+        $console->writeOut(
           "Set key '%s' = %s in %s config.\n",
           $key,
           $val,
           $which);
       } else {
-        echo pht(
+        $console->writeOut(
           "Set key '%s' = %s in %s config (was %s).\n",
           $key,
           $val,

From 13596cd10fbe8bc98956ce4cb47c15a0936cbc78 Mon Sep 17 00:00:00 2001
From: Aviv Eyal <avivey@gmail.com>
Date: Thu, 16 Feb 2017 11:08:45 +0000
Subject: [PATCH 06/34] Add back pht()s and tsprintf()s to arg set-config

Summary: See D17357

Test Plan: invoke and still see output in English?

Reviewers: #blessed_reviewers, joshuaspence, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17362
---
 src/workflow/ArcanistSetConfigWorkflow.php | 31 +++++++++++++---------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/workflow/ArcanistSetConfigWorkflow.php b/src/workflow/ArcanistSetConfigWorkflow.php
index 7bad38c1..75b4334f 100644
--- a/src/workflow/ArcanistSetConfigWorkflow.php
+++ b/src/workflow/ArcanistSetConfigWorkflow.php
@@ -72,11 +72,14 @@ EOTEXT
     $console = PhutilConsole::getConsole();
 
     if (!$settings->getHelp($key)) {
-      $warn = pht(
-        'The configuration key \'%s\' is not recognized by arc. It may '.
-        'be misspelled or out of date.',
-        $key);
-      $console->writeErr("**%s:** %s\n", pht('Warning'), $warn);
+      $warning = tsprintf(
+        "**%s:** %s\n",
+        pht('Warning'),
+        pht(
+          'The configuration key "%s" is not recognized by arc. It may '.
+          'be misspelled or out of date.',
+          $key));
+      $console->writeErr('%s', $warning);
     }
 
     $old = null;
@@ -95,17 +98,18 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        $console->writeOut(
-          "Deleted key '%s' from %s config.\n",
+        $message = pht(
+          'Deleted key "%s" from %s config.',
           $key,
           $which);
       } else {
-        $console->writeOut(
-          "Deleted key '%s' from %s config (was %s).\n",
+        $message = pht(
+          'Deleted key "%s" from %s config (was %s).',
           $key,
           $which,
           $old);
       }
+      $console->writeOut('%s', tsprintf("%s\n", $message));
     } else {
       $val = $settings->willWriteValue($key, $val);
 
@@ -120,19 +124,20 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        $console->writeOut(
-          "Set key '%s' = %s in %s config.\n",
+        $message = pht(
+          'Set key "%s" = %s in %s config.',
           $key,
           $val,
           $which);
       } else {
-        $console->writeOut(
-          "Set key '%s' = %s in %s config (was %s).\n",
+        $message = pht(
+          'Set key "%s" = %s in %s config (was %s).',
           $key,
           $val,
           $which,
           $old);
       }
+      $console->writeOut('%s', tsprintf("%s\n", $message));
     }
 
     return 0;

From 460b0e46eea0b380dcb04154ead43f0adc7a710d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 16 Feb 2017 05:08:51 -0800
Subject: [PATCH 07/34] Fix a property name collision in
 ArcanistHgServerChannel

Summary:
See D2665. Here, `channel` is the Mercurial wire protocol channel name ("o" = "output", "e" = "error", etc), not the underlying channel from `PhutilProxyChannel`.

Rename the property so there's no collision.

Test Plan:
```
$ ../../core/lib/arcanist/scripts/hgdaemon/hgdaemon_client.php . --trace --skip-hello
array(3) {
  [0]=>
  int(0)
  [1]=>
  string(103) "capabilities: getencoding runcommand
encoding: UTF-8
pid: 73263e20cf21273d50f1f66cab6e0f7c74f4864e67f0f"
  [2]=>
  string(0) ""
}

Executed in 30647 us.
```

Reviewers: vrana, chad

Reviewed By: vrana

Differential Revision: https://secure.phabricator.com/D17366
---
 src/hgdaemon/ArcanistHgServerChannel.php | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/hgdaemon/ArcanistHgServerChannel.php b/src/hgdaemon/ArcanistHgServerChannel.php
index 1850c9af..22f6b4ab 100644
--- a/src/hgdaemon/ArcanistHgServerChannel.php
+++ b/src/hgdaemon/ArcanistHgServerChannel.php
@@ -54,6 +54,7 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel {
   private $mode                   = self::MODE_CHANNEL;
   private $byteLengthOfNextChunk  = 1;
   private $buf                    = '';
+  private $outputChannel;
 
 
 /* -(  Protocol Implementation  )-------------------------------------------- */
@@ -137,7 +138,7 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel {
           // 'output', 'error', 'result' or 'debug' respectively. This is a
           // single byte long. Next, we'll expect a length.
 
-          $this->channel = $chunk;
+          $this->outputChannel = $chunk;
           $this->byteLengthOfNextChunk = 4;
           $this->mode = self::MODE_LENGTH;
           break;
@@ -153,11 +154,11 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel {
           // given length. We produce a message from the channel and the data
           // and return it. Next, we expect another channel name.
 
-          $message = array($this->channel, $chunk);
+          $message = array($this->outputChannel, $chunk);
 
           $this->byteLengthOfNextChunk = 1;
           $this->mode = self::MODE_CHANNEL;
-          $this->channel = null;
+          $this->outputChannel = null;
 
           $messages[] = $message;
           break;

From d0353d238113bfc1c1a3341288e29a7d26128a52 Mon Sep 17 00:00:00 2001
From: Jakub Vrana <jakub@vrana.cz>
Date: Thu, 16 Feb 2017 13:54:43 +0000
Subject: [PATCH 08/34] Fix errors found by PHPStan

Test Plan:
Ran `phpstan analyze -a autoload.php arcanist/src` with `autoload.php` containing:

  <?php
  require_once 'libphutil/src/__phutil_library_init__.php';
  require_once 'arcanist/src/__phutil_library_init__.php';

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17367
---
 src/land/ArcanistGitLandEngine.php                         | 2 +-
 src/lint/linter/ArcanistBaseXHPASTLinter.php               | 2 +-
 .../linter/__tests__/ArcanistClosureLinterTestCase.php     | 7 +++++--
 .../parser/__tests__/XUnitTestResultParserTestCase.php     | 4 ++--
 src/workflow/ArcanistLandWorkflow.php                      | 4 +---
 src/workflow/ArcanistLintWorkflow.php                      | 2 +-
 src/workflow/ArcanistStartWorkflow.php                     | 2 +-
 src/workflow/ArcanistStopWorkflow.php                      | 2 +-
 src/workflow/ArcanistTasksWorkflow.php                     | 2 +-
 src/workflow/ArcanistUnitWorkflow.php                      | 2 +-
 src/workflow/ArcanistWorkflow.php                          | 4 +---
 11 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php
index 12b90548..6b0a38cd 100644
--- a/src/land/ArcanistGitLandEngine.php
+++ b/src/land/ArcanistGitLandEngine.php
@@ -59,7 +59,7 @@ final class ArcanistGitLandEngine
 
   public function __destruct() {
     if ($this->restoreWhenDestroyed) {
-      $this->writeWARN(
+      $this->writeWarn(
         pht('INTERRUPTED!'),
         pht('Restoring working copy to its original state.'));
 
diff --git a/src/lint/linter/ArcanistBaseXHPASTLinter.php b/src/lint/linter/ArcanistBaseXHPASTLinter.php
index 72a9bd7e..f0f44827 100644
--- a/src/lint/linter/ArcanistBaseXHPASTLinter.php
+++ b/src/lint/linter/ArcanistBaseXHPASTLinter.php
@@ -189,7 +189,7 @@ abstract class ArcanistBaseXHPASTLinter extends ArcanistFutureLinter {
    * Get a path's parse exception from the responsible linter.
    *
    * @param   string          Path to retrieve exception for.
-   * @return  Exeption|null   Parse exception, if available.
+   * @return  Exception|null  Parse exception, if available.
    * @task sharing
    */
   final protected function getXHPASTExceptionForPath($path) {
diff --git a/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php b/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php
index 50c01ca5..f02dc21b 100644
--- a/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php
+++ b/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php
@@ -3,11 +3,14 @@
 final class ArcanistClosureLinterTestCase
   extends ArcanistExternalLinterTestCase {
 
-  public function testLinter() {
+  protected function getLinter() {
     $linter = new ArcanistClosureLinter();
     $linter->setFlags(array('--additional_extensions=lint-test'));
+    return $linter;
+  }
 
-    $this->executeTestsInDirectory(dirname(__FILE__).'/gjslint/', $linter);
+  public function testLinter() {
+    $this->executeTestsInDirectory(dirname(__FILE__).'/gjslint/');
   }
 
 }
diff --git a/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php b/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php
index c26356d0..960bb46c 100644
--- a/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php
+++ b/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php
@@ -28,7 +28,7 @@ final class XUnitTestResultParserTestCase extends PhutilTestCase {
       $parsed_results = id(new ArcanistXUnitTestResultParser())
         ->parseTestResults('');
 
-      $this->failTest(pht('Should throw on empty input'));
+      $this->assertFailure(pht('Should throw on empty input'));
     } catch (Exception $e) {
       // OK
     }
@@ -43,7 +43,7 @@ final class XUnitTestResultParserTestCase extends PhutilTestCase {
       $parsed_results = id(new ArcanistXUnitTestResultParser())
         ->parseTestResults($stubbed_results);
 
-      $this->failTest(pht('Should throw on non-xml input'));
+      $this->assertFailure(pht('Should throw on non-xml input'));
     } catch (Exception $e) {
       // OK
     }
diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php
index 884f3a54..8323ce24 100644
--- a/src/workflow/ArcanistLandWorkflow.php
+++ b/src/workflow/ArcanistLandWorkflow.php
@@ -514,9 +514,7 @@ EOTEXT
     $this->branch = head($branch);
     $this->keepBranch = $this->getArgument('keep-branch');
 
-    $update_strategy = $this->getConfigFromAnySource(
-      'arc.land.update.default',
-      'merge');
+    $update_strategy = $this->getConfigFromAnySource('arc.land.update.default');
     $this->shouldUpdateWithRebase = $update_strategy == 'rebase';
     if ($this->getArgument('update-with-rebase')) {
       $this->shouldUpdateWithRebase = true;
diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php
index bb53cde7..36126c5d 100644
--- a/src/workflow/ArcanistLintWorkflow.php
+++ b/src/workflow/ArcanistLintWorkflow.php
@@ -227,7 +227,7 @@ EOTEXT
     }
 
     if ($everything) {
-      $paths = iterator_to_array($this->getRepositoryApi()->getAllFiles());
+      $paths = iterator_to_array($this->getRepositoryAPI()->getAllFiles());
       $this->shouldLintAll = true;
     } else {
       $paths = $this->selectPathsForWorkflow($paths, $rev);
diff --git a/src/workflow/ArcanistStartWorkflow.php b/src/workflow/ArcanistStartWorkflow.php
index b940265e..b39cea5a 100644
--- a/src/workflow/ArcanistStartWorkflow.php
+++ b/src/workflow/ArcanistStartWorkflow.php
@@ -76,7 +76,7 @@ EOTEXT
       "%s:  %s\n\n",
       pht('Started'),
       implode(', ', ipull($phid_query, 'fullName')));
-    $this->printCurrentTracking(true);
+    $this->printCurrentTracking();
   }
 
 }
diff --git a/src/workflow/ArcanistStopWorkflow.php b/src/workflow/ArcanistStopWorkflow.php
index 1e9f2518..2af72edb 100644
--- a/src/workflow/ArcanistStopWorkflow.php
+++ b/src/workflow/ArcanistStopWorkflow.php
@@ -105,7 +105,7 @@ EOTEXT
       "%s  %s\n\n",
       pht('Stopped:'),
       implode(', ', ipull($phid_query, 'fullName')));
-    $this->printCurrentTracking(true);
+    $this->printCurrentTracking();
   }
 
 }
diff --git a/src/workflow/ArcanistTasksWorkflow.php b/src/workflow/ArcanistTasksWorkflow.php
index 95cc7de4..3b9a0789 100644
--- a/src/workflow/ArcanistTasksWorkflow.php
+++ b/src/workflow/ArcanistTasksWorkflow.php
@@ -81,7 +81,7 @@ EOTEXT
     $unassigned = $this->getArgument('unassigned');
 
     if ($owner) {
-      $owner_phid = $this->findOwnerPhid($owner);
+      $owner_phid = $this->findOwnerPHID($owner);
     } else if ($unassigned) {
       $owner_phid = null;
     } else {
diff --git a/src/workflow/ArcanistUnitWorkflow.php b/src/workflow/ArcanistUnitWorkflow.php
index a1aadd3b..00ef8a59 100644
--- a/src/workflow/ArcanistUnitWorkflow.php
+++ b/src/workflow/ArcanistUnitWorkflow.php
@@ -140,7 +140,7 @@ EOTEXT
     }
 
     if ($everything) {
-      $paths = iterator_to_array($this->getRepositoryApi()->getAllFiles());
+      $paths = iterator_to_array($this->getRepositoryAPI()->getAllFiles());
     } else {
       $paths = $this->selectPathsForWorkflow($paths, $rev);
     }
diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
index 2365083a..192cf243 100644
--- a/src/workflow/ArcanistWorkflow.php
+++ b/src/workflow/ArcanistWorkflow.php
@@ -1008,9 +1008,7 @@ abstract class ArcanistWorkflow extends Phobject {
         }
         $should_commit = true;
       } else {
-        $permit_autostash = $this->getConfigFromAnySource(
-          'arc.autostash',
-          false);
+        $permit_autostash = $this->getConfigFromAnySource('arc.autostash');
         if ($permit_autostash && $api->canStashChanges()) {
            echo pht(
             'Stashing uncommitted changes. (You can restore them with `%s`).',

From 3b6b523c2b236e3724a1e115f126cb6fd05fa128 Mon Sep 17 00:00:00 2001
From: Jakub Vrana <jakub@vrana.cz>
Date: Sat, 18 Feb 2017 09:24:19 +0000
Subject: [PATCH 09/34] Fix errors found by PHPStan

Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17376
---
 src/lint/ArcanistLintMessage.php                              | 3 ++-
 .../xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php | 2 +-
 src/repository/api/ArcanistGitAPI.php                         | 2 +-
 src/repository/api/ArcanistMercurialAPI.php                   | 2 +-
 src/repository/api/ArcanistRepositoryAPI.php                  | 2 +-
 src/unit/engine/XUnitTestEngine.php                           | 4 ++--
 src/workflow/ArcanistLandWorkflow.php                         | 2 +-
 src/workflow/ArcanistPatchWorkflow.php                        | 4 ++--
 src/workflow/ArcanistWorkflow.php                             | 2 +-
 9 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php
index 29f9ab92..dafc28ba 100644
--- a/src/lint/ArcanistLintMessage.php
+++ b/src/lint/ArcanistLintMessage.php
@@ -40,7 +40,8 @@ final class ArcanistLintMessage extends Phobject {
     $message->setGranularity(idx($dict, 'granularity'));
     $message->setOtherLocations(idx($dict, 'locations', array()));
     if (isset($dict['bypassChangedLineFiltering'])) {
-      $message->bypassChangedLineFiltering($dict['bypassChangedLineFiltering']);
+      $message->setBypassChangedLineFiltering(
+        $dict['bypassChangedLineFiltering']);
     }
     return $message;
   }
diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php
index c7790eb2..d73d1fae 100644
--- a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php
+++ b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php
@@ -369,7 +369,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule
       }
     }
 
-    $literals = $root->selectDescendantsOftype('n_ARRAY_LITERAL');
+    $literals = $root->selectDescendantsOfType('n_ARRAY_LITERAL');
     foreach ($literals as $literal) {
       $open_token = head($literal->getTokens())->getValue();
       if ($open_token == '[') {
diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index 8efe1bce..a316093e 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -1405,7 +1405,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
    * or cycle locally.
    *
    * @param string Ref to start from.
-   * @return list<wild> Path to an upstream.
+   * @return ArcanistGitUpstreamPath Path to an upstream.
    */
   public function getPathToUpstream($start) {
     $cursor = $start;
diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php
index 6fa6c48a..5fe14327 100644
--- a/src/repository/api/ArcanistMercurialAPI.php
+++ b/src/repository/api/ArcanistMercurialAPI.php
@@ -791,7 +791,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
         'commit --amend -l %s',
         $tmp_file);
     } catch (CommandException $ex) {
-      if (preg_match('/nothing changed/', $ex->getStdOut())) {
+      if (preg_match('/nothing changed/', $ex->getStdout())) {
         // NOTE: Mercurial considers it an error to make a no-op amend. Although
         // we generally defer to the underlying VCS to dictate behavior, this
         // one seems a little goofy, and we use amend as part of various
diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php
index a77a8c90..f95742c1 100644
--- a/src/repository/api/ArcanistRepositoryAPI.php
+++ b/src/repository/api/ArcanistRepositoryAPI.php
@@ -282,7 +282,7 @@ abstract class ArcanistRepositoryAPI extends Phobject {
    * Hook for implementations to dirty working copy caches after the working
    * copy has been updated.
    *
-   * @return this
+   * @return void
    * @task status
    */
   protected function didReloadWorkingCopy() {
diff --git a/src/unit/engine/XUnitTestEngine.php b/src/unit/engine/XUnitTestEngine.php
index 803880af..d6a20464 100644
--- a/src/unit/engine/XUnitTestEngine.php
+++ b/src/unit/engine/XUnitTestEngine.php
@@ -255,7 +255,7 @@ class XUnitTestEngine extends ArcanistUnitTestEngine {
         throw $exc;
       }
       $result->setResult(ArcanistUnitTestResult::RESULT_FAIL);
-      $result->setUserdata($exc->getStdout());
+      $result->setUserData($exc->getStdout());
     }
 
     $result->setDuration(microtime(true) - $regenerate_start);
@@ -301,7 +301,7 @@ class XUnitTestEngine extends ArcanistUnitTestEngine {
           throw $exc;
         }
         $result->setResult(ArcanistUnitTestResult::RESULT_FAIL);
-        $result->setUserdata($exc->getStdout());
+        $result->setUserData($exc->getStdout());
         $build_failed = true;
       }
 
diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php
index 8323ce24..ee59a8d2 100644
--- a/src/workflow/ArcanistLandWorkflow.php
+++ b/src/workflow/ArcanistLandWorkflow.php
@@ -886,7 +886,7 @@ EOTEXT
         }
       } catch (CommandException $ex) {
         $err = $ex->getError();
-        $stdout = $ex->getStdOut();
+        $stdout = $ex->getStdout();
 
         // Copied from: PhabricatorRepositoryPullLocalDaemon.php
         // NOTE: Between versions 2.1 and 2.1.1, Mercurial changed the
diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php
index e7c6760d..c1a8f878 100644
--- a/src/workflow/ArcanistPatchWorkflow.php
+++ b/src/workflow/ArcanistPatchWorkflow.php
@@ -710,7 +710,7 @@ EOTEXT
       }
 
       // in case there were any submodule changes involved
-      $repository_api->execpassthru('submodule update --init --recursive');
+      $repository_api->execPassthru('submodule update --init --recursive');
 
       if ($this->shouldCommit()) {
         if ($bundle->getFullAuthor()) {
@@ -763,7 +763,7 @@ EOTEXT
         echo phutil_console_format(
           "\n<bg:red>** %s **</bg>\n",
           pht('Patch Failed!'));
-        $stderr = $ex->getStdErr();
+        $stderr = $ex->getStderr();
         if (preg_match('/case-folding collision/', $stderr)) {
           echo phutil_console_wrap(
             phutil_console_format(
diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
index 192cf243..ad78b243 100644
--- a/src/workflow/ArcanistWorkflow.php
+++ b/src/workflow/ArcanistWorkflow.php
@@ -336,7 +336,7 @@ abstract class ArcanistWorkflow extends Phobject {
 
           $this->conduitAuthenticated = true;
 
-          return;
+          return $this;
         } catch (Exception $ex) {
           $conduit->setConduitToken(null);
           throw $ex;

From d1db9a72b552151613a918e3d49fa72433387a68 Mon Sep 17 00:00:00 2001
From: Jon Parise <jon@pinterest.com>
Date: Fri, 24 Mar 2017 08:26:03 -0700
Subject: [PATCH 10/34] Improve the flake8 line-matching regex

Summary:
Because the character offset group is optional, the logic for dealing
with the previous index-based match object was more complex than it
needs to be. Switching to named groups makes this clearer.

As an example of how this was causing a problem, when the character
group *was* present (at index 3), it was being appending to the linter's
name in the call to ->setName(), resulting in confusing linter output.
We may have also been setting the character offset to the error code's
string, too, which is further nonsense.

Because we reliably capture the flake8 error code, I don't think there's
a need to append it to the linter's name at all now, so I removed that
part (so it's always just `flake8`), but it's not a problem to restore.

Lastly, I hoisted `$regex` out of the loop because it's a constant and
de-indenting it gave me enough room to continuing writing it all on one
line.

Test Plan: Tested primarily with flake8 3.3.0

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17552
---
 src/lint/linter/ArcanistFlake8Linter.php | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php
index 19d57432..37fb45e1 100644
--- a/src/lint/linter/ArcanistFlake8Linter.php
+++ b/src/lint/linter/ArcanistFlake8Linter.php
@@ -51,12 +51,14 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
   protected function parseLinterOutput($path, $err, $stdout, $stderr) {
     $lines = phutil_split_lines($stdout, false);
 
+    // stdin:2: W802 undefined name 'foo'  # pyflakes
+    // stdin:3:1: E302 expected 2 blank lines, found 1  # pep8
+    $regexp =
+      '/^(?:.*?):(?P<line>\d+):(?:(?P<char>\d+):)? (?P<code>\S+) (?P<msg>.*)$/';
+
     $messages = array();
     foreach ($lines as $line) {
       $matches = null;
-      // stdin:2: W802 undefined name 'foo'  # pyflakes
-      // stdin:3:1: E302 expected 2 blank lines, found 1  # pep8
-      $regexp = '/^(.*?):(\d+):(?:(\d+):)? (\S+) (.*)$/';
       if (!preg_match($regexp, $line, $matches)) {
         continue;
       }
@@ -66,14 +68,14 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
 
       $message = new ArcanistLintMessage();
       $message->setPath($path);
-      $message->setLine($matches[2]);
-      if (!empty($matches[3])) {
-        $message->setChar($matches[3]);
+      $message->setLine($matches['line']);
+      if (!empty($matches['char'])) {
+        $message->setChar($matches['char']);
       }
-      $message->setCode($matches[4]);
-      $message->setName($this->getLinterName().' '.$matches[3]);
-      $message->setDescription($matches[5]);
-      $message->setSeverity($this->getLintMessageSeverity($matches[4]));
+      $message->setCode($matches['code']);
+      $message->setName($this->getLinterName().' '.$matches['code']);
+      $message->setDescription($matches['msg']);
+      $message->setSeverity($this->getLintMessageSeverity($matches['code']));
 
       $messages[] = $message;
     }

From 82b7cd778a288dfb2b9035b8395d97c8f6185e2b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 09:50:00 -0700
Subject: [PATCH 11/34] Make "arc download" use "file.search" if available

Summary: Fixes T8348. Just use normal HTTP GET to download files if the server is sufficiently modern.

Test Plan: Downloaded various files with `--as`, `--show`, large files, small files, old server, new server.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8348

Differential Revision: https://secure.phabricator.com/D17614
---
 src/workflow/ArcanistDownloadWorkflow.php | 157 ++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/src/workflow/ArcanistDownloadWorkflow.php b/src/workflow/ArcanistDownloadWorkflow.php
index f3eb466b..66fa7331 100644
--- a/src/workflow/ArcanistDownloadWorkflow.php
+++ b/src/workflow/ArcanistDownloadWorkflow.php
@@ -79,6 +79,163 @@ EOTEXT
   public function run() {
     $conduit = $this->getConduit();
 
+    $id = $this->id;
+    $display_name = 'F'.$id;
+    $is_show = $this->show;
+    $save_as = $this->saveAs;
+
+    try {
+      $file = $conduit->callMethodSynchronous(
+        'file.search',
+        array(
+          'constraints' => array(
+            'ids' => array($id),
+          ),
+        ));
+
+      $data = $file['data'];
+      if (!$data) {
+        throw new ArcanistUsageException(
+          pht(
+            'File "%s" is not a valid file, or not visible.',
+            $display_name));
+      }
+
+      $file = head($data);
+      $data_uri = idxv($file, array('fields', 'dataURI'));
+
+      if ($data_uri === null) {
+        throw new ArcanistUsageException(
+          pht(
+            'File "%s" can not be downloaded.',
+            $display_name));
+      }
+
+      if ($is_show) {
+        // Skip all the file path stuff if we're just going to echo the
+        // file contents.
+      } else {
+        if ($save_as !== null) {
+          $path = Filesystem::resolvePath($save_as);
+
+          $try_unique = false;
+        } else {
+          $path = idxv($file, array('fields', 'name'), $display_name);
+          $path = basename($path);
+          $path = Filesystem::resolvePath($path);
+
+          $try_unique = true;
+        }
+
+        if ($try_unique) {
+          $path = Filesystem::writeUniqueFile($path, '');
+        } else {
+          if (Filesystem::pathExists($path)) {
+            throw new ArcanistUsageException(
+              pht(
+                'File "%s" already exists.',
+                $save_as));
+          }
+
+          Filesystem::writeFile($path, '');
+        }
+
+        $display_path = Filesystem::readablePath($path);
+      }
+
+      $size = idxv($file, array('fields', 'size'), 0);
+
+      if ($is_show) {
+        $file_handle = null;
+      } else {
+        $file_handle = fopen($path, 'ab+');
+        if ($file_handle === false) {
+          throw new Exception(
+            pht(
+              'Failed to open file "%s" for writing.',
+              $path));
+        }
+
+        $this->writeInfo(
+          pht('DATA'),
+          pht(
+            'Downloading "%s" (%s byte(s))...',
+            $display_name,
+            new PhutilNumber($size)));
+      }
+
+      $future = new HTTPSFuture($data_uri);
+
+      // For small files, don't bother drawing a progress bar.
+      $minimum_bar_bytes = (1024 * 1024 * 4);
+
+      if ($is_show || ($size < $minimum_bar_bytes)) {
+        $bar = null;
+      } else {
+        $bar = id(new PhutilConsoleProgressBar())
+          ->setTotal($size);
+      }
+
+      // TODO: We should stream responses to disk, but cURL gives us the raw
+      // HTTP response data and BaseHTTPFuture can not currently parse it in
+      // a stream-oriented way. Until this is resolved, buffer the file data
+      // in memory and write it to disk in one shot.
+
+      list($status, $data) = $future->resolve();
+      if ($status->getStatusCode() !== 200) {
+        throw new Exception(
+          pht(
+            'Got HTTP %d status response, expected HTTP 200.',
+            $status));
+      }
+
+      if (strlen($data)) {
+        if ($is_show) {
+          echo $data;
+        } else {
+          $ok = fwrite($file_handle, $data);
+          if ($ok === false) {
+            throw new Exception(
+              pht(
+                'Failed to write file data to "%s".',
+                $path));
+          }
+        }
+      }
+
+      if ($bar) {
+        $bar->update(strlen($data));
+      }
+
+      if ($bar) {
+        $bar->done();
+      }
+
+      if ($file_handle) {
+        $ok = fclose($file_handle);
+        if ($ok === false) {
+          throw new Exception(
+            pht(
+              'Failed to close file handle for "%s".',
+              $path));
+        }
+      }
+
+      if (!$is_show) {
+        $this->writeOkay(
+          pht('DONE'),
+          pht(
+            'Saved "%s" as "%s".',
+            $display_name,
+            $display_path));
+      }
+
+      return 0;
+    } catch (Exception $ex) {
+      // If we fail for any reason, fall back to the older mechanism using
+      // "file.info" and "file.download".
+    }
+
     $this->writeStatusMessage(pht('Getting file information...')."\n");
     $info = $conduit->callMethodSynchronous(
       'file.info',

From c4e84550fcd33f0e0ea0a245044352dac52accfe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 15:46:31 -0700
Subject: [PATCH 12/34] Allow "arc upload" to work correctly if it can not hash
 content

Summary:
Ref T12464. This is similar to D17619 and prepares us to move to SHA256 in the client.

Note that it's fine if `arc` and Phabricator disagree about hashing algorithms. We don't really trust the client anyway, so if things are mismatched clients will just end up transferring a bit more data instead of getting to cheat when Phabricator already has copies of data.

Test Plan: Ran `arc upload`, got a clean upload.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17621
---
 src/upload/ArcanistFileDataRef.php | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/upload/ArcanistFileDataRef.php b/src/upload/ArcanistFileDataRef.php
index 318418ec..1a79e143 100644
--- a/src/upload/ArcanistFileDataRef.php
+++ b/src/upload/ArcanistFileDataRef.php
@@ -222,15 +222,6 @@ final class ArcanistFileDataRef extends Phobject {
             $path));
       }
 
-      $hash = @sha1_file($path);
-      if ($hash === false) {
-        throw new Exception(
-          pht(
-            'Unable to upload file: failed to calculate file data hash for '.
-            'path "%s".',
-            $path));
-      }
-
       $size = @filesize($path);
       if ($size === false) {
         throw new Exception(
@@ -240,11 +231,11 @@ final class ArcanistFileDataRef extends Phobject {
             $path));
       }
 
-      $this->hash = $hash;
+      $this->hash = $this->newFileHash($path);
       $this->size = $size;
     } else {
       $data = $this->data;
-      $this->hash = sha1($data);
+      $this->hash = $this->newDataHash($data);
       $this->size = strlen($data);
     }
   }
@@ -354,4 +345,12 @@ final class ArcanistFileDataRef extends Phobject {
     return $data;
   }
 
+  private function newFileHash($path) {
+    return null;
+  }
+
+  private function newDataHash($data) {
+    return null;
+  }
+
 }

From a59cfca5f190c44403dfc7449c678a2aa1626bb4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 15:52:46 -0700
Subject: [PATCH 13/34] Upgrade "arc upload" to use SHA256

Summary: Fixes T12464. Moves "arc upload" to SHA256 where applicable.

Test Plan: Ran `arc upload` against a server with D17620 twice, saw it skip the actual upload the second time.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17622
---
 src/upload/ArcanistFileDataRef.php | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/upload/ArcanistFileDataRef.php b/src/upload/ArcanistFileDataRef.php
index 1a79e143..99ed03d9 100644
--- a/src/upload/ArcanistFileDataRef.php
+++ b/src/upload/ArcanistFileDataRef.php
@@ -346,11 +346,23 @@ final class ArcanistFileDataRef extends Phobject {
   }
 
   private function newFileHash($path) {
-    return null;
+    $hash = hash_file('sha256', $path, $raw_output = false);
+
+    if ($hash === false) {
+      return null;
+    }
+
+    return $hash;
   }
 
   private function newDataHash($data) {
-    return null;
+    $hash = hash('sha256', $data, $raw_output = false);
+
+    if ($hash === false) {
+      return null;
+    }
+
+    return $hash;
   }
 
 }

From 146693307f607ffa93dfb99599f1989d5b27e03d Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@dropbox.com>
Date: Thu, 20 Apr 2017 14:50:03 -0700
Subject: [PATCH 14/34] Make exception reporting from `arc` be in red

Summary:
Many other status updates (such as "Builds passed!") show up
in bright background colors, making them more salient than a final
fatal "Exception".

Make exception reporting be just as colorful, so it stands out.

Test Plan:
Added an explicit `throw new Exception("!!!")` and saw it
in red.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17748
---
 scripts/arcanist.php | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/arcanist.php b/scripts/arcanist.php
index dc2f07b7..c0ae4d8c 100755
--- a/scripts/arcanist.php
+++ b/scripts/arcanist.php
@@ -422,7 +422,8 @@ try {
   }
 
   if (!$is_usage) {
-    fwrite(STDERR, phutil_console_format("**%s**\n", pht('Exception')));
+    fwrite(STDERR, phutil_console_format(
+      "<bg:red>** %s **</bg>\n", pht('Exception')));
 
     while ($ex) {
       fwrite(STDERR, $ex->getMessage()."\n");

From 5d0f5afca8cdba3effa822b1d0f0ed3d90fec8b6 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Mon, 24 Apr 2017 16:12:03 -0700
Subject: [PATCH 15/34] Add ArcanistRaggedClassTreeEdgeXHPASTLinterRule to
 Phutil linter map

Summary: Fixes T12555.

Test Plan:
Added this class to the codebase and ran `arc liberate`:
```
<?php

class FooBar {
    public static function doTheFoo() {
        return 'foobar';
    }
}
```

Ran `arc lint` and observed this warning:
```
   Warning  (XHP87) Class Not `abstract` Or `final`
    This class is neither `final` nor `abstract`, and does not have a
    docblock marking it `@concrete-extensible`.

               1 <?php
               2
    >>>        3 class FooBar {
               4     public static function doTheFoo() {
               5         return "foobar";
               6     }
```

Added a `final` modifier to `FooBar`'s declaration and observed the warning went away.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12555

Differential Revision: https://secure.phabricator.com/D17787
---
 .../phutil/ArcanistPhutilXHPASTLinterStandard.php      | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php b/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php
index 17547476..77560d11 100644
--- a/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php
+++ b/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php
@@ -56,12 +56,14 @@ final class ArcanistPhutilXHPASTLinterStandard
   }
 
   public function getLinterSeverityMap() {
-    $advice = ArcanistLintSeverity::SEVERITY_ADVICE;
-    $error  = ArcanistLintSeverity::SEVERITY_ERROR;
+    $advice  = ArcanistLintSeverity::SEVERITY_ADVICE;
+    $error   = ArcanistLintSeverity::SEVERITY_ERROR;
+    $warning = ArcanistLintSeverity::SEVERITY_WARNING;
 
     return array(
-      ArcanistTodoCommentXHPASTLinterRule::ID    => $advice,
-      ArcanistCommentSpacingXHPASTLinterRule::ID => $error,
+      ArcanistTodoCommentXHPASTLinterRule::ID         => $advice,
+      ArcanistCommentSpacingXHPASTLinterRule::ID      => $error,
+      ArcanistRaggedClassTreeEdgeXHPASTLinterRule::ID => $warning,
     );
   }
 

From 27b51e619237116d99e03fc1b3e8cd6399087214 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 27 Apr 2017 16:17:04 -0700
Subject: [PATCH 16/34] Fix two minor issues with "arc download"

Summary:
Ref T12651. Ran into these during D17799:

  - Use `getStatusCode()` to put the actual status code into the message.
  - If we fail but wrote an empty file to reserve the filename, clean it up.

Test Plan:
  - Faked the error, `phlog()`'d the exception.
  - Saw sensible exception message.
  - Saw empty file get cleaned up.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12651

Differential Revision: https://secure.phabricator.com/D17800
---
 src/workflow/ArcanistDownloadWorkflow.php | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/workflow/ArcanistDownloadWorkflow.php b/src/workflow/ArcanistDownloadWorkflow.php
index 66fa7331..543b715e 100644
--- a/src/workflow/ArcanistDownloadWorkflow.php
+++ b/src/workflow/ArcanistDownloadWorkflow.php
@@ -83,6 +83,7 @@ EOTEXT
     $display_name = 'F'.$id;
     $is_show = $this->show;
     $save_as = $this->saveAs;
+    $path = null;
 
     try {
       $file = $conduit->callMethodSynchronous(
@@ -186,7 +187,7 @@ EOTEXT
         throw new Exception(
           pht(
             'Got HTTP %d status response, expected HTTP 200.',
-            $status));
+            $status->getStatusCode()));
       }
 
       if (strlen($data)) {
@@ -232,6 +233,14 @@ EOTEXT
 
       return 0;
     } catch (Exception $ex) {
+
+      // If we created an empty file, clean it up.
+      if (!$is_show) {
+        if ($path !== null) {
+          Filesystem::remove($path);
+        }
+      }
+
       // If we fail for any reason, fall back to the older mechanism using
       // "file.info" and "file.download".
     }

From 3c4735795a2963c5ddff6dceaf60122d01ca3dc0 Mon Sep 17 00:00:00 2001
From: Hubert Kowalski <h.kowalski@hakger.pl>
Date: Wed, 3 May 2017 03:29:14 -0700
Subject: [PATCH 17/34] Detect trailing spaces and tabs

Summary: Ref T12655 - this change properly detects trailing spaces or tabs (or combinations of thereof) on end of lines.

Test Plan: Use Text lint with trailing whitespace rule on files with spaces, tabs or combinations of thereof. Should properly detect and fix all those.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, Itms

Maniphest Tasks: T12655

Differential Revision: https://secure.phabricator.com/D17814
---
 src/lint/linter/ArcanistTextLinter.php            |  2 +-
 ....lint-test => trailing-whitespace-1.lint-test} |  0
 .../text/trailing-whitespace-2.lint-test          | 15 +++++++++++++++
 .../text/trailing-whitespace-3.lint-test          | 15 +++++++++++++++
 4 files changed, 31 insertions(+), 1 deletion(-)
 rename src/lint/linter/__tests__/text/{trailing-whitespace.lint-test => trailing-whitespace-1.lint-test} (100%)
 create mode 100644 src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test
 create mode 100644 src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test

diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php
index 2f81b039..87a2d8a7 100644
--- a/src/lint/linter/ArcanistTextLinter.php
+++ b/src/lint/linter/ArcanistTextLinter.php
@@ -245,7 +245,7 @@ final class ArcanistTextLinter extends ArcanistLinter {
 
     $matches = null;
     $preg = preg_match_all(
-      '/ +$/m',
+      '/[[:blank:]]+$/m',
       $data,
       $matches,
       PREG_OFFSET_CAPTURE);
diff --git a/src/lint/linter/__tests__/text/trailing-whitespace.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-1.lint-test
similarity index 100%
rename from src/lint/linter/__tests__/text/trailing-whitespace.lint-test
rename to src/lint/linter/__tests__/text/trailing-whitespace-1.lint-test
diff --git a/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test
new file mode 100644
index 00000000..0863c187
--- /dev/null
+++ b/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test
@@ -0,0 +1,15 @@
+Lorem ipsum dolor sit amet,	
+consectetur adipiscing elit.	
+Phasellus sodales nibh erat,	
+in hendrerit nulla dictum interdum.	
+~~~~~~~~~~
+error:1:28
+autofix:1:28
+autofix:2:29
+autofix:3:29
+autofix:4:36
+~~~~~~~~~~
+Lorem ipsum dolor sit amet,
+consectetur adipiscing elit.
+Phasellus sodales nibh erat,
+in hendrerit nulla dictum interdum.
diff --git a/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test
new file mode 100644
index 00000000..e92f7bbd
--- /dev/null
+++ b/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test
@@ -0,0 +1,15 @@
+Lorem ipsum dolor sit amet,	 	 	 	 	 	
+consectetur adipiscing elit.	      			
+Phasellus sodales nibh erat,	  	
+in hendrerit nulla dictum interdum.    		
+~~~~~~~~~~
+error:1:28
+autofix:1:28
+autofix:2:29
+autofix:3:29
+autofix:4:36
+~~~~~~~~~~
+Lorem ipsum dolor sit amet,
+consectetur adipiscing elit.
+Phasellus sodales nibh erat,
+in hendrerit nulla dictum interdum.

From 129d51fa0936c9bae48fadf3a3f39e26d69d24da Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@dropbox.com>
Date: Thu, 18 May 2017 12:09:44 -0400
Subject: [PATCH 18/34] If the base commit for `arc patch` does not exist
 locally, try to fetch it

Summary:
If the commit does not exist locally, aborting still leaves
the user checked out on the branch.  In nearly all cases, all that is
necessary is a fetch -- but the branch must also be cleaned up.  This
leads to the pattern of:
```
arc patch D12345
[...base commit does not exist...]
^C
git checkout master
git branch -D arcpatch-D12345
git fetch
arc patch D12345
```

Solve this common problem by simply trying to fetch once if the commit does not
exist locally.

Test Plan: Ran `arc patch` on a recent diff.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17949
---
 src/workflow/ArcanistPatchWorkflow.php | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php
index c1a8f878..227b020f 100644
--- a/src/workflow/ArcanistPatchWorkflow.php
+++ b/src/workflow/ArcanistPatchWorkflow.php
@@ -430,6 +430,18 @@ EOTEXT
     $repository_api = $this->getRepositoryAPI();
     $has_base_revision = $repository_api->hasLocalCommit(
       $bundle->getBaseRevision());
+    if (!$has_base_revision) {
+      if ($repository_api instanceof ArcanistGitAPI) {
+        echo phutil_console_format(
+          "<bg:blue>** %s **</bg> %s\n",
+          pht('INFO'),
+          pht('Base commit is not in local repository; trying to fetch.'));
+        $repository_api->execManualLocal('fetch --quiet --all');
+        $has_base_revision = $repository_api->hasLocalCommit(
+          $bundle->getBaseRevision());
+      }
+    }
+
     if ($this->canBranch() &&
          ($this->shouldBranch() ||
          ($this->shouldCommit() && $has_base_revision))) {

From c04f141ab0231e593a513356b3832a30f9404627 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 9 Jun 2017 06:52:18 -0700
Subject: [PATCH 19/34] Remove obsolete "arc land" flags:
 --update-with-merge/rebase, --delete-remote

Summary:
Fixes T12815. During the last update to "arc land", some flags were disabled but remained in place in case we needed to retain them.

It now seems reasonably clear that we do not. The "rebase" and "merge" strategies for landing were replaced by a better "headless" strategy which seems to avoid the original issues, so these flags no longer do anything or reasonably could do anything.

`--delete-remote` is slightly more ambiguous (e.g., see T12650 and maybe others) but the only real use case is "git push = save changes".

Test Plan:
Ran `arc land --update-with-rebase`, was told the flag does not exist.
Grepped for affected flags/symbols.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12815

Differential Revision: https://secure.phabricator.com/D18108
---
 src/configuration/ArcanistSettings.php |   7 --
 src/workflow/ArcanistLandWorkflow.php  | 125 +------------------------
 2 files changed, 3 insertions(+), 129 deletions(-)

diff --git a/src/configuration/ArcanistSettings.php b/src/configuration/ArcanistSettings.php
index f87fb356..fdf68259 100644
--- a/src/configuration/ArcanistSettings.php
+++ b/src/configuration/ArcanistSettings.php
@@ -80,13 +80,6 @@ final class ArcanistSettings extends Phobject {
           'arc land'),
         'example' => '"develop"',
       ),
-      'arc.land.update.default' => array(
-        'type' => 'string',
-        'help' => pht(
-          'The default strategy to use when arc land updates the feature '.
-          'branch. Supports "rebase" and "merge" strategies.'),
-        'example' => '"rebase"',
-      ),
       'arc.lint.cache' => array(
         'type' => 'bool',
         'help' => pht(
diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php
index ee59a8d2..66ce42d5 100644
--- a/src/workflow/ArcanistLandWorkflow.php
+++ b/src/workflow/ArcanistLandWorkflow.php
@@ -17,7 +17,6 @@ final class ArcanistLandWorkflow extends ArcanistWorkflow {
   private $remote;
   private $useSquash;
   private $keepBranch;
-  private $shouldUpdateWithRebase;
   private $branchType;
   private $ontoType;
   private $preview;
@@ -197,43 +196,8 @@ EOTEXT
         'conflicts' => array(
           'keep-branch' => true,
         ),
-      ),
-      'update-with-rebase' => array(
-        'help' => pht(
-          "When updating the feature branch, use rebase instead of merge. ".
-          "This might make things work better in some cases. Set ".
-          "%s to '%s' to make this the default.",
-          'arc.land.update.default',
-          'rebase'),
-        'conflicts' => array(
-          'merge' => pht(
-            'The %s strategy does not update the feature branch.',
-            '--merge'),
-          'update-with-merge' => pht(
-            'Cannot be used with %s.',
-            '--update-with-merge'),
-        ),
         'supports' => array(
-          'git',
-        ),
-      ),
-      'update-with-merge' => array(
-        'help' => pht(
-          "When updating the feature branch, use merge instead of rebase. ".
-          "This is the default behavior. Setting %s to '%s' can also be ".
-          "used to make this the default.",
-          'arc.land.update.default',
-          'merge'),
-        'conflicts' => array(
-          'merge' => pht(
-            'The %s strategy does not update the feature branch.',
-            '--merge'),
-          'update-with-rebase' => pht(
-            'Cannot be used with %s.',
-            '--update-with-rebase'),
-        ),
-        'supports' => array(
-          'git',
+          'hg',
         ),
       ),
       'revision' => array(
@@ -261,22 +225,6 @@ EOTEXT
 
     if ($engine) {
       $this->readEngineArguments();
-
-      $obsolete = array(
-        'delete-remote',
-        'update-with-merge',
-        'update-with-rebase',
-      );
-
-      foreach ($obsolete as $flag) {
-        if ($this->getArgument($flag)) {
-          throw new ArcanistUsageException(
-            pht(
-              'Flag "%s" is no longer supported under Git.',
-              '--'.$flag));
-        }
-      }
-
       $this->requireCleanWorkingCopy();
 
       $should_hold = $this->getArgument('hold');
@@ -514,14 +462,6 @@ EOTEXT
     $this->branch = head($branch);
     $this->keepBranch = $this->getArgument('keep-branch');
 
-    $update_strategy = $this->getConfigFromAnySource('arc.land.update.default');
-    $this->shouldUpdateWithRebase = $update_strategy == 'rebase';
-    if ($this->getArgument('update-with-rebase')) {
-      $this->shouldUpdateWithRebase = true;
-    } else if ($this->getArgument('update-with-merge')) {
-      $this->shouldUpdateWithRebase = false;
-    }
-
     $this->preview = $this->getArgument('preview');
 
     if (!$this->branchType) {
@@ -951,42 +891,7 @@ EOTEXT
     $repository_api = $this->getRepositoryAPI();
 
     chdir($repository_api->getPath());
-    if ($this->isGit) {
-      if ($this->shouldUpdateWithRebase) {
-        echo phutil_console_format(pht(
-          'Rebasing **%s** onto **%s**',
-          $this->branch,
-          $this->onto)."\n");
-        $err = phutil_passthru('git rebase %s', $this->onto);
-        if ($err) {
-          throw new ArcanistUsageException(pht(
-            "'%s' failed. You can abort with '%s', or resolve conflicts ".
-            "and use '%s' to continue forward. After resolving the rebase, ".
-            "run '%s' again.",
-            sprintf('git rebase %s', $this->onto),
-            'git rebase --abort',
-            'git rebase --continue',
-            'arc land'));
-        }
-      } else {
-        echo phutil_console_format(pht(
-          'Merging **%s** into **%s**',
-          $this->branch,
-          $this->onto)."\n");
-        $err = phutil_passthru(
-          'git merge --no-stat %s -m %s',
-          $this->onto,
-          pht("Automatic merge by '%s'", 'arc land'));
-        if ($err) {
-          throw new ArcanistUsageException(pht(
-            "'%s' failed. To continue: resolve the conflicts, commit ".
-            "the changes, then run '%s' again. To abort: run '%s'.",
-            sprintf('git merge %s', $this->onto),
-            'arc land',
-            'git merge --abort'));
-        }
-      }
-    } else if ($this->isHg) {
+    if ($this->isHg) {
       $onto_tip = $repository_api->getCanonicalRevisionName($this->onto);
       $common_ancestor = $repository_api->getCanonicalRevisionName(
         hgsprintf('ancestor(%s, %s)', $this->onto, $this->branch));
@@ -1367,31 +1272,7 @@ EOTEXT
     }
 
     if ($this->getArgument('delete-remote')) {
-      if ($this->isGit) {
-        list($err, $ref) = $repository_api->execManualLocal(
-          'rev-parse --verify %s/%s',
-          $this->remote,
-          $this->branch);
-
-        if ($err) {
-          echo pht(
-            'No remote feature %s to clean up.',
-            $this->branchType);
-          echo "\n";
-        } else {
-
-          // NOTE: In Git, you delete a remote branch by pushing it with a
-          // colon in front of its name:
-          //
-          //   git push <remote> :<branch>
-
-          echo pht('Cleaning up remote feature %s...', $this->branchType), "\n";
-          $repository_api->execxLocal(
-            'push %s :%s',
-            $this->remote,
-            $this->branch);
-        }
-      } else if ($this->isHg) {
+      if ($this->isHg) {
         // named branches were closed as part of the earlier commit
         // so only worry about bookmarks
         if ($repository_api->isBookmark($this->branch)) {

From 165df12046e58390f0d00f4cf3e1aa1ab0eb1365 Mon Sep 17 00:00:00 2001
From: Stephanie Ren <stef@dropbox.com>
Date: Wed, 5 Jul 2017 19:09:31 -0700
Subject: [PATCH 20/34] Consistently use `phutil_console_confirm()`.

Summary:
`PhutilConsole->confirm()` is vestigial as noted in efcd70c, as
`PhutilConsole` may be deprecated sometime in the future. `PhutilConsole`
was developed as a tool for T4281 but the feature has been removed
since.

Replace existing occurences of the `PhutilConsole->confirm()` pattern with
`phutil_console_confirm()`. There should be no change in functionality
since the two functions are interchangeable.

Test Plan: Manually tested by running `bin/arc lint`, `bin/arc diff --preview`, `bin/arc land --preview`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, alexmv

Differential Revision: https://secure.phabricator.com/D18185
---
 src/workflow/ArcanistDiffWorkflow.php | 2 +-
 src/workflow/ArcanistLandWorkflow.php | 2 +-
 src/workflow/ArcanistLintWorkflow.php | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php
index c7f2ff0a..14a7ef8f 100644
--- a/src/workflow/ArcanistDiffWorkflow.php
+++ b/src/workflow/ArcanistDiffWorkflow.php
@@ -1355,7 +1355,7 @@ EOTEXT
                 'Unit testing raised errors, but all '.
                 'failing tests are unsound.'));
           } else {
-            $continue = $this->console->confirm(
+            $continue = phutil_console_confirm(
               pht(
                 'Unit test results included failures, but all failing tests '.
                 'are known to be unsound. Ignore unsound test failures?'));
diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php
index 66ce42d5..5f13683a 100644
--- a/src/workflow/ArcanistLandWorkflow.php
+++ b/src/workflow/ArcanistLandWorkflow.php
@@ -1417,7 +1417,7 @@ EOTEXT
       pht('Harbormaster URI'),
       $buildable['uri']);
 
-    if (!$console->confirm($prompt)) {
+    if (!phutil_console_confirm($prompt)) {
       throw new ArcanistUserAbortException();
     }
   }
diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php
index 36126c5d..de215aad 100644
--- a/src/workflow/ArcanistLintWorkflow.php
+++ b/src/workflow/ArcanistLintWorkflow.php
@@ -518,7 +518,7 @@ EOTEXT
           $prompt = pht(
             'Apply this patch to %s?',
             phutil_console_format('__%s__', $result->getPath()));
-          if (!$console->confirm($prompt, $default = true)) {
+          if (!phutil_console_confirm($prompt, $default_no = false)) {
             continue;
           }
         }
@@ -546,7 +546,7 @@ EOTEXT
           pht('Automatically amending HEAD with lint patches.'));
         $amend = true;
       } else {
-        $amend = $console->confirm(pht('Amend HEAD with lint patches?'));
+        $amend = phutil_console_confirm(pht('Amend HEAD with lint patches?'));
       }
 
       if ($amend) {

From 7bb8dbabce83a3cd3c26fc5413a9a3429b97863b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 21 Jul 2017 11:38:06 -0700
Subject: [PATCH 21/34] Reduce the strength of "arc executing on arc" from an
 error to a warning

Summary:
See PHI13. This was introduced a very, very long time ago in D311 and D312, and I think T168 was the original report.

It prevents `arc` from being used in some semi-reasonable (maybe?) automation workflows where you're hooking some version of "Land Revision" up to `arc land`. This isn't necessarily the right approach, but I think the concession here to make this work is small.

Running `arc` against another copy of `arc` makes `arc unit` not work, but we provide a good error message. Most other `arc` operations still work correctly.

All of these situations are bizarre edge cases but I think we can safely warn and continue here. Even if we revert this behavior later, almost no one should be affected, since this essentially only impacts users developing `arc` itself.

Test Plan: Ran one copy of `arc` against another, saw a warning instead of an error. `arc unit` failed, but with a good error.

Reviewers: chad, jmeador

Reviewed By: jmeador

Differential Revision: https://secure.phabricator.com/D18264
---
 scripts/arcanist.php | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/scripts/arcanist.php b/scripts/arcanist.php
index c0ae4d8c..b950dbbf 100755
--- a/scripts/arcanist.php
+++ b/scripts/arcanist.php
@@ -641,16 +641,33 @@ function arcanist_load_libraries(
       if ($ex->getLibrary() != 'arcanist') {
         throw $ex;
       }
-      $arc_dir = dirname(dirname(__FILE__));
-      $error = pht(
-        "You are trying to run one copy of Arcanist on another copy of ".
-        "Arcanist. This operation is not supported. To execute Arcanist ".
-        "operations against this working copy, run `%s` (from the current ".
-        "working copy) not some other copy of '%s' (you ran one from '%s').",
-        './bin/arc',
-        'arc',
-        $arc_dir);
-      throw new ArcanistUsageException($error);
+
+      // NOTE: If you are running `arc` against itself, we ignore the library
+      // conflict created by loading the local `arc` library (in the current
+      // working directory) and continue without loading it.
+
+      // This means we only execute code in the `arcanist/` directory which is
+      // associated with the binary you are running, whereas we would normally
+      // execute local code.
+
+      // This can make `arc` development slightly confusing if your setup is
+      // especially bizarre, but it allows `arc` to be used in automation
+      // workflows more easily. For some context, see PHI13.
+
+      $executing_directory = dirname(dirname(__FILE__));
+      $working_directory = dirname($location);
+
+      fwrite(
+        STDERR,
+        tsprintf(
+          "**<bg:yellow> %s </bg>** %s\n",
+          pht('VERY META'),
+          pht(
+            'You are running one copy of Arcanist (at path "%s") against '.
+            'another copy of Arcanist (at path "%s"). Code in the current '.
+            'working directory will not be loaded or executed.',
+            $executing_directory,
+            $working_directory)));
     }
   }
 }

From 836768bdccd42ae6a0a95dca7760e25992067ffc Mon Sep 17 00:00:00 2001
From: Asher Baker <asherkin@limetech.io>
Date: Sat, 22 Jul 2017 03:02:01 +0100
Subject: [PATCH 22/34] Fix ArcanistPHPCloseTagXHPASTLinterRule always bailing
 out

Summary:
D13794 changed ArcanistPHPCloseTagXHPASTLinterRule to ignore inline HTML blocks, but selectDescendantsOfType returns an AASTNodeList (which always exists).

Instead, check that the count() of the node list is > 0.

empty.lint-test had to be changed, it wouldn't have been accepted had this rule not been broken before it was commited.

Added tests to cover ArcanistPHPCloseTagXHPASTLinterRule in the future.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18271
---
 src/__phutil_library_map__.php                         |  2 ++
 src/lint/linter/__tests__/xhpast/empty.lint-test       |  3 ++-
 .../rules/ArcanistPHPCloseTagXHPASTLinterRule.php      |  2 +-
 .../ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php    | 10 ++++++++++
 .../php-close-tag-inline-html-good.lint-test           |  6 ++++++
 .../__tests__/php-close-tag/php-close-tag.lint-test    |  5 +++++
 6 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index d8f10dd4..2a3cfcf8 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -266,6 +266,7 @@ phutil_register_library_map(array(
     'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php',
     'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php',
     'ArcanistPHPCloseTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php',
+    'ArcanistPHPCloseTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php',
     'ArcanistPHPCompatibilityXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php',
     'ArcanistPHPCompatibilityXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPCompatibilityXHPASTLinterRuleTestCase.php',
     'ArcanistPHPEchoTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPEchoTagXHPASTLinterRule.php',
@@ -680,6 +681,7 @@ phutil_register_library_map(array(
     'ArcanistPEP8Linter' => 'ArcanistExternalLinter',
     'ArcanistPEP8LinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistPHPCloseTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistPHPCloseTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPHPCompatibilityXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistPHPCompatibilityXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPHPEchoTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
diff --git a/src/lint/linter/__tests__/xhpast/empty.lint-test b/src/lint/linter/__tests__/xhpast/empty.lint-test
index 57595708..ddbbaebd 100644
--- a/src/lint/linter/__tests__/xhpast/empty.lint-test
+++ b/src/lint/linter/__tests__/xhpast/empty.lint-test
@@ -1,3 +1,4 @@
-<?php ?>
+<?php
+
 ~~~~~~~~~~
 warning::
diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php
index 95a9918f..8321a27e 100644
--- a/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php
+++ b/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php
@@ -12,7 +12,7 @@ final class ArcanistPHPCloseTagXHPASTLinterRule
   public function process(XHPASTNode $root) {
     $inline_html = $root->selectDescendantsOfType('n_INLINE_HTML');
 
-    if ($inline_html) {
+    if (count($inline_html) > 0) {
       return;
     }
 
diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php
new file mode 100644
index 00000000..b7c2067e
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php
@@ -0,0 +1,10 @@
+<?php
+
+final class ArcanistPHPCloseTagXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(dirname(__FILE__).'/php-close-tag/');
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test
new file mode 100644
index 00000000..826beb54
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test
@@ -0,0 +1,6 @@
+<?php
+?>
+stuff.
+<?php
+?>
+~~~~~~~~~~
diff --git a/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test
new file mode 100644
index 00000000..54fb1fc5
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test
@@ -0,0 +1,5 @@
+<?php
+return;
+?>
+~~~~~~~~~~
+error:3:1

From 5eda40337bb4135ca4929617602686302edc7cc0 Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@dropbox.com>
Date: Fri, 4 Aug 2017 12:57:34 -0700
Subject: [PATCH 23/34] Fix missing whitespace in `arc linters --help` message

Test Plan: Ran `arc linters --help`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18344
---
 src/workflow/ArcanistLintersWorkflow.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/workflow/ArcanistLintersWorkflow.php b/src/workflow/ArcanistLintersWorkflow.php
index 9932f3b9..600d1019 100644
--- a/src/workflow/ArcanistLintersWorkflow.php
+++ b/src/workflow/ArcanistLintersWorkflow.php
@@ -36,7 +36,7 @@ EOTEXT
         'param' => 'search',
         'repeat' => true,
         'help' => pht(
-          'Search for linters. Search is case-insensitive, and is performed'.
+          'Search for linters. Search is case-insensitive, and is performed '.
           'against name and description of each linter.'),
       ),
       '*' => 'exact',

From ab0d81bca292756bbfb1bf1cc42c9cae01fc7cc3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 31 Aug 2017 08:13:19 -0700
Subject: [PATCH 24/34] Add basic test coverage for lint console rendering

Summary: Ref T9846. The algorithm here is fairly invovled, so lay down some test coverage before breaking it.

Test Plan: Ran tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18508
---
 src/__phutil_library_map__.php                |  2 +
 .../ArcanistConsoleLintRendererTestCase.php   | 89 +++++++++++++++++++
 .../renderer/__tests__/data/inline.expect     |  8 ++
 src/lint/renderer/__tests__/data/inline.txt   |  1 +
 .../renderer/__tests__/data/simple.expect     | 10 +++
 src/lint/renderer/__tests__/data/simple.txt   |  3 +
 6 files changed, 113 insertions(+)
 create mode 100644 src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
 create mode 100644 src/lint/renderer/__tests__/data/inline.expect
 create mode 100644 src/lint/renderer/__tests__/data/inline.txt
 create mode 100644 src/lint/renderer/__tests__/data/simple.expect
 create mode 100644 src/lint/renderer/__tests__/data/simple.txt

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 2a3cfcf8..00590037 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -87,6 +87,7 @@ phutil_register_library_map(array(
     'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php',
     'ArcanistConfigurationManager' => 'configuration/ArcanistConfigurationManager.php',
     'ArcanistConsoleLintRenderer' => 'lint/renderer/ArcanistConsoleLintRenderer.php',
+    'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php',
@@ -502,6 +503,7 @@ phutil_register_library_map(array(
     'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine',
     'ArcanistConfigurationManager' => 'Phobject',
     'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer',
+    'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
new file mode 100644
index 00000000..aa07850e
--- /dev/null
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -0,0 +1,89 @@
+<?php
+
+final class ArcanistConsoleLintRendererTestCase
+  extends PhutilTestCase {
+
+  public function testRendering() {
+    $map = array(
+      'simple' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => 'a',
+        'replacement' => 'z',
+      ),
+      'inline' => array(
+        'line' => 1,
+        'char' => 7,
+        'original' => 'cat',
+        'replacement' => 'dog',
+      ),
+    );
+
+    $defaults = array(
+      'severity' => ArcanistLintSeverity::SEVERITY_WARNING,
+      'name' => 'Lint Warning',
+      'path' => 'path/to/example.c',
+      'description' => 'Consider this.',
+      'code' => 'WARN123',
+    );
+
+    foreach ($map as $key => $test_case) {
+      $data = $this->readTestData("{$key}.txt");
+      $expect = $this->readTestData("{$key}.expect");
+
+      $test_case = $test_case + $defaults;
+
+      $path = $test_case['path'];
+      $severity = $test_case['severity'];
+      $name = $test_case['name'];
+      $description = $test_case['description'];
+      $code = $test_case['code'];
+
+      $line = $test_case['line'];
+      $char = $test_case['char'];
+
+      $original = idx($test_case, 'original');
+      $replacement = idx($test_case, 'replacement');
+
+      $message = id(new ArcanistLintMessage())
+        ->setPath($path)
+        ->setSeverity($severity)
+        ->setName($name)
+        ->setDescription($description)
+        ->setCode($code)
+        ->setLine($line)
+        ->setChar($char)
+        ->setOriginalText($original)
+        ->setReplacementText($replacement);
+
+      $result = id(new ArcanistLintResult())
+        ->setPath($path)
+        ->setData($data)
+        ->addMessage($message);
+
+      $renderer = new ArcanistConsoleLintRenderer();
+
+      try {
+        PhutilConsoleFormatter::disableANSI(true);
+        $actual = $renderer->renderLintResult($result);
+        PhutilConsoleFormatter::disableANSI(false);
+      } catch (Exception $ex) {
+        PhutilConsoleFormatter::disableANSI(false);
+        throw $ex;
+      }
+
+      $this->assertEqual(
+        $expect,
+        $actual,
+        pht(
+          'Lint rendering for "%s".',
+          $key));
+    }
+  }
+
+  private function readTestData($filename) {
+    $path = dirname(__FILE__).'/data/'.$filename;
+    return Filesystem::readFile($path);
+  }
+
+}
diff --git a/src/lint/renderer/__tests__/data/inline.expect b/src/lint/renderer/__tests__/data/inline.expect
new file mode 100644
index 00000000..eba4522b
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/inline.expect
@@ -0,0 +1,8 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 adjudicated
+        +        adjudidoged
diff --git a/src/lint/renderer/__tests__/data/inline.txt b/src/lint/renderer/__tests__/data/inline.txt
new file mode 100644
index 00000000..1907182f
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/inline.txt
@@ -0,0 +1 @@
+adjudicated
diff --git a/src/lint/renderer/__tests__/data/simple.expect b/src/lint/renderer/__tests__/data/simple.expect
new file mode 100644
index 00000000..43b9910f
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/simple.expect
@@ -0,0 +1,10 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 a
+        +        z
+               2 b
+               3 c
diff --git a/src/lint/renderer/__tests__/data/simple.txt b/src/lint/renderer/__tests__/data/simple.txt
new file mode 100644
index 00000000..de980441
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/simple.txt
@@ -0,0 +1,3 @@
+a
+b
+c

From be67df611856ceb31b31bb45f39be1659702b655 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 31 Aug 2017 09:14:07 -0700
Subject: [PATCH 25/34] Extract and cover the logic for "trimming" a lint
 message

Summary:
Ref T9846. Sometimes, a lint message says to replace "the big bad wolf" with "the huge bad wolf": that is, the original and replacement text are the same at the beginning, or the end, or both.

To make this easier for humans to understand, we want to just show that "big" is being replaced with "huge", not that the entire phrase is being replaced.

This logic currently happens inline in console rendering. Pull it out and cover it so a future change can simplify console rendering.

Test Plan: Ran unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18509
---
 src/__phutil_library_map__.php                |  2 +
 src/lint/ArcanistLintMessage.php              | 71 ++++++++++++++++
 .../__tests__/ArcanistLintMessageTestCase.php | 80 +++++++++++++++++++
 src/lint/linter/__tests__/xml/attr2.lint-test |  4 -
 .../ArcanistConsoleLintRendererTestCase.php   |  9 +++
 .../renderer/__tests__/data/overlap.expect    |  8 ++
 src/lint/renderer/__tests__/data/overlap.txt  |  1 +
 7 files changed, 171 insertions(+), 4 deletions(-)
 create mode 100644 src/lint/__tests__/ArcanistLintMessageTestCase.php
 delete mode 100644 src/lint/linter/__tests__/xml/attr2.lint-test
 create mode 100644 src/lint/renderer/__tests__/data/overlap.expect
 create mode 100644 src/lint/renderer/__tests__/data/overlap.txt

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 00590037..7ab5a448 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -222,6 +222,7 @@ phutil_register_library_map(array(
     'ArcanistLibraryTestCase' => '__tests__/ArcanistLibraryTestCase.php',
     'ArcanistLintEngine' => 'lint/engine/ArcanistLintEngine.php',
     'ArcanistLintMessage' => 'lint/ArcanistLintMessage.php',
+    'ArcanistLintMessageTestCase' => 'lint/__tests__/ArcanistLintMessageTestCase.php',
     'ArcanistLintPatcher' => 'lint/ArcanistLintPatcher.php',
     'ArcanistLintRenderer' => 'lint/renderer/ArcanistLintRenderer.php',
     'ArcanistLintResult' => 'lint/ArcanistLintResult.php',
@@ -638,6 +639,7 @@ phutil_register_library_map(array(
     'ArcanistLibraryTestCase' => 'PhutilLibraryTestCase',
     'ArcanistLintEngine' => 'Phobject',
     'ArcanistLintMessage' => 'Phobject',
+    'ArcanistLintMessageTestCase' => 'PhutilTestCase',
     'ArcanistLintPatcher' => 'Phobject',
     'ArcanistLintRenderer' => 'Phobject',
     'ArcanistLintResult' => 'Phobject',
diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php
index dafc28ba..eb575f58 100644
--- a/src/lint/ArcanistLintMessage.php
+++ b/src/lint/ArcanistLintMessage.php
@@ -274,4 +274,75 @@ final class ArcanistLintMessage extends Phobject {
     return $value;
   }
 
+  public function newTrimmedMessage() {
+    if (!$this->isPatchable()) {
+      return clone $this;
+    }
+
+    // If the original and replacement text have a similar prefix or suffix,
+    // we trim it to reduce the size of the diff we show to the user.
+
+    $replacement = $this->getReplacementText();
+    $original = $this->getOriginalText();
+
+    $replacement_length = strlen($replacement);
+    $original_length = strlen($original);
+
+    $minimum_length = min($original_length, $replacement_length);
+
+    $prefix_length = 0;
+    for ($ii = 0; $ii < $minimum_length; $ii++) {
+      if ($original[$ii] !== $replacement[$ii]) {
+        break;
+      }
+      $prefix_length++;
+    }
+
+    // NOTE: The two strings can't be the same because the message won't be
+    // "patchable" if they are, so we don't need a special check for the case
+    // where the entire string is a shared prefix.
+
+    $suffix_length = 0;
+    for ($ii = 1; $ii <= $minimum_length; $ii++) {
+      $original_char = $original[$original_length - $ii];
+      $replacement_char = $replacement[$replacement_length - $ii];
+      if ($original_char !== $replacement_char) {
+        break;
+      }
+      $suffix_length++;
+    }
+
+    if ($suffix_length) {
+      $original = substr($original, 0, -$suffix_length);
+      $replacement = substr($replacement, 0, -$suffix_length);
+    }
+
+    $line = $this->getLine();
+    $char = $this->getChar();
+
+    if ($prefix_length) {
+      $prefix = substr($original, 0, $prefix_length);
+
+      $original = substr($original, $prefix_length);
+      $replacement = substr($replacement, $prefix_length);
+
+      // If we've removed a prefix, we need to push the character and line
+      // number for the warning forward to account for the characters we threw
+      // away.
+      for ($ii = 0; $ii < $prefix_length; $ii++) {
+        $char++;
+        if ($prefix[$ii] == "\n") {
+          $line++;
+          $char = 1;
+        }
+      }
+    }
+
+    return id(clone $this)
+      ->setOriginalText($original)
+      ->setReplacementText($replacement)
+      ->setLine($line)
+      ->setChar($char);
+  }
+
 }
diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php
new file mode 100644
index 00000000..ae02b2b6
--- /dev/null
+++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php
@@ -0,0 +1,80 @@
+<?php
+
+final class ArcanistLintMessageTestCase
+  extends PhutilTestCase {
+
+  public function testMessageTrimming() {
+    $map = array(
+      'simple' => array(
+        'old' => 'a',
+        'new' => 'b',
+        'old.expect' => 'a',
+        'new.expect' => 'b',
+        'line' => 1,
+        'char' => 1,
+      ),
+      'prefix' => array(
+        'old' => 'ever after',
+        'new' => 'evermore',
+        'old.expect' => ' after',
+        'new.expect' => 'more',
+        'line' => 1,
+        'char' => 5,
+      ),
+      'suffix' => array(
+        'old' => 'arcane archaeology',
+        'new' => 'mythic archaeology',
+        'old.expect' => 'arcane',
+        'new.expect' => 'mythic',
+        'line' => 1,
+        'char' => 1,
+      ),
+      'both' => array(
+        'old' => 'large red apple',
+        'new' => 'large blue apple',
+        'old.expect' => 'red',
+        'new.expect' => 'blue',
+        'line' => 1,
+        'char' => 7,
+      ),
+      'prefix-newline' => array(
+        'old' => "four score\nand five years ago",
+        'new' => "four score\nand seven years ago",
+        'old.expect' => 'five',
+        'new.expect' => 'seven',
+        'line' => 2,
+        'char' => 5,
+      ),
+    );
+
+    foreach ($map as $key => $test_case) {
+      $message = id(new ArcanistLintMessage())
+        ->setOriginalText($test_case['old'])
+        ->setReplacementText($test_case['new'])
+        ->setLine(1)
+        ->setChar(1);
+
+      $actual = $message->newTrimmedMessage();
+
+      $this->assertEqual(
+        $test_case['old.expect'],
+        $actual->getOriginalText(),
+        pht('Original text for "%s".', $key));
+
+      $this->assertEqual(
+        $test_case['new.expect'],
+        $actual->getReplacementText(),
+        pht('Replacement text for "%s".', $key));
+
+      $this->assertEqual(
+        $test_case['line'],
+        $actual->getLine(),
+        pht('Line for "%s".', $key));
+
+      $this->assertEqual(
+        $test_case['char'],
+        $actual->getChar(),
+        pht('Char for "%s".', $key));
+    }
+  }
+}
diff --git a/src/lint/linter/__tests__/xml/attr2.lint-test b/src/lint/linter/__tests__/xml/attr2.lint-test
deleted file mode 100644
index 021e0200..00000000
--- a/src/lint/linter/__tests__/xml/attr2.lint-test
+++ /dev/null
@@ -1,4 +0,0 @@
-<foo foo=">oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
-~~~~~~~~~~
-error:1:2
-error:2:1
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index aa07850e..320c00fb 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -17,6 +17,15 @@ final class ArcanistConsoleLintRendererTestCase
         'original' => 'cat',
         'replacement' => 'dog',
       ),
+
+      // In this test, the original and replacement texts have a large
+      // amount of overlap.
+      'overlap' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => 'tantawount',
+        'replacement' => 'tantamount',
+      ),
     );
 
     $defaults = array(
diff --git a/src/lint/renderer/__tests__/data/overlap.expect b/src/lint/renderer/__tests__/data/overlap.expect
new file mode 100644
index 00000000..19de1c46
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/overlap.expect
@@ -0,0 +1,8 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 tantawount
+        +        tantamount
diff --git a/src/lint/renderer/__tests__/data/overlap.txt b/src/lint/renderer/__tests__/data/overlap.txt
new file mode 100644
index 00000000..87458fdd
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/overlap.txt
@@ -0,0 +1 @@
+tantawount

From 86779b152654b81ac17ad4b24d585edddf9917d5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 31 Aug 2017 11:40:28 -0700
Subject: [PATCH 26/34] Render lint patches which have newlines in a less
 misleading way

Summary:
Ref T9846. This rewrites the rendering algorithm in a mostly-compatible way and fixes the major issue.

  - Includes test coverage for removing a newline, from T12765.
  - Includes test coverage for mangling an XML tag, from T9846.

This omits two features, which I'll port forward separately:

  - For one-line patches, highlighting the patched section.
  - For zero-line patches, putting a little caret ("^") under the character where the warning occurred.

I'll restore these features in a followup change.

Test Plan: Ran unit tests, linted a few things.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18510
---
 .../renderer/ArcanistConsoleLintRenderer.php  | 281 +++++++++---------
 .../ArcanistConsoleLintRendererTestCase.php   |  33 ++
 .../renderer/__tests__/data/addline.expect    |  12 +
 src/lint/renderer/__tests__/data/addline.txt  |   5 +
 .../__tests__/data/addlinesuffix.expect       |  12 +
 .../renderer/__tests__/data/addlinesuffix.txt |   5 +
 .../renderer/__tests__/data/newline.expect    |  14 +
 src/lint/renderer/__tests__/data/newline.txt  |  11 +
 src/lint/renderer/__tests__/data/xml.expect   |  12 +
 src/lint/renderer/__tests__/data/xml.txt      |   4 +
 10 files changed, 254 insertions(+), 135 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/addline.expect
 create mode 100644 src/lint/renderer/__tests__/data/addline.txt
 create mode 100644 src/lint/renderer/__tests__/data/addlinesuffix.expect
 create mode 100644 src/lint/renderer/__tests__/data/addlinesuffix.txt
 create mode 100644 src/lint/renderer/__tests__/data/newline.expect
 create mode 100644 src/lint/renderer/__tests__/data/newline.txt
 create mode 100644 src/lint/renderer/__tests__/data/xml.expect
 create mode 100644 src/lint/renderer/__tests__/data/xml.txt

diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index 6f3476b0..20e79b2f 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -15,11 +15,11 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
   public function renderLintResult(ArcanistLintResult $result) {
     $messages = $result->getMessages();
     $path = $result->getPath();
+    $data = $result->getData();
 
-    $lines = explode("\n", $result->getData());
+    $line_map = $this->newOffsetMap($data);
 
     $text = array();
-
     foreach ($messages as $message) {
       if (!$this->showAutofixPatches && $message->isAutofix()) {
         continue;
@@ -57,7 +57,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
         phutil_console_wrap($description, 4));
 
       if ($message->hasFileContext()) {
-        $text[] = $this->renderContext($message, $lines);
+        $text[] = $this->renderContext($message, $data, $line_map);
       }
     }
 
@@ -75,153 +75,148 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
 
   protected function renderContext(
     ArcanistLintMessage $message,
-    array $line_data) {
+    $data,
+    array $line_map) {
+
+    $context = 3;
+
+    $message = $message->newTrimmedMessage();
+
+    $original = $message->getOriginalText();
+    $replacement = $message->getReplacementText();
+
+    $line = $message->getLine();
+    $char = $message->getChar();
+
+    $old = $data;
+    $old_lines = phutil_split_lines($old);
+
+    if ($message->isPatchable()) {
+      $patch_offset = $line_map[$line] + ($char - 1);
+
+      $new = substr_replace(
+        $old,
+        $replacement,
+        $patch_offset,
+        strlen($original));
+      $new_lines = phutil_split_lines($new);
+
+      // Figure out how many "-" and "+" lines we have by counting the newlines
+      // for the relevant patches. This may overestimate things if we are adding
+      // or removing entire lines, but we'll adjust things below.
+      $old_impact = substr_count($original, "\n") + 1;
+      $new_impact = substr_count($replacement, "\n") + 1;
+
+      $start = $line;
+
+      // If lines at the beginning of the changed line range are actually the
+      // same, shrink the range. This happens when a patch just adds a line.
+      do {
+        if ($old_lines[$start - 1] != $new_lines[$start - 1]) {
+          break;
+        }
+
+        $start++;
+        $old_impact--;
+        $new_impact--;
+
+        if ($old_impact < 0 || $new_impact < 0) {
+          throw new Exception(
+            pht(
+              'Modified prefix line range has become negative '.
+              '(old = %d, new = %d).',
+              $old_impact,
+              $new_impact));
+        }
+      } while (true);
+
+      // If the lines at the end of the changed line range are actually the
+      // same, shrink the range. This happens when a patch just removes a
+      // line.
+      do {
+        $old_suffix = $old_lines[$start + $old_impact - 2];
+        $new_suffix = $new_lines[$start + $new_impact - 2];
+
+        if ($old_suffix != $new_suffix) {
+          break;
+        }
+
+        $old_impact--;
+        $new_impact--;
+
+        if ($old_impact < 0 || $new_impact < 0) {
+          throw new Exception(
+            pht(
+              'Modified suffix line range has become negative '.
+              '(old = %d, new = %d).',
+              $old_impact,
+              $new_impact));
+        }
+      } while (true);
+
+    } else {
+      $old_impact = 0;
+      $new_impact = 0;
+    }
 
-    $lines_of_context = 3;
     $out = array();
 
-    $num_lines = count($line_data);
-     // make line numbers line up with array indexes
-    array_unshift($line_data, '');
-
-    $line_num = min($message->getLine(), $num_lines);
-    $line_num = max(1, $line_num);
-
-    // Print out preceding context before the impacted region.
-    $cursor = max(1, $line_num - $lines_of_context);
-    for (; $cursor < $line_num; $cursor++) {
-      $out[] = $this->renderLine($cursor, $line_data[$cursor]);
+    $head = max(1, $start - $context);
+    for ($ii = $head; $ii < $start; $ii++) {
+      $out[] = array(
+        'text' => $old_lines[$ii - 1],
+        'number' => $ii,
+      );
     }
 
-    $text = $message->getOriginalText();
-    $start = $message->getChar() - 1;
-    $patch = '';
-    // Refine original and replacement text to eliminate start and end in common
-    if ($message->isPatchable()) {
-      $patch = $message->getReplacementText();
-      $text_strlen = strlen($text);
-      $patch_strlen = strlen($patch);
-      $min_length = min($text_strlen, $patch_strlen);
-
-      $same_at_front = 0;
-      for ($ii = 0; $ii < $min_length; $ii++) {
-        if ($text[$ii] !== $patch[$ii]) {
-          break;
-        }
-        $same_at_front++;
-        $start++;
-        if ($text[$ii] == "\n") {
-          $out[] = $this->renderLine($cursor, $line_data[$cursor]);
-          $cursor++;
-          $start = 0;
-          $line_num++;
-        }
-      }
-      // deal with shorter string '     ' longer string '     a     '
-      $min_length -= $same_at_front;
-
-      // And check the end of the string
-      $same_at_end = 0;
-      for ($ii = 1; $ii <= $min_length; $ii++) {
-        if ($text[$text_strlen - $ii] !== $patch[$patch_strlen - $ii]) {
-          break;
-        }
-        $same_at_end++;
-      }
-
-      $text = substr(
-        $text,
-        $same_at_front,
-        $text_strlen - $same_at_end - $same_at_front);
-      $patch = substr(
-        $patch,
-        $same_at_front,
-        $patch_strlen - $same_at_end - $same_at_front);
-    }
-    // Print out the impacted region itself.
-    $diff = $message->isPatchable() ? '-' : null;
-
-    $text_lines = explode("\n", $text);
-    $text_length = count($text_lines);
-
-    $intraline = ($text != '' || $start || !preg_match('/\n$/', $patch));
-
-    if ($intraline) {
-      for (; $cursor < $line_num + $text_length; $cursor++) {
-        $chevron = ($cursor == $line_num);
-        // We may not have any data if, e.g., the old file does not exist.
-        $data = idx($line_data, $cursor, null);
-
-        // Highlight the problem substring.
-        $text_line = $text_lines[$cursor - $line_num];
-        if (strlen($text_line)) {
-          $data = substr_replace(
-            $data,
-            phutil_console_format('##%s##', $text_line),
-            ($cursor == $line_num ? ($start > 0 ? $start : null) : 0),
-            strlen($text_line));
-        }
-
-        $out[] = $this->renderLine($cursor, $data, $chevron, $diff);
-      }
+    for ($ii = $start; $ii < $start + $old_impact; $ii++) {
+      $out[] = array(
+        'text' => $old_lines[$ii - 1],
+        'number' => $ii,
+        'type' => '-',
+        'chevron' => ($ii == $start),
+      );
     }
 
-    // Print out replacement text.
-    if ($message->isPatchable()) {
-      // Strip trailing newlines, since "explode" will create an extra patch
-      // line for these.
-      if (strlen($patch) && ($patch[strlen($patch) - 1] === "\n")) {
-        $patch = substr($patch, 0, -1);
-      }
-      $patch_lines = explode("\n", $patch);
-      $patch_length = count($patch_lines);
-
-      $patch_line = $patch_lines[0];
-
-      $len = isset($text_lines[0]) ? strlen($text_lines[0]) : 0;
-
-      $patched = phutil_console_format('##%s##', $patch_line);
-
-      if ($intraline) {
-        $patched = substr_replace(
-          $line_data[$line_num],
-          $patched,
-          $start,
-          $len);
-      }
-
-      $out[] = $this->renderLine(null, $patched, false, '+');
-
-      foreach (array_slice($patch_lines, 1) as $patch_line) {
-        $out[] = $this->renderLine(
-          null,
-          phutil_console_format('##%s##', $patch_line), false, '+');
-      }
+    for ($ii = $start; $ii < $start + $new_impact; $ii++) {
+      $out[] = array(
+        'text' => $new_lines[$ii - 1],
+        'type' => '+',
+        'chevron' => ($ii == $start),
+      );
     }
 
-    $end = min($num_lines, $cursor + $lines_of_context);
-    for (; $cursor < $end; $cursor++) {
-      // If there is no original text, we didn't print out a chevron or any
-      // highlighted text above, so print it out here. This allows messages
-      // which don't have any original/replacement information to still
-      // render with indicator chevrons.
-      if ($text || $message->isPatchable()) {
+    $cursor = $start + $old_impact;
+    $foot = min(count($old_lines), $cursor + $context);
+    for ($ii = $cursor; $ii <= $foot; $ii++) {
+      $out[] = array(
+        'text' => $old_lines[$ii - 1],
+        'number' => $ii,
+        'chevron' => ($ii == $cursor),
+      );
+    }
+
+    $result = array();
+
+    $seen_chevron = false;
+    foreach ($out as $spec) {
+      if ($seen_chevron) {
         $chevron = false;
       } else {
-        $chevron = ($cursor == $line_num);
+        $chevron = !empty($spec['chevron']);
+        if ($chevron) {
+          $seen_chevron = true;
+        }
       }
-      $out[] = $this->renderLine($cursor, $line_data[$cursor], $chevron);
 
-      // With original text, we'll render the text highlighted above. If the
-      // lint message only has a line/char offset there's nothing to
-      // highlight, so print out a caret on the next line instead.
-      if ($chevron && $message->getChar()) {
-        $out[] = $this->renderCaret($message->getChar());
-      }
+      $result[] = $this->renderLine(
+        idx($spec, 'number'),
+        $spec['text'],
+        $chevron,
+        idx($spec, 'type'));
     }
-    $out[] = null;
 
-    return implode("\n", $out);
+    return implode('', $result);
   }
 
   private function renderCaret($pos) {
@@ -245,4 +240,20 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       pht('No lint warnings.'));
   }
 
+  private function newOffsetMap($data) {
+    $lines = phutil_split_lines($data);
+
+    $line_map = array();
+
+    $number = 1;
+    $offset = 0;
+    foreach ($lines as $line) {
+      $line_map[$number] = $offset;
+      $number++;
+      $offset += strlen($line);
+    }
+
+    return $line_map;
+  }
+
 }
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index 320c00fb..7f39d352 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -26,6 +26,34 @@ final class ArcanistConsoleLintRendererTestCase
         'original' => 'tantawount',
         'replacement' => 'tantamount',
       ),
+
+      'newline' => array(
+        'line' => 6,
+        'char' => 1,
+        'original' => "\n",
+        'replacement' => '',
+      ),
+
+      'addline' => array(
+        'line' => 3,
+        'char' => 1,
+        'original' => '',
+        'replacement' => "cherry\n",
+      ),
+
+      'addlinesuffix' => array(
+        'line' => 2,
+        'char' => 7,
+        'original' => '',
+        'replacement' => "\ncherry",
+      ),
+
+      'xml' => array(
+        'line' => 3,
+        'char' => 6,
+        'original' => '',
+        'replacement' => "\n",
+      ),
     );
 
     $defaults = array(
@@ -81,6 +109,11 @@ final class ArcanistConsoleLintRendererTestCase
         throw $ex;
       }
 
+      // Trim "~" off the ends of lines. This allows the "expect" file to test
+      // for trailing whitespace without actually containing trailing
+      // whitespace.
+      $expect = preg_replace('/~+$/m', '', $expect);
+
       $this->assertEqual(
         $expect,
         $actual,
diff --git a/src/lint/renderer/__tests__/data/addline.expect b/src/lint/renderer/__tests__/data/addline.expect
new file mode 100644
index 00000000..44aa945c
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/addline.expect
@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 apple
+               2 banana
+    >>> +        cherry
+               3 date
+               4 eclaire
+               5 fig
diff --git a/src/lint/renderer/__tests__/data/addline.txt b/src/lint/renderer/__tests__/data/addline.txt
new file mode 100644
index 00000000..07147513
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/addline.txt
@@ -0,0 +1,5 @@
+apple
+banana
+date
+eclaire
+fig
diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.expect b/src/lint/renderer/__tests__/data/addlinesuffix.expect
new file mode 100644
index 00000000..44aa945c
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/addlinesuffix.expect
@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 apple
+               2 banana
+    >>> +        cherry
+               3 date
+               4 eclaire
+               5 fig
diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.txt b/src/lint/renderer/__tests__/data/addlinesuffix.txt
new file mode 100644
index 00000000..07147513
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/addlinesuffix.txt
@@ -0,0 +1,5 @@
+apple
+banana
+date
+eclaire
+fig
diff --git a/src/lint/renderer/__tests__/data/newline.expect b/src/lint/renderer/__tests__/data/newline.expect
new file mode 100644
index 00000000..0533707c
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/newline.expect
@@ -0,0 +1,14 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               3 ccc
+               4 ddd
+               5 eee
+    >>> -      6 ~
+               7 fff
+               8 ggg
+               9 hhh
+              10 iii
diff --git a/src/lint/renderer/__tests__/data/newline.txt b/src/lint/renderer/__tests__/data/newline.txt
new file mode 100644
index 00000000..830c73a1
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/newline.txt
@@ -0,0 +1,11 @@
+aaa
+bbb
+ccc
+ddd
+eee
+
+fff
+ggg
+hhh
+iii
+jjj
diff --git a/src/lint/renderer/__tests__/data/xml.expect b/src/lint/renderer/__tests__/data/xml.expect
new file mode 100644
index 00000000..767ea655
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/xml.expect
@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 <
+               2   wow
+    >>> -      3   xml>
+        +          xml
+        +        >
+               4 <xml />
diff --git a/src/lint/renderer/__tests__/data/xml.txt b/src/lint/renderer/__tests__/data/xml.txt
new file mode 100644
index 00000000..136a688e
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/xml.txt
@@ -0,0 +1,4 @@
+<
+  wow
+  xml>
+<xml />

From 213ed3ff15d71ac9f44b4435f857826be5a41705 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 31 Aug 2017 12:37:19 -0700
Subject: [PATCH 27/34] Restore the caret pointer ("^") for lint lines which
 only have a character offset

Summary: Ref T9846. This was dropped when I refactored how things are rendered; restore it.

Test Plan: Added unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18511
---
 src/lint/renderer/ArcanistConsoleLintRenderer.php     | 11 +++++++++--
 .../__tests__/ArcanistConsoleLintRendererTestCase.php |  7 +++++++
 src/lint/renderer/__tests__/data/caret.expect         | 11 +++++++++++
 src/lint/renderer/__tests__/data/caret.txt            |  4 ++++
 4 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/caret.expect
 create mode 100644 src/lint/renderer/__tests__/data/caret.txt

diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index 20e79b2f..4acbb644 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -90,6 +90,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
 
     $old = $data;
     $old_lines = phutil_split_lines($old);
+    $start = $line;
 
     if ($message->isPatchable()) {
       $patch_offset = $line_map[$line] + ($char - 1);
@@ -107,8 +108,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       $old_impact = substr_count($original, "\n") + 1;
       $new_impact = substr_count($replacement, "\n") + 1;
 
-      $start = $line;
-
       // If lines at the beginning of the changed line range are actually the
       // same, shrink the range. This happens when a patch just adds a line.
       do {
@@ -214,6 +213,14 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
         $spec['text'],
         $chevron,
         idx($spec, 'type'));
+
+      // If this is just a message and does not have a patch, put a little
+      // caret underneath the line to point out where the issue is.
+      if ($chevron) {
+        if (!$message->isPatchable() && !strlen($original)) {
+          $result[] = $this->renderCaret($char)."\n";
+        }
+      }
     }
 
     return implode('', $result);
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index 7f39d352..693b1788 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -54,6 +54,13 @@ final class ArcanistConsoleLintRendererTestCase
         'original' => '',
         'replacement' => "\n",
       ),
+
+      'caret' => array(
+        'line' => 2,
+        'char' => 13,
+        'name' => 'Fruit Misinformation',
+        'description' => 'Arguably untrue.',
+      ),
     );
 
     $defaults = array(
diff --git a/src/lint/renderer/__tests__/data/caret.expect b/src/lint/renderer/__tests__/data/caret.expect
new file mode 100644
index 00000000..1bb26faa
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/caret.expect
@@ -0,0 +1,11 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Fruit Misinformation
+    Arguably untrue.
+
+               1 Apples are round.
+    >>>        2 Bananas are round.
+                             ^
+               3 Cherries are round.
+               4 Dates are round.
diff --git a/src/lint/renderer/__tests__/data/caret.txt b/src/lint/renderer/__tests__/data/caret.txt
new file mode 100644
index 00000000..a36a81cf
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/caret.txt
@@ -0,0 +1,4 @@
+Apples are round.
+Bananas are round.
+Cherries are round.
+Dates are round.

From ad8214456add127935b2a49119d25670d9ef7963 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 31 Aug 2017 12:50:06 -0700
Subject: [PATCH 28/34] Restore ANSI highlighting of lint sections to lint
 output

Summary: Fixes T9846. This restores the last missing feature, ANSI highlighting of diff sections.

Test Plan:
Added a mode so we can actually test this stuff, activated that mode, wrote unit tests.

Did a bunch of actual lint locally too and looked at it, all seemed sane.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18512
---
 .../renderer/ArcanistConsoleLintRenderer.php  | 50 ++++++++++++++++++-
 .../ArcanistConsoleLintRendererTestCase.php   |  9 +++-
 .../renderer/__tests__/data/inline.expect     |  4 +-
 .../renderer/__tests__/data/original.expect   |  7 +++
 src/lint/renderer/__tests__/data/original.txt |  1 +
 .../renderer/__tests__/data/overlap.expect    |  4 +-
 .../renderer/__tests__/data/simple.expect     |  4 +-
 7 files changed, 71 insertions(+), 8 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/original.expect
 create mode 100644 src/lint/renderer/__tests__/data/original.txt

diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index 4acbb644..8880fbe8 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -6,12 +6,22 @@
 final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
 
   private $showAutofixPatches = false;
+  private $testableMode;
 
   public function setShowAutofixPatches($show_autofix_patches) {
     $this->showAutofixPatches = $show_autofix_patches;
     return $this;
   }
 
+  public function setTestableMode($testable_mode) {
+    $this->testableMode = $testable_mode;
+    return $this;
+  }
+
+  public function getTestableMode() {
+    return $this->testableMode;
+  }
+
   public function renderLintResult(ArcanistLintResult $result) {
     $messages = $result->getMessages();
     $path = $result->getPath();
@@ -90,6 +100,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
 
     $old = $data;
     $old_lines = phutil_split_lines($old);
+    $old_impact = substr_count($original, "\n") + 1;
     $start = $line;
 
     if ($message->isPatchable()) {
@@ -105,9 +116,26 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       // Figure out how many "-" and "+" lines we have by counting the newlines
       // for the relevant patches. This may overestimate things if we are adding
       // or removing entire lines, but we'll adjust things below.
-      $old_impact = substr_count($original, "\n") + 1;
       $new_impact = substr_count($replacement, "\n") + 1;
 
+
+      // If this is a change on a single line, we'll try to highlight the
+      // changed character range to make it easier to pick out.
+      if ($old_impact === 1 && $new_impact === 1) {
+        $old_lines[$start - 1] = substr_replace(
+          $old_lines[$start - 1],
+          $this->highlightText($original),
+          $char - 1,
+          strlen($original));
+
+        $new_lines[$start - 1] = substr_replace(
+          $new_lines[$start - 1],
+          $this->highlightText($replacement),
+          $char - 1,
+          strlen($replacement));
+      }
+
+
       // If lines at the beginning of the changed line range are actually the
       // same, shrink the range. This happens when a patch just adds a line.
       do {
@@ -154,6 +182,18 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       } while (true);
 
     } else {
+
+      // If we have "original" text and it is contained on a single line,
+      // highlight the affected area. If we don't have any text, we'll mark
+      // the character with a caret (below, in rendering) instead.
+      if ($old_impact == 1 && strlen($original)) {
+        $old_lines[$start - 1] = substr_replace(
+          $old_lines[$start - 1],
+          $this->highlightText($original),
+          $char - 1,
+          strlen($original));
+      }
+
       $old_impact = 0;
       $new_impact = 0;
     }
@@ -263,4 +303,12 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
     return $line_map;
   }
 
+  private function highlightText($text) {
+    if ($this->getTestableMode()) {
+      return '>'.$text.'<';
+    } else {
+      return (string)tsprintf('##%s##', $text);
+    }
+  }
+
 }
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index 693b1788..68a35420 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -61,6 +61,12 @@ final class ArcanistConsoleLintRendererTestCase
         'name' => 'Fruit Misinformation',
         'description' => 'Arguably untrue.',
       ),
+
+      'original' => array(
+        'line' => 1,
+        'char' => 4,
+        'original' => 'should of',
+      ),
     );
 
     $defaults = array(
@@ -105,7 +111,8 @@ final class ArcanistConsoleLintRendererTestCase
         ->setData($data)
         ->addMessage($message);
 
-      $renderer = new ArcanistConsoleLintRenderer();
+      $renderer = id(new ArcanistConsoleLintRenderer())
+        ->setTestableMode(true);
 
       try {
         PhutilConsoleFormatter::disableANSI(true);
diff --git a/src/lint/renderer/__tests__/data/inline.expect b/src/lint/renderer/__tests__/data/inline.expect
index eba4522b..d1d63fda 100644
--- a/src/lint/renderer/__tests__/data/inline.expect
+++ b/src/lint/renderer/__tests__/data/inline.expect
@@ -4,5 +4,5 @@
    Warning  (WARN123) Lint Warning
     Consider this.
 
-    >>> -      1 adjudicated
-        +        adjudidoged
+    >>> -      1 adjudi>cat<ed
+        +        adjudi>dog<ed
diff --git a/src/lint/renderer/__tests__/data/original.expect b/src/lint/renderer/__tests__/data/original.expect
new file mode 100644
index 00000000..f3bec302
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/original.expect
@@ -0,0 +1,7 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>>        1 He >should of< known.
diff --git a/src/lint/renderer/__tests__/data/original.txt b/src/lint/renderer/__tests__/data/original.txt
new file mode 100644
index 00000000..090fe16d
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/original.txt
@@ -0,0 +1 @@
+He should of known.
diff --git a/src/lint/renderer/__tests__/data/overlap.expect b/src/lint/renderer/__tests__/data/overlap.expect
index 19de1c46..d7756f3d 100644
--- a/src/lint/renderer/__tests__/data/overlap.expect
+++ b/src/lint/renderer/__tests__/data/overlap.expect
@@ -4,5 +4,5 @@
    Warning  (WARN123) Lint Warning
     Consider this.
 
-    >>> -      1 tantawount
-        +        tantamount
+    >>> -      1 tanta>w<ount
+        +        tanta>m<ount
diff --git a/src/lint/renderer/__tests__/data/simple.expect b/src/lint/renderer/__tests__/data/simple.expect
index 43b9910f..c363ba07 100644
--- a/src/lint/renderer/__tests__/data/simple.expect
+++ b/src/lint/renderer/__tests__/data/simple.expect
@@ -4,7 +4,7 @@
    Warning  (WARN123) Lint Warning
     Consider this.
 
-    >>> -      1 a
-        +        z
+    >>> -      1 >a<
+        +        >z<
                2 b
                3 c

From d9cb5b18fbc1a22630eeaa16da7d291b206fba21 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 1 Sep 2017 17:11:11 -0700
Subject: [PATCH 29/34] Cheat our way through `arc version` on Windows for the
 moment

Summary: Fixes T8291. See PHI52. This is papering over the real issue (T8298) but it's a 10-second patch so just improve things slightly for now.

Test Plan: Ran `arc version` locally; patch confirmed on a Windows system by an affected user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8291

Differential Revision: https://secure.phabricator.com/D18518
---
 src/workflow/ArcanistVersionWorkflow.php | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/workflow/ArcanistVersionWorkflow.php b/src/workflow/ArcanistVersionWorkflow.php
index 807c5732..9c470fc6 100644
--- a/src/workflow/ArcanistVersionWorkflow.php
+++ b/src/workflow/ArcanistVersionWorkflow.php
@@ -51,8 +51,14 @@ EOTEXT
           pht('%s is not a git working copy.', $lib));
       }
 
-      list($stdout) = $repository->execxLocal('log -1 --format=%s', '%H %ct');
-      list($commit, $timestamp) = explode(' ', $stdout);
+      // NOTE: Carefully execute these commands in a way that works on Windows
+      // until T8298 is properly fixed. See PHI52.
+
+      list($commit) = $repository->execxLocal('log -1 --format=%%H');
+      $commit = trim($commit);
+
+      list($timestamp) = $repository->execxLocal('log -1 --format=%%ct');
+      $timestamp = trim($timestamp);
 
       $console->writeOut(
         "%s %s (%s)\n",

From 7371277d20650160c5192417cd9aefaf132372f5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Sep 2017 11:49:51 -0700
Subject: [PATCH 30/34] Fix a prefix/suffix counting issue in Arcanist lint
 rendering

Summary: Ref T9846. See PHI48. For replacing text in the form "ABC" with "ABBC", the trimmer had a bug.

Test Plan: Added failing tests, fixed 'em.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18538
---
 src/lint/ArcanistLintMessage.php              | 14 +++++++++---
 .../__tests__/ArcanistLintMessageTestCase.php |  8 +++++++
 .../ArcanistConsoleLintRendererTestCase.php   | 22 +++++++++++++++++++
 .../renderer/__tests__/data/midline.expect    | 11 ++++++++++
 src/lint/renderer/__tests__/data/midline.txt  |  4 ++++
 5 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/midline.expect
 create mode 100644 src/lint/renderer/__tests__/data/midline.txt

diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php
index eb575f58..7be1d52d 100644
--- a/src/lint/ArcanistLintMessage.php
+++ b/src/lint/ArcanistLintMessage.php
@@ -302,8 +302,13 @@ final class ArcanistLintMessage extends Phobject {
     // "patchable" if they are, so we don't need a special check for the case
     // where the entire string is a shared prefix.
 
+    // However, if the two strings are in the form "ABC" and "ABBC", we may
+    // find a prefix and a suffix with a combined length greater than the
+    // total size of the smaller string if we don't limit the search.
+    $max_suffix = ($minimum_length - $prefix_length);
+
     $suffix_length = 0;
-    for ($ii = 1; $ii <= $minimum_length; $ii++) {
+    for ($ii = 1; $ii <= $max_suffix; $ii++) {
       $original_char = $original[$original_length - $ii];
       $replacement_char = $replacement[$replacement_length - $ii];
       if ($original_char !== $replacement_char) {
@@ -323,8 +328,11 @@ final class ArcanistLintMessage extends Phobject {
     if ($prefix_length) {
       $prefix = substr($original, 0, $prefix_length);
 
-      $original = substr($original, $prefix_length);
-      $replacement = substr($replacement, $prefix_length);
+      // NOTE: Prior to PHP7, `substr("a", 1)` returned false instead of
+      // the empty string. Cast these to force the PHP7-ish behavior we
+      // expect.
+      $original = (string)substr($original, $prefix_length);
+      $replacement = (string)substr($replacement, $prefix_length);
 
       // If we've removed a prefix, we need to push the character and line
       // number for the warning forward to account for the characters we threw
diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php
index ae02b2b6..c27acd1b 100644
--- a/src/lint/__tests__/ArcanistLintMessageTestCase.php
+++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php
@@ -45,6 +45,14 @@ final class ArcanistLintMessageTestCase
         'line' => 2,
         'char' => 5,
       ),
+      'mid-newline' => array(
+        'old' => 'ABA',
+        'new' => 'ABBA',
+        'old.expect' => '',
+        'new.expect' => 'B',
+        'line' => 1,
+        'char' => 3,
+      ),
     );
 
     foreach ($map as $key => $test_case) {
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index 68a35420..bf7bca4b 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -4,6 +4,21 @@ final class ArcanistConsoleLintRendererTestCase
   extends PhutilTestCase {
 
   public function testRendering() {
+    $midline_original = <<<EOTEXT
+import apple;
+import banana;
+import cat;
+import dog;
+EOTEXT;
+
+    $midline_replacement = <<<EOTEXT
+import apple;
+import banana;
+
+import cat;
+import dog;
+EOTEXT;
+
     $map = array(
       'simple' => array(
         'line' => 1,
@@ -67,6 +82,13 @@ final class ArcanistConsoleLintRendererTestCase
         'char' => 4,
         'original' => 'should of',
       ),
+
+      'midline' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => $midline_original,
+        'replacement' => $midline_replacement,
+      ),
     );
 
     $defaults = array(
diff --git a/src/lint/renderer/__tests__/data/midline.expect b/src/lint/renderer/__tests__/data/midline.expect
new file mode 100644
index 00000000..d5f513ea
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/midline.expect
@@ -0,0 +1,11 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 import apple;
+               2 import banana;
+    >>> +        ~
+               3 import cat;
+               4 import dog;
diff --git a/src/lint/renderer/__tests__/data/midline.txt b/src/lint/renderer/__tests__/data/midline.txt
new file mode 100644
index 00000000..8639daf5
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/midline.txt
@@ -0,0 +1,4 @@
+import apple;
+import banana;
+import cat;
+import dog;

From cbc785ddce71be073f536c1faea8c231e495d5df Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Sep 2017 16:50:59 -0700
Subject: [PATCH 31/34] Fix a similar lint rendering issue when trimming
 identical lines out of patches

Summary: Ref T9846. See PHI48. See D18538 for a similar fix. We can contract the suffix lines too much if, e.g, a newline after another newline is removed. Prevent contraction to fewer than 0 lines.

Test Plan: Added a failing test, made it pass.

Reviewers: chad

Reviewed By: chad

Subscribers: alexmv

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18541
---
 .../renderer/ArcanistConsoleLintRenderer.php  | 11 ++++-----
 .../ArcanistConsoleLintRendererTestCase.php   | 24 +++++++++++++++++++
 .../renderer/__tests__/data/remline.expect    | 12 ++++++++++
 src/lint/renderer/__tests__/data/remline.txt  |  6 +++++
 4 files changed, 46 insertions(+), 7 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/remline.expect
 create mode 100644 src/lint/renderer/__tests__/data/remline.txt

diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index 8880fbe8..12ef4e69 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -171,13 +171,10 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
         $old_impact--;
         $new_impact--;
 
-        if ($old_impact < 0 || $new_impact < 0) {
-          throw new Exception(
-            pht(
-              'Modified suffix line range has become negative '.
-              '(old = %d, new = %d).',
-              $old_impact,
-              $new_impact));
+        // We can end up here if a patch removes a line which occurs after
+        // another identical line.
+        if ($old_impact <= 0 || $new_impact <= 0) {
+          break;
         }
       } while (true);
 
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index bf7bca4b..f36b30e7 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -15,6 +15,23 @@ EOTEXT;
 import apple;
 import banana;
 
+import cat;
+import dog;
+EOTEXT;
+
+    $remline_original = <<<EOTEXT
+import apple;
+import banana;
+
+
+import cat;
+import dog;
+EOTEXT;
+
+    $remline_replacement = <<<EOTEXT
+import apple;
+import banana;
+
 import cat;
 import dog;
 EOTEXT;
@@ -89,6 +106,13 @@ EOTEXT;
         'original' => $midline_original,
         'replacement' => $midline_replacement,
       ),
+
+      'remline' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => $remline_original,
+        'replacement' => $remline_replacement,
+      ),
     );
 
     $defaults = array(
diff --git a/src/lint/renderer/__tests__/data/remline.expect b/src/lint/renderer/__tests__/data/remline.expect
new file mode 100644
index 00000000..a4d65c95
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/remline.expect
@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 import apple;
+               2 import banana;
+               3 ~
+    >>> -      4 ~
+               5 import cat;
+               6 import dog;
diff --git a/src/lint/renderer/__tests__/data/remline.txt b/src/lint/renderer/__tests__/data/remline.txt
new file mode 100644
index 00000000..4e526abe
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/remline.txt
@@ -0,0 +1,6 @@
+import apple;
+import banana;
+
+
+import cat;
+import dog;

From df81d79d77f89dfd27529a10c2a0aab6020638c5 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Thu, 14 Sep 2017 11:24:36 -0700
Subject: [PATCH 32/34] Add explicit limits to unit test/lint error names

Summary:
Fixes T12981.

See new `arc lint` output: P2071

See new `arc unit` output: P2072

Test Plan: Ran `arc unit/lint/diff` and observed new error instead of a Conduit error

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12981

Differential Revision: https://secure.phabricator.com/D18603
---
 src/lint/ArcanistLintMessage.php    | 15 +++++++++++++++
 src/unit/ArcanistUnitTestResult.php | 14 ++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php
index 7be1d52d..c92c5ec8 100644
--- a/src/lint/ArcanistLintMessage.php
+++ b/src/lint/ArcanistLintMessage.php
@@ -126,6 +126,21 @@ final class ArcanistLintMessage extends Phobject {
   }
 
   public function setName($name) {
+    $maximum_bytes = 255;
+    $actual_bytes = strlen($name);
+
+    if ($actual_bytes > $maximum_bytes) {
+      throw new Exception(
+        pht(
+          'Parameter ("%s") passed to "%s" when constructing a lint message '.
+          'must be a string with a maximum length of %s bytes, but is %s '.
+          'bytes in length.',
+          $name,
+          'setName()',
+          new PhutilNumber($maximum_bytes),
+          new PhutilNumber($actual_bytes)));
+    }
+
     $this->name = $name;
     return $this;
   }
diff --git a/src/unit/ArcanistUnitTestResult.php b/src/unit/ArcanistUnitTestResult.php
index a9061bd0..ec25c20f 100644
--- a/src/unit/ArcanistUnitTestResult.php
+++ b/src/unit/ArcanistUnitTestResult.php
@@ -30,6 +30,20 @@ final class ArcanistUnitTestResult extends Phobject {
   }
 
   public function setName($name) {
+    $maximum_bytes = 255;
+    $actual_bytes = strlen($name);
+
+    if ($actual_bytes > $maximum_bytes) {
+      throw new Exception(
+        pht(
+          'Parameter ("%s") passed to "%s" when constructing a unit test '.
+          'message must be a string with a maximum length of %s bytes, but '.
+          'is %s bytes in length.',
+          $name,
+          'setName()',
+          new PhutilNumber($maximum_bytes),
+          new PhutilNumber($actual_bytes)));
+    }
     $this->name = $name;
     return $this;
   }

From b836557b12b37376302f5548fe0808e5254f1c22 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 Sep 2017 11:59:18 -0700
Subject: [PATCH 33/34] Correct lint rendering when patching trailing
 whitespace in files

Summary: Ref PHI48. If a patch removes all of the lines at the end of a file, we can get some array index errors.

Test Plan: Added failing test, made it pass.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18631
---
 src/lint/linter/ArcanistTextLinter.php              |  4 +---
 src/lint/renderer/ArcanistConsoleLintRenderer.php   | 13 +++++++------
 .../ArcanistConsoleLintRendererTestCase.php         |  9 +++++++++
 .../renderer/__tests__/data/extrawhitespace.expect  |  8 ++++++++
 .../renderer/__tests__/data/extrawhitespace.txt     |  3 +++
 5 files changed, 28 insertions(+), 9 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/extrawhitespace.expect
 create mode 100644 src/lint/renderer/__tests__/data/extrawhitespace.txt

diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php
index 87a2d8a7..c7c4af52 100644
--- a/src/lint/linter/ArcanistTextLinter.php
+++ b/src/lint/linter/ArcanistTextLinter.php
@@ -311,9 +311,7 @@ final class ArcanistTextLinter extends ArcanistLinter {
     $this->raiseLintAtOffset(
       $offset,
       self::LINT_EOF_WHITESPACE,
-      pht(
-        'This file contains trailing whitespace at the end of the file. '.
-        'This is unnecessary and should be avoided when possible.'),
+      pht('This file contains unnecessary trailing whitespace.'),
       $string,
       '');
   }
diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index 12ef4e69..ea233e3d 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -118,7 +118,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       // or removing entire lines, but we'll adjust things below.
       $new_impact = substr_count($replacement, "\n") + 1;
 
-
       // If this is a change on a single line, we'll try to highlight the
       // changed character range to make it easier to pick out.
       if ($old_impact === 1 && $new_impact === 1) {
@@ -135,11 +134,13 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
           strlen($replacement));
       }
 
-
       // If lines at the beginning of the changed line range are actually the
       // same, shrink the range. This happens when a patch just adds a line.
       do {
-        if ($old_lines[$start - 1] != $new_lines[$start - 1]) {
+        $old_line = idx($old_lines, $start - 1, null);
+        $new_line = idx($new_lines, $start - 1, null);
+
+        if ($old_line !== $new_line) {
           break;
         }
 
@@ -161,10 +162,10 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       // same, shrink the range. This happens when a patch just removes a
       // line.
       do {
-        $old_suffix = $old_lines[$start + $old_impact - 2];
-        $new_suffix = $new_lines[$start + $new_impact - 2];
+        $old_suffix = idx($old_lines, $start + $old_impact - 2, null);
+        $new_suffix = idx($new_lines, $start + $new_impact - 2, null);
 
-        if ($old_suffix != $new_suffix) {
+        if ($old_suffix !== $new_suffix) {
           break;
         }
 
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index f36b30e7..9014288b 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -113,6 +113,13 @@ EOTEXT;
         'original' => $remline_original,
         'replacement' => $remline_replacement,
       ),
+
+      'extrawhitespace' => array(
+        'line' => 2,
+        'char' => 1,
+        'original' => "\n",
+        'replacement' => '',
+      ),
     );
 
     $defaults = array(
@@ -125,6 +132,8 @@ EOTEXT;
 
     foreach ($map as $key => $test_case) {
       $data = $this->readTestData("{$key}.txt");
+      $data = preg_replace('/~+\s*$/m', '', $data);
+
       $expect = $this->readTestData("{$key}.expect");
 
       $test_case = $test_case + $defaults;
diff --git a/src/lint/renderer/__tests__/data/extrawhitespace.expect b/src/lint/renderer/__tests__/data/extrawhitespace.expect
new file mode 100644
index 00000000..f083852d
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/extrawhitespace.expect
@@ -0,0 +1,8 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 Adrift upon the sea.
+    >>> -      2 ~
diff --git a/src/lint/renderer/__tests__/data/extrawhitespace.txt b/src/lint/renderer/__tests__/data/extrawhitespace.txt
new file mode 100644
index 00000000..ba587d4c
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/extrawhitespace.txt
@@ -0,0 +1,3 @@
+Adrift upon the sea.
+
+~

From c804c5026011f27614a7bbdb2bb32cab590d68ca Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@dropbox.com>
Date: Wed, 20 Sep 2017 14:09:04 -0700
Subject: [PATCH 34/34] Don't show a blank line if there is no user data

Summary:
SKIP lines, for instance, often have no UserData; there is no
reason to display a content-less blank line.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18632
---
 src/unit/renderer/ArcanistUnitConsoleRenderer.php | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/unit/renderer/ArcanistUnitConsoleRenderer.php b/src/unit/renderer/ArcanistUnitConsoleRenderer.php
index fa646fc5..6294cdee 100644
--- a/src/unit/renderer/ArcanistUnitConsoleRenderer.php
+++ b/src/unit/renderer/ArcanistUnitConsoleRenderer.php
@@ -21,7 +21,8 @@ final class ArcanistUnitConsoleRenderer extends ArcanistUnitRenderer {
       $this->getFormattedResult($result->getResult()).$duration,
       $test_name);
 
-    if ($result_code != ArcanistUnitTestResult::RESULT_PASS) {
+    if ($result_code != ArcanistUnitTestResult::RESULT_PASS
+        && strlen($result->getUserData())) {
       $return .= $result->getUserData()."\n";
     }