Move Git-specific "arc land" parsing of "--onto" and "--remote" into GitLandEngine

Summary: Ref T13434. Move some git-engine-specific handling of "arc land" arguments into the Git engine. This prepares to handle Perforce workflows.

Test Plan: Will "arc land" this change.

Maniphest Tasks: T13434

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

src/land/ArcanistGitLandEngine.php

@@ -9,6 +9,14 @@ final class ArcanistGitLandEngine
   private $mergedRef;
   private $restoreWhenDestroyed;
 
+  public function parseArguments() {
+    $onto = $this->getEngineOnto();
+    $this->setTargetOnto($onto);
+
+    $remote = $this->getEngineRemote();
+    $this->setTargetRemote($remote);
+  }
+
   public function execute() {
     $this->verifySourceAndTargetExist();
     $this->fetchTarget();
@@ -615,4 +623,114 @@ final class ArcanistGitLandEngine
     return !$err;
   }
 
+  private function getEngineOnto() {
+    $source_ref = $this->getSourceRef();
+
+    $onto = $this->getOntoArgument();
+    if ($onto !== null) {
+      $this->writeInfo(
+        pht('TARGET'),
+        pht(
+          'Landing onto "%s", selected with the "--onto" flag.',
+          $onto));
+      return $onto;
+    }
+
+    $api = $this->getRepositoryAPI();
+    $path = $api->getPathToUpstream($source_ref);
+
+    if ($path->getLength()) {
+      $cycle = $path->getCycle();
+      if ($cycle) {
+        $this->writeWarn(
+          pht('LOCAL CYCLE'),
+          pht(
+            'Local branch tracks an upstream, but following it leads to a '.
+            'local cycle; ignoring branch upstream.'));
+
+        echo tsprintf(
+          "\n    %s\n\n",
+          implode(' -> ', $cycle));
+
+      } else {
+        if ($path->isConnectedToRemote()) {
+          $onto = $path->getRemoteBranchName();
+          $this->writeInfo(
+            pht('TARGET'),
+            pht(
+              'Landing onto "%s", selected by following tracking branches '.
+              'upstream to the closest remote.',
+              $onto));
+          return $onto;
+        } else {
+          $this->writeInfo(
+            pht('NO PATH TO UPSTREAM'),
+            pht(
+              'Local branch tracks an upstream, but there is no path '.
+              'to a remote; ignoring branch upstream.'));
+        }
+      }
+    }
+
+    $workflow = $this->getWorkflow();
+
+    $config_key = 'arc.land.onto.default';
+    $onto = $workflow->getConfigFromAnySource($config_key);
+    if ($onto !== null) {
+      $this->writeInfo(
+        pht('TARGET'),
+        pht(
+          'Landing onto "%s", selected by "%s" configuration.',
+          $onto,
+          $config_key));
+      return $onto;
+    }
+
+    $onto = 'master';
+    $this->writeInfo(
+      pht('TARGET'),
+      pht(
+        'Landing onto "%s", the default target under git.',
+        $onto));
+
+    return $onto;
+  }
+
+  private function getEngineRemote() {
+    $source_ref = $this->getSourceRef();
+
+    $remote = $this->getRemoteArgument();
+    if ($remote !== null) {
+      $this->writeInfo(
+        pht('REMOTE'),
+        pht(
+          'Using remote "%s", selected with the "--remote" flag.',
+          $remote));
+      return $remote;
+    }
+
+    $api = $this->getRepositoryAPI();
+    $path = $api->getPathToUpstream($source_ref);
+
+    $remote = $path->getRemoteRemoteName();
+    if ($remote !== null) {
+      $this->writeInfo(
+        pht('REMOTE'),
+        pht(
+          'Using remote "%s", selected by following tracking branches '.
+          'upstream to the closest remote.',
+          $remote));
+      return $remote;
+    }
+
+    $remote = 'origin';
+    $this->writeInfo(
+      pht('REMOTE'),
+      pht(
+        'Using remote "%s", the default remote under git.',
+        $remote));
+
+    return $remote;
+  }
+
 }

src/land/ArcanistLandEngine.php

@@ -13,6 +13,8 @@ abstract class ArcanistLandEngine extends Phobject {
   private $shouldSquash;
   private $shouldDeleteRemote;
   private $shouldPreview;
+  private $remoteArgument;
+  private $ontoArgument;
 
   // TODO: This is really grotesque.
   private $buildMessageCallback;
@@ -117,6 +119,25 @@ abstract class ArcanistLandEngine extends Phobject {
     return $this->commitMessageFile;
   }
 
+  final public function setRemoteArgument($remote_argument) {
+    $this->remoteArgument = $remote_argument;
+    return $this;
+  }
+
+  final public function getRemoteArgument() {
+    return $this->remoteArgument;
+  }
+
+  final public function setOntoArgument($onto_argument) {
+    $this->ontoArgument = $onto_argument;
+    return $this;
+  }
+
+  final public function getOntoArgument() {
+    return $this->ontoArgument;
+  }
+
+  abstract public function parseArguments();
   abstract public function execute();
 
   abstract protected function getLandingCommits();

src/workflow/ArcanistLandWorkflow.php

@@ -224,23 +224,28 @@ EOTEXT
     }
 
     if ($engine) {
-      $this->readEngineArguments();
-      $this->requireCleanWorkingCopy();
-
       $should_hold = $this->getArgument('hold');
+      $remote_arg = $this->getArgument('remote');
+      $onto_arg = $this->getArgument('onto');
 
       $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)
