Make "arc land" great again

Summary:
Ref T3855. Fixes T9537. Fixes T8620. Fixes T4333.

This declares bankruptcy and replaces the entire `arc land` workflow under Git. These are the notable changes:

  - (T3855) You can now land from a branch to itself.
  - (T3855) We now try to restore the original state very aggressively after any failure, instead of dumping you into the middle of a mess.
  - (T9537) You can now land without a local branch.
  - ([not actually] T9543) We'll now ignore the local branch if it just happens to be named the same thing as the remote branch but doesn't actually track it.
  - (T8620) You can now land from a detached HEAD.
  - (T4333) We now preserve the author and author date of whatever you land.

This may need some followup work. In particular:

  - The signal handler (that tries to put you in a better place if you ^C in the middle of things) causes ^C to work awkwardly in prompts. This might not be worth it.
  - Errors/instructions on push/merge issues might need work.
  - I dropped support for `--delete-remote` and `--update-with-blah-blah` because I think these flags aren't worth their complexity.
  - I've simplified the update/merge algorithm a bit. It may need some complexity added back in.
  - I probably missed a few things because this covers like 200 unique, creative workflows.
  - Users might need more guidance on the workflows that drop them in the middle of nowhere if they manage to reach them more often than I think.

Test Plan:
  - Used `arc land` to land like at least 15,000 different kinds of changes.
  - Landed normally.
  - Landed from a branch onto itself.
  - Landed from a detached head.
  - Landed nothing.
  - Landed with no local branch.
  - Landed onto made-up branches.
  - Landed with bad targets.
  - ^C'd things in the middle.

Reviewers: chad

Reviewed By: chad

Subscribers: tycho.tatitscheff

Maniphest Tasks: T3855, T4333, T8620, T9537, T9543

Differential Revision: https://secure.phabricator.com/D14356

src/__phutil_library_map__.php

