(experimental) Merge branch 'master' into experimental

scripts/arcanist.php

@@ -448,7 +448,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");
@@ -666,16 +667,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)));
     }
   }
 }

src/__phutil_library_map__.php

@@ -100,6 +100,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',
@@ -238,6 +239,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',
@@ -287,6 +289,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',
@@ -543,6 +546,7 @@ phutil_register_library_map(array(
     'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine',
     'ArcanistConfigurationManager' => 'Phobject',
     'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer',
+    'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
@@ -681,6 +685,7 @@ phutil_register_library_map(array(
     'ArcanistLibraryTestCase' => 'PhutilLibraryTestCase',
     'ArcanistLintEngine' => 'Phobject',
     'ArcanistLintMessage' => 'Phobject',
+    'ArcanistLintMessageTestCase' => 'PhutilTestCase',
     'ArcanistLintPatcher' => 'Phobject',
     'ArcanistLintRenderer' => 'Phobject',
     'ArcanistLintResult' => 'Phobject',
@@ -730,6 +735,7 @@ phutil_register_library_map(array(
     'ArcanistPEP8Linter' => 'ArcanistExternalLinter',
     'ArcanistPEP8LinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistPHPCloseTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistPHPCloseTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPHPCompatibilityXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistPHPCompatibilityXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPHPEchoTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',

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(

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;

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.'));
 

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;
   }
@@ -288,4 +289,83 @@ 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.
+
+    // 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 <= $max_suffix; $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);
+
+      // 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
+      // 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);
+  }
+
 }

src/lint/__tests__/ArcanistLintMessageTestCase.php

@@ -0,0 +1,88 @@
+<?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,
+      ),
+      'mid-newline' => array(
+        'old' => 'ABA',
+        'new' => 'ABBA',
+        'old.expect' => '',
+        'new.expect' => 'B',
+        'line' => 1,
+        'char' => 3,
+      ),
+    );
+
+    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));
+    }
+  }
+}

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) {

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;
     }

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);
@@ -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,
       '');
   }

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/');
   }
 
 }

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.

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.

src/lint/linter/__tests__/xhpast/empty.lint-test

@@ -1,3 +1,4 @@
-<?php ?>
+<?php
+
 ~~~~~~~~~~
 warning::

src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php

@@ -58,10 +58,12 @@ final class ArcanistPhutilXHPASTLinterStandard
   public function getLinterSeverityMap() {
     $advice  = ArcanistLintSeverity::SEVERITY_ADVICE;
     $error   = ArcanistLintSeverity::SEVERITY_ERROR;
+    $warning = ArcanistLintSeverity::SEVERITY_WARNING;
 
     return array(
       ArcanistTodoCommentXHPASTLinterRule::ID         => $advice,
       ArcanistCommentSpacingXHPASTLinterRule::ID      => $error,
+      ArcanistRaggedClassTreeEdgeXHPASTLinterRule::ID => $warning,
     );
   }
 

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;
     }
 

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 == '[') {

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/');
+  }
+
+}

src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test

@@ -0,0 +1,6 @@
+<?php
+?>
+stuff.
+<?php
+?>
+~~~~~~~~~~

src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test

@@ -0,0 +1,5 @@
+<?php
+return;
+?>
+~~~~~~~~~~
+error:3:1

src/lint/renderer/ArcanistConsoleLintRenderer.php

@@ -6,20 +6,30 @@
 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();
+    $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 +67,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 +85,183 @@ 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);
+    $old_impact = substr_count($original, "\n") + 1;
+    $start = $line;
+
+    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.
+      $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 {
+        $old_line = idx($old_lines, $start - 1, null);
+        $new_line = idx($new_lines, $start - 1, null);
+
+        if ($old_line !== $new_line) {
+          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 = idx($old_lines, $start + $old_impact - 2, null);
+        $new_suffix = idx($new_lines, $start + $new_impact - 2, null);
+
+        if ($old_suffix !== $new_suffix) {
+          break;
+        }
+
+        $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);
+
+    } 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;
+    }
 
-    $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++;
+    for ($ii = $start; $ii < $start + $old_impact; $ii++) {
+      $out[] = array(
+        'text' => $old_lines[$ii - 1],
+        'number' => $ii,
+        'type' => '-',
+        'chevron' => ($ii == $start),
+      );
     }
 