+        ->setRemoteArgument($remote_arg)
+        ->setOntoArgument($onto_arg)
         ->setBuildMessageCallback(array($this, 'buildEngineMessage'));
 
+      // The goal here is to raise errors with flags early (which is cheap),
+      // before we test if the working copy is clean (which can be slow). This
+      // could probably be structured more cleanly.
+
+      $engine->parseArguments();
+      $this->requireCleanWorkingCopy();
       $engine->execute();
 
       if (!$should_hold && !$this->preview) {
@@ -337,123 +342,6 @@ EOTEXT
     return $refspec;
   }
 
-  private function readEngineArguments() {
-    // NOTE: This is hard-coded for Git right now.
-    // TODO: Clean this up and move it into LandEngines.
-
-    $onto = $this->getEngineOnto();
-    $remote = $this->getEngineRemote();
-
-    // This just overwrites work we did earlier, but it has to be up in this
-    // class for now because other parts of the workflow still depend on it.
-    $this->onto = $onto;
-    $this->remote = $remote;
-    $this->ontoRemoteBranch = $this->remote.'/'.$onto;
-  }
-
-  private function getEngineOnto() {
-    $onto = $this->getArgument('onto');
-    if ($onto !== null) {
-      $this->writeInfo(
-        pht('TARGET'),
-        pht(
-          'Landing onto "%s", selected by the --onto flag.',
-          $onto));
-      return $onto;
-    }
-
-    $api = $this->getRepositoryAPI();
-    $path = $api->getPathToUpstream($this->branch);
-
-    if ($path->getLength()) {
-      $cycle = $path->getCycle();
-      if ($cycle) {
-        $this->writeWarn(
-          pht('LOCAL CYCLE'),
-          pht(
-            'Local branch tracks an upstream, but following it leads to a '.
-            'local cycle; ignoring branch upstream.'));
-
-        echo tsprintf(
-          "\n    %s\n\n",
-          implode(' -> ', $cycle));
-
-      } else {
-        if ($path->isConnectedToRemote()) {
-          $onto = $path->getRemoteBranchName();
-          $this->writeInfo(
-            pht('TARGET'),
-            pht(
-              'Landing onto "%s", selected by following tracking branches '.
-              'upstream to the closest remote.',
-              $onto));
-          return $onto;
-        } else {
-          $this->writeInfo(
-            pht('NO PATH TO UPSTREAM'),
-            pht(
-              'Local branch tracks an upstream, but there is no path '.
-              'to a remote; ignoring branch upstream.'));
-        }
-      }
-    }
-
-    $config_key = 'arc.land.onto.default';
-    $onto = $this->getConfigFromAnySource($config_key);
-    if ($onto !== null) {
-      $this->writeInfo(
-        pht('TARGET'),
-        pht(
-          'Landing onto "%s", selected by "%s" configuration.',
-          $onto,
-          $config_key));
-      return $onto;
-    }
-
-    $onto = 'master';
-    $this->writeInfo(
-      pht('TARGET'),
-      pht(
-        'Landing onto "%s", the default target under git.',
-        $onto));
-    return $onto;
-  }
-
-  private function getEngineRemote() {
-    $remote = $this->getArgument('remote');
-    if ($remote !== null) {
-      $this->writeInfo(
-        pht('REMOTE'),
-        pht(
-          'Using remote "%s", selected by the --remote flag.',
-          $remote));
-      return $remote;
-    }
-
-    $api = $this->getRepositoryAPI();
-    $path = $api->getPathToUpstream($this->branch);
-
-    $remote = $path->getRemoteRemoteName();
-    if ($remote !== null) {
-      $this->writeInfo(
-        pht('REMOTE'),
-        pht(
-          'Using remote "%s", selected by following tracking branches '.
-          'upstream to the closest remote.',
-          $remote));
-      return $remote;
-    }
-
-    $remote = 'origin';
-    $this->writeInfo(
-      pht('REMOTE'),
-      pht(
-        'Using remote "%s", the default remote under git.',
-        $remote));
-    return $remote;
-  }
-
-
   private function readArguments() {
     $repository_api = $this->getRepositoryAPI();
     $this->isGit = $repository_api instanceof ArcanistGitAPI;