@@ -112,6 +112,7 @@ phutil_register_library_map(array(
     'ArcanistGeneratedLinterTestCase' => 'lint/linter/__tests__/ArcanistGeneratedLinterTestCase.php',
     'ArcanistGetConfigWorkflow' => 'workflow/ArcanistGetConfigWorkflow.php',
     'ArcanistGitAPI' => 'repository/api/ArcanistGitAPI.php',
+    'ArcanistGitLandEngine' => 'land/ArcanistGitLandEngine.php',
     'ArcanistGlobalVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php',
     'ArcanistGoLintLinter' => 'lint/linter/ArcanistGoLintLinter.php',
     'ArcanistGoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistGoLintLinterTestCase.php',
@@ -144,6 +145,7 @@ phutil_register_library_map(array(
     'ArcanistJscsLinterTestCase' => 'lint/linter/__tests__/ArcanistJscsLinterTestCase.php',
     'ArcanistKeywordCasingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php',
     'ArcanistLambdaFuncFunctionXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistLambdaFuncFunctionXHPASTLinterRule.php',
+    'ArcanistLandEngine' => 'land/ArcanistLandEngine.php',
     'ArcanistLandWorkflow' => 'workflow/ArcanistLandWorkflow.php',
     'ArcanistLanguageConstructParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistLanguageConstructParenthesesXHPASTLinterRule.php',
     'ArcanistLesscLinter' => 'lint/linter/ArcanistLesscLinter.php',
@@ -398,6 +400,7 @@ phutil_register_library_map(array(
     'ArcanistGeneratedLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistGetConfigWorkflow' => 'ArcanistWorkflow',
     'ArcanistGitAPI' => 'ArcanistRepositoryAPI',
+    'ArcanistGitLandEngine' => 'ArcanistLandEngine',
     'ArcanistGlobalVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistGoLintLinter' => 'ArcanistExternalLinter',
     'ArcanistGoLintLinterTestCase' => 'ArcanistExternalLinterTestCase',
@@ -430,6 +433,7 @@ phutil_register_library_map(array(
     'ArcanistJscsLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistKeywordCasingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistLambdaFuncFunctionXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistLandEngine' => 'Phobject',
     'ArcanistLandWorkflow' => 'ArcanistWorkflow',
     'ArcanistLanguageConstructParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistLesscLinter' => 'ArcanistExternalLinter',

src/internationalization/ArcanistUSEnglishTranslation.php

@@ -78,6 +78,11 @@ final class ArcanistUSEnglishTranslation extends PhutilTranslation {
         'Ignore the changes to this submodule and continue?',
         'Ignore the changes to these submodules and continue?',
       ),
+
+      'These %s commit(s) will be landed:' => array(
+        'This commit will be landed:',
+        'These commits will be landed:',
+      ),
     );
   }
 

src/land/ArcanistGitLandEngine.php

@@ -0,0 +1,387 @@
+<?php
+
+final class ArcanistGitLandEngine
+  extends ArcanistLandEngine {
+
+  private $localRef;
+  private $localCommit;
+  private $sourceCommit;
+  private $mergedRef;
+  private $restoreWhenDestroyed;
+
+  public function execute() {
+    $this->verifySourceAndTargetExist();
+    $this->fetchTarget();
+
+    $this->printLandingCommits();
+
+    if ($this->getShouldPreview()) {
+      $this->writeInfo(
+        pht('PREVIEW'),
+        pht('Completed preview of operation.'));
+      return;
+    }
+
+    $this->saveLocalState();
+
+    try {
+      $this->identifyRevision();
+      $this->updateWorkingCopy();
+
+      if ($this->getShouldHold()) {
+        $this->writeInfo(
+          pht('HOLD'),
+          pht('Holding change locally, it has not been pushed.'));
+      } else {
+        $this->pushChange();
+        $this->reconcileLocalState();
+
+        if ($this->getShouldKeep()) {
+          echo tsprintf(
+            "%s\n",
+            pht('Keeping local branch.'));
+        } else {
+          $this->destroyLocalBranch();
+        }
+
+        $this->writeOkay(
+          pht('DONE'),
+          pht('Landed changes.'));
+      }
+
+      $this->restoreWhenDestroyed = false;
+    } catch (Exception $ex) {
+      $this->restoreLocalState();
+      throw $ex;
+    }
+  }
+
+  public function __destruct() {
+    if ($this->restoreWhenDestroyed) {
+      $this->writeWARN(
+        pht('INTERRUPTED!'),
+        pht('Restoring working copy to its original state.'));
+
+      $this->restoreLocalState();
+    }
+  }
+
+  protected function getLandingCommits() {
+    $api = $this->getRepositoryAPI();
+
+    list($out) = $api->execxLocal(
+      'log --oneline %s..%s --',
+      $this->getTargetFullRef(),
+      $this->sourceCommit);
+
+    $out = trim($out);
+
+    if (!strlen($out)) {
+      return array();
+    } else {
+      return phutil_split_lines($out, false);
+    }
+  }
+
+  private function identifyRevision() {
+    $api = $this->getRepositoryAPI();
+    $api->execxLocal('checkout %s --', $this->getSourceRef());
+    call_user_func($this->getBuildMessageCallback(), $this);
+  }
+
+  private function verifySourceAndTargetExist() {
+    $api = $this->getRepositoryAPI();
+
+    list($err) = $api->execManualLocal(
+      'rev-parse --verify %s',
+      $this->getTargetFullRef());
+
+    if ($err) {
+      throw new Exception(
+        pht(
+          'Branch "%s" does not exist in remote "%s".',
+          $this->getTargetOnto(),
+          $this->getTargetRemote()));
+    }
+
+    list($err, $stdout) = $api->execManualLocal(
+      'rev-parse --verify %s',
+      $this->getSourceRef());
+
+    if ($err) {
+      throw new Exception(
+        pht(
+          'Branch "%s" does not exist in the local working copy.',
+          $this->getSourceRef()));
+    }
+
+    $this->sourceCommit = trim($stdout);
+  }
+
+  private function fetchTarget() {
+    $api = $this->getRepositoryAPI();
+
+    $ref = $this->getTargetFullRef();
+
+    $this->writeInfo(
+      pht('FETCH'),
+      pht('Fetching %s...', $ref));
+
+    $api->execxLocal(
+      'fetch -- %s %s',
+      $this->getTargetRemote(),
+      $this->getTargetOnto());
+  }
+
+  private function updateWorkingCopy() {
+    $api = $this->getRepositoryAPI();
+    $source = $this->sourceCommit;
+
+    $api->execxLocal(
+      'checkout %s --',
+      $this->getTargetFullRef());
+
+    list($original_author, $original_date) = $this->getAuthorAndDate($source);
+
+    try {
+      if ($this->getShouldSquash()) {
+        $api->execxLocal(
+          'merge --no-stat --no-commit --squash -- %s',
+          $source);
+      } else {
+        $api->execxLocal(
+          'merge --no-stat --no-commit --no-ff -- %s',
+          $source);
+      }
+    } catch (Exception $ex) {
+      $api->execManualLocal('merge --abort');
+
+      // TODO: Maybe throw a better or more helpful exception here?
+
+      throw $ex;
+    }
+
+    $api->execxLocal(
+      'commit --author %s --date %s -F %s --',
+      $original_author,
+      $original_date,
+      $this->getCommitMessageFile());
+
+    list($stdout) = $api->execxLocal(
+      'rev-parse --verify %s',
+      'HEAD');
+    $this->mergedRef = trim($stdout);
+  }
+
+  private function pushChange() {
+    $api = $this->getRepositoryAPI();
+
+    $this->writeInfo(
+      pht('PUSHING'),
+      pht('Pushing changes to "%s".', $this->getTargetFullRef()));
+
+    list($err) = $api->execPassthru(
+      'push -- %s %s:%s',
+      $this->getTargetRemote(),
+      $this->mergedRef,
+      $this->getTargetOnto());
+
+    if ($err) {
+      throw new ArcanistUsageException(
+        pht(
+          'Push failed! Fix the error and run "%s" again.',
+          'arc land'));
+    }
+  }
+
+  private function reconcileLocalState() {
+    $api = $this->getRepositoryAPI();
+
+    // Try to put the user into the best final state we can. This is very
+    // complicated because users are incredibly creative and their local
+    // branches may have the same names as branches in the remote but no
+    // relationship to them.
+
+    if ($this->localRef != $this->getSourceRef()) {
+      // The user ran `arc land X` but was on a different branch, so just put
+      // them back wherever they were before.
+      echo tsprintf(
+        "%s\n",
+        pht('Switching back to "%s".', $this->localRef));
+      $this->restoreLocalState();
+      return;
+    }
+
+    list($err) = $api->execManualLocal(
+      'rev-parse --verify %s',
+      $this->getTargetOnto());
+    if ($err) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Local branch "%s" does not exist, staying on detached HEAD.',
+          $this->getTargetOnto()));
+      return;
+    }
+
+    list($err, $upstream) = $api->execManualLocal(
+      'rev-parse --verify --symbolic-full-name %s',
+      $this->getTargetOnto().'@{upstream}');
+    if ($err) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Local branch "%s" has no upstream, staying on detached HEAD.',
+          $this->getTargetOnto()));
+      return;
+    }
+
+    $upstream = trim($upstream);
+    $expect_upstream = 'refs/remotes/'.$this->getTargetFullRef();
+    if ($upstream != $expect_upstream) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Local branch "%s" tracks remote "%s" (not target remote "%s"), '.
+          'staying on detached HEAD.',
+          $this->getTargetOnto(),
+          $upstream,
+          $expect_upstream));
+      return;
+    }
+
+    list($stdout) = $api->execxLocal(
+      'log %s..%s --',
+      $this->mergedRef,
+      $this->getTargetOnto());
+    $stdout = trim($stdout);
+
+    if (!strlen($stdout)) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Local "%s" tracks target remote "%s", checking out and '.
+          'pulling changes.',
+          $this->getTargetOnto(),
+          $this->getTargetFullRef()));
+
+      $api->execxLocal('checkout %s --', $this->getTargetOnto());
+      $api->execxLocal('pull --');
+      $api->execxLocal('submodule update --init --recursive');
+
+      return;
+    }
+
+    if ($this->getTargetOnto() !== $this->getSourceRef()) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Local "%s" is ahead of remote "%s". Checking out but '.
+          'not pulling changes.',
+          $this->getTargetOnto(),
+          $this->getTargetFullRef()));
+
+      $api->execxLocal('checkout %s --', $this->getTargetOnto());
+      $api->execxLocal('submodule update --init --recursive');
+
+      return;
+    }
+
+    // In this case, the user did something like land a branch onto itself,
+    // and the branch is tracking the correct remote. We're going to discard
+    // the local state and reset it to the state we just pushed.
+
+    echo tsprintf(
+      "%s\n",
+      pht(
+        'Local "%s" landed into remote "%s", resetting local branch to '.
+        'remote state.',
+        $this->getTargetOnto(),
+        $this->getTargetFullRef()));
+
+    $api->execxLocal('checkout %s --', $this->getTargetOnto());
+    $api->execxLocal('reset --hard %s --', $this->getTargetFullRef());
+    $api->execxLocal('submodule update --init --recursive');
+  }
+
+  private function destroyLocalBranch() {
+    $api = $this->getRepositoryAPI();
+
+    if ($this->localRef == $this->getSourceRef()) {
+      // If we landed a branch onto itself, don't destroy it.
+      return;
+    }
+
+    $recovery_command = csprintf(
+      'git checkout -b %R %R',
+      $this->getSourceRef(),
+      $this->sourceCommit);
+
+    echo tsprintf(
+      "%s\n",
+      pht('Cleaning up branch "%s"...', $this->getSourceRef()));
+
+    echo tsprintf(
+      "%s\n",
+      pht('(Use `%s` if you want it back.)', $recovery_command));
+
+    $api->execxLocal('branch -D -- %s', $this->getSourceRef());
+  }
+
+  /**
+   * Save the local working copy state so we can restore it later.
+   */
+  private function saveLocalState() {
+    $api = $this->getRepositoryAPI();
+
+    $this->localCommit = $api->getWorkingCopyRevision();
+
+    list($ref) = $api->execxLocal('rev-parse --abbrev-ref HEAD');
+    $ref = trim($ref);
+    if ($ref === 'HEAD') {
+      $ref = $this->localCommit;
+    }
+
+    $this->localRef = $ref;
+
+    $this->restoreWhenDestroyed = true;
+  }
+
+  /**
+   * Restore the working copy to the state it was in before we started
+   * performing writes.
+   */
+  private function restoreLocalState() {
+    $api = $this->getRepositoryAPI();
+
+    $api->execxLocal('checkout %s --', $this->localRef);
+    $api->execxLocal('reset --hard %s --', $this->localCommit);
+    $api->execxLocal('submodule update --init --recursive');
+
+    $this->restoreWhenDestroyed = false;
+  }
+
+  private function getTargetFullRef() {
+    return $this->getTargetRemote().'/'.$this->getTargetOnto();
+  }
+
+  private function getAuthorAndDate($commit) {
+    $api = $this->getRepositoryAPI();
+
+    // TODO: This is working around Windows escaping problems, see T8298.
+
+    list($info) = $api->execxLocal(
+      'log -n1 --format=%C %s --',
+      '%aD%n%an%n%ae',
+      $commit);
+
+    $info = trim($info);
+    list($date, $author, $email) = explode("\n", $info, 3);
+
+    return array(
+      "$author <{$email}>",
+      $date,
+    );
+  }
+
+}