-      $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));
+    for ($ii = $start; $ii < $start + $new_impact; $ii++) {
+      $out[] = array(
+        'text' => $new_lines[$ii - 1],
+        'type' => '+',
+        'chevron' => ($ii == $start),
+      );
     }
 
-        $out[] = $this->renderLine($cursor, $data, $chevron, $diff);
-      }
+    $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),
+      );
     }
 
-    // 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);
+    $result = array();
 
-      $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, '+');
-      }
-    }
-
-    $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()) {
+    $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());
-      }
-    }
-    $out[] = null;
+      $result[] = $this->renderLine(
+        idx($spec, 'number'),
+        $spec['text'],
+        $chevron,
+        idx($spec, 'type'));
 
-    return implode("\n", $out);
+      // 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);
   }
 
   private function renderCaret($pos) {
@@ -245,4 +285,28 @@ 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;
+  }
+
+  private function highlightText($text) {
+    if ($this->getTestableMode()) {
+      return '>'.$text.'<';
+    } else {
+      return (string)tsprintf('##%s##', $text);
+    }
+  }
+
 }

src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php

@@ -0,0 +1,200 @@
+<?php
+
+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;
+
+    $remline_original = <<<EOTEXT
+import apple;
+import banana;
+
+
+import cat;
+import dog;
+EOTEXT;
+
+    $remline_replacement = <<<EOTEXT
+import apple;
+import banana;
+
+import cat;
+import dog;
+EOTEXT;
+
+    $map = array(
+      'simple' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => 'a',
+        'replacement' => 'z',
+      ),
+      'inline' => array(
+        'line' => 1,
+        'char' => 7,
+        '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',
+      ),
+
+      '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",
+      ),
+
+      'caret' => array(
+        'line' => 2,
+        'char' => 13,
+        'name' => 'Fruit Misinformation',
+        'description' => 'Arguably untrue.',
+      ),
+
+      'original' => array(
+        'line' => 1,
+        'char' => 4,
+        'original' => 'should of',
+      ),
+
+      'midline' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => $midline_original,
+        'replacement' => $midline_replacement,
+      ),
+
+      'remline' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => $remline_original,
+        'replacement' => $remline_replacement,
+      ),
+
+      'extrawhitespace' => array(
+        'line' => 2,
+        'char' => 1,
+        'original' => "\n",
+        'replacement' => '',
+      ),
+    );
+
+    $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");
+      $data = preg_replace('/~+\s*$/m', '', $data);
+
+      $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 = id(new ArcanistConsoleLintRenderer())
+        ->setTestableMode(true);
+
+      try {
+        PhutilConsoleFormatter::disableANSI(true);
+        $actual = $renderer->renderLintResult($result);
+        PhutilConsoleFormatter::disableANSI(false);
+      } catch (Exception $ex) {
+        PhutilConsoleFormatter::disableANSI(false);
+        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,
+        pht(
+          'Lint rendering for "%s".',
+          $key));
+    }
+  }
+
+  private function readTestData($filename) {
+    $path = dirname(__FILE__).'/data/'.$filename;
+    return Filesystem::readFile($path);
+  }
+
+}

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

src/lint/renderer/__tests__/data/addline.txt

@@ -0,0 +1,5 @@
+apple
+banana
+date
+eclaire
+fig

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

src/lint/renderer/__tests__/data/addlinesuffix.txt

@@ -0,0 +1,5 @@
+apple
+banana
+date
+eclaire
+fig

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.

src/lint/renderer/__tests__/data/caret.txt

@@ -0,0 +1,4 @@
+Apples are round.
+Bananas are round.
+Cherries are round.
+Dates are round.

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 ~

src/lint/renderer/__tests__/data/extrawhitespace.txt

@@ -0,0 +1,3 @@
+Adrift upon the sea.
+
+~

src/lint/renderer/__tests__/data/inline.expect

@@ -0,0 +1,8 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 adjudi>cat<ed
+        +        adjudi>dog<ed

src/lint/renderer/__tests__/data/inline.txt

@@ -0,0 +1 @@
+adjudicated

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;

src/lint/renderer/__tests__/data/midline.txt

@@ -0,0 +1,4 @@
+import apple;
+import banana;
+import cat;
+import dog;

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

src/lint/renderer/__tests__/data/newline.txt

@@ -0,0 +1,11 @@
+aaa
+bbb
+ccc
+ddd
+eee
+
+fff
+ggg
+hhh
+iii
+jjj

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.