src/land/ArcanistLandEngine.php

@@ -0,0 +1,161 @@
+<?php
+
+abstract class ArcanistLandEngine extends Phobject {
+
+  private $workflow;
+  private $repositoryAPI;
+  private $targetRemote;
+  private $targetOnto;
+  private $sourceRef;
+  private $commitMessageFile;
+  private $shouldHold;
+  private $shouldKeep;
+  private $shouldSquash;
+  private $shouldDeleteRemote;
+  private $shouldPreview;
+
+  // TODO: This is really grotesque.
+  private $buildMessageCallback;
+
+  final public function setWorkflow(ArcanistWorkflow $workflow) {
+    $this->workflow = $workflow;
+    return $this;
+  }
+
+  final public function getWorkflow() {
+    return $this->workflow;
+  }
+
+  final public function setRepositoryAPI(
+    ArcanistRepositoryAPI $repository_api) {
+    $this->repositoryAPI = $repository_api;
+    return $this;
+  }
+
+  final public function getRepositoryAPI() {
+    return $this->repositoryAPI;
+  }
+
+  final public function setShouldHold($should_hold) {
+    $this->shouldHold = $should_hold;
+    return $this;
+  }
+
+  final public function getShouldHold() {
+    return $this->shouldHold;
+  }
+
+  final public function setShouldKeep($should_keep) {
+    $this->shouldKeep = $should_keep;
+    return $this;
+  }
+
+  final public function getShouldKeep() {
+    return $this->shouldKeep;
+  }
+
+  final public function setShouldSquash($should_squash) {
+    $this->shouldSquash = $should_squash;
+    return $this;
+  }
+
+  final public function getShouldSquash() {
+    return $this->shouldSquash;
+  }
+
+  final public function setShouldPreview($should_preview) {
+    $this->shouldPreview = $should_preview;
+    return $this;
+  }
+
+  final public function getShouldPreview() {
+    return $this->shouldPreview;
+  }
+
+  final public function setTargetRemote($target_remote) {
+    $this->targetRemote = $target_remote;
+    return $this;
+  }
+
+  final public function getTargetRemote() {
+    return $this->targetRemote;
+  }
+
+  final public function setTargetOnto($target_onto) {
+    $this->targetOnto = $target_onto;
+    return $this;
+  }
+
+  final public function getTargetOnto() {
+    return $this->targetOnto;
+  }
+
+  final public function setSourceRef($source_ref) {
+    $this->sourceRef = $source_ref;
+    return $this;
+  }
+
+  final public function getSourceRef() {
+    return $this->sourceRef;
+  }
+
+  final public function setBuildMessageCallback($build_message_callback) {
+    $this->buildMessageCallback = $build_message_callback;
+    return $this;
+  }
+
+  final public function getBuildMessageCallback() {
+    return $this->buildMessageCallback;
+  }
+
+  final public function setCommitMessageFile($commit_message_file) {
+    $this->commitMessageFile = $commit_message_file;
+    return $this;
+  }
+
+  final public function getCommitMessageFile() {
+    return $this->commitMessageFile;
+  }
+
+  abstract public function execute();
+
+  abstract protected function getLandingCommits();
+
+  protected function printLandingCommits() {
+    $logs = $this->getLandingCommits();
+
+    if (!$logs) {
+      throw new ArcanistUsageException(
+        pht(
+          'There are no commits on "%s" which are not already present on '.
+          'the target.',
+          $this->getSourceRef()));
+    }
+
+    $list = id(new PhutilConsoleList())
+      ->setWrap(false)
+      ->addItems($logs);
+
+    id(new PhutilConsoleBlock())
+      ->addParagraph(
+        pht(
+          'These %s commit(s) will be landed:',
+          new PhutilNumber(count($logs))))
+      ->addList($list)
+      ->draw();
+  }
+
+  protected function writeWarn($title, $message) {
+    return $this->getWorkflow()->writeWarn($title, $message);
+  }
+
+  protected function writeInfo($title, $message) {
+    return $this->getWorkflow()->writeInfo($title, $message);
+  }
+
+  protected function writeOkay($title, $message) {
+    return $this->getWorkflow()->writeOkay($title, $message);
+  }
+
+
+}

src/workflow/ArcanistLandWorkflow.php

@@ -196,6 +196,53 @@ EOTEXT
 
   public function run() {
     $this->readArguments();
+
+    $engine = null;
+    if ($this->isGit && !$this->isGitSvn) {
+      $engine = new ArcanistGitLandEngine();
+    }
+
+    if ($engine) {
+      $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');
+
+      $engine
+        ->setWorkflow($this)
+        ->setRepositoryAPI($this->getRepositoryAPI())
+        ->setSourceRef($this->branch)
+        ->setTargetRemote($this->remote)
+        ->setTargetOnto($this->onto)
+        ->setShouldHold($should_hold)
+        ->setShouldKeep($this->keepBranch)
+        ->setShouldSquash($this->useSquash)
+        ->setShouldPreview($this->preview)
+        ->setBuildMessageCallback(array($this, 'buildEngineMessage'));
+
+      $engine->execute();
+
+      if (!$should_hold) {
+        $this->didPush();
+      }
+
+      return 0;
+    }
+
     $this->validate();
 
     try {
@@ -1085,16 +1132,7 @@ EOTEXT
           $cmd));
       }
 
-      $this->askForRepositoryUpdate();
+      $this->didPush();
-
-      $mark_workflow = $this->buildChildWorkflow(
-        'close-revision',
-        array(
-          '--finalize',
-          '--quiet',
-          $this->revision['id'],
-        ));
-      $mark_workflow->run();
 
       echo "\n";
     }