src/lint/renderer/__tests__/data/original.txt

@@ -0,0 +1 @@
+He should of known.

src/lint/renderer/__tests__/data/overlap.expect

@@ -0,0 +1,8 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 tanta>w<ount
+        +        tanta>m<ount

src/lint/renderer/__tests__/data/overlap.txt

@@ -0,0 +1 @@
+tantawount

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;

src/lint/renderer/__tests__/data/remline.txt

@@ -0,0 +1,6 @@
+import apple;
+import banana;
+
+
+import cat;
+import dog;

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

src/lint/renderer/__tests__/data/simple.txt

@@ -0,0 +1,3 @@
+a
+b
+c

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 />

src/lint/renderer/__tests__/data/xml.txt

@@ -0,0 +1,4 @@
+<
+  wow
+  xml>
+<xml />

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);
   }
@@ -544,8 +547,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;
     }
 
@@ -1422,7 +1427,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;

src/repository/api/ArcanistMercurialAPI.php

@@ -804,7 +804,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

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() {

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;
       }
 

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
     }

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";
     }
 

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,24 @@ final class ArcanistFileDataRef extends Phobject {
     return $data;
   }
 
+  private function newFileHash($path) {
+    $hash = hash_file('sha256', $path, $raw_output = false);
+
+    if ($hash === false) {
+      return null;
+    }
+
+    return $hash;
+  }
+
+  private function newDataHash($data) {
+    $hash = hash('sha256', $data, $raw_output = false);
+
+    if ($hash === false) {
+      return null;
+    }
+
+    return $hash;
+  }
+
 }

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?'));
@@ -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;

src/workflow/ArcanistDownloadWorkflow.php

@@ -79,6 +79,172 @@ EOTEXT
   public function run() {
     $conduit = $this->getConduit();
 
+    $id = $this->id;
+    $display_name = 'F'.$id;
+    $is_show = $this->show;
+    $save_as = $this->saveAs;
+    $path = null;
+
+    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->getStatusCode()));
+      }
+
+      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 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".
+    }
+
     $this->writeStatusMessage(pht('Getting file information...')."\n");
     $info = $conduit->callMethodSynchronous(
       'file.info',

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,16 +462,6 @@ EOTEXT
     $this->branch = head($branch);
     $this->keepBranch = $this->getArgument('keep-branch');
 
-    $update_strategy = $this->getConfigFromAnySource(
-      'arc.land.update.default',
-      'merge');
-    $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) {
@@ -888,7 +826,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
@@ -953,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));
@@ -1369,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)) {
@@ -1538,7 +1417,7 @@ EOTEXT
       pht('Harbormaster URI'),
       $buildable['uri']);
 
-    if (!$console->confirm($prompt)) {
+    if (!phutil_console_confirm($prompt)) {
       throw new ArcanistUserAbortException();
     }
   }

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);
@@ -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) {

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))) {
@@ -710,7 +722,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 +775,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(

src/workflow/ArcanistSetConfigWorkflow.php

@@ -69,6 +69,19 @@ EOTEXT
 
     $settings = new ArcanistSettings();
 
+    $console = PhutilConsole::getConsole();
+
+    if (!$settings->getHelp($key)) {
+      $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;
     if (array_key_exists($key, $config)) {
       $old = $config[$key];
@@ -85,17 +98,18 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        echo pht(
-          "Deleted key '%s' from %s config.\n",
+        $message = pht(
+          'Deleted key "%s" from %s config.',
           $key,
           $which);
       } else {
-        echo pht(
-          "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);
 
@@ -110,19 +124,20 @@ EOTEXT
       $old = $settings->formatConfigValueForDisplay($key, $old);
 
       if ($old === null) {
-        echo pht(
-          "Set key '%s' = %s in %s config.\n",
+        $message = pht(
+          'Set key "%s" = %s in %s config.',
           $key,
           $val,
           $which);
       } else {
-        echo pht(
-          "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;

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();
   }
 
 }

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();
   }
 
 }

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 {

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);
     }

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",

src/workflow/ArcanistWorkflow.php

@@ -338,7 +338,7 @@ abstract class ArcanistWorkflow extends Phobject {
 
           $this->conduitAuthenticated = true;
 
-          return;
+          return $this;
         } catch (Exception $ex) {
           $conduit->setConduitToken(null);
           throw $ex;
@@ -1011,9 +1011,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`).',
@@ -1199,6 +1197,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);