@@ -1193,6 +1231,11 @@ EOTEXT
     $repository_api = $this->getRepositoryAPI();
     if ($this->isGit) {
       $branch = $repository_api->getBranchName();
+
+      // If we don't have a branch name, just use whatever's at HEAD.
+      if (!strlen($branch) && !$this->isGitSvn) {
+        $branch = $repository_api->getWorkingCopyRevision();
+      }
     } else if ($this->isHg) {
       $branch = $repository_api->getActiveBookmark();
       if (!$branch) {
@@ -1317,4 +1360,23 @@ EOTEXT
     }
   }
 
+  public function buildEngineMessage(ArcanistLandEngine $engine) {
+    // TODO: This is oh-so-gross.
+    $this->findRevision();
+    $engine->setCommitMessageFile($this->messageFile);
+  }
+
+  public function didPush() {
+    $this->askForRepositoryUpdate();
+
+    $mark_workflow = $this->buildChildWorkflow(
+      'close-revision',
+      array(
+        '--finalize',
+        '--quiet',
+        $this->revision['id'],
+      ));
+    $mark_workflow->run();
+  }
+
 }

src/workflow/ArcanistWorkflow.php

@@ -1361,7 +1361,7 @@ abstract class ArcanistWorkflow extends Phobject {
     fwrite(STDERR, $msg);
   }
 
-  final protected function writeInfo($title, $message) {
+  final public function writeInfo($title, $message) {
     $this->writeStatusMessage(
       phutil_console_format(
         "<bg:blue>** %s **</bg> %s\n",
@@ -1369,7 +1369,7 @@ abstract class ArcanistWorkflow extends Phobject {
         $message));
   }
 
-  final protected function writeWarn($title, $message) {
+  final public function writeWarn($title, $message) {
     $this->writeStatusMessage(
       phutil_console_format(
         "<bg:yellow>** %s **</bg> %s\n",
@@ -1377,7 +1377,7 @@ abstract class ArcanistWorkflow extends Phobject {
         $message));
   }
 
-  final protected function writeOkay($title, $message) {
+  final public function writeOkay($title, $message) {
     $this->writeStatusMessage(
       phutil_console_format(
         "<bg:green>** %s **</bg> %s\n",