Support Perforce/Git repositories in "arc land"

Summary: Ref T13434. Detect perforce remotes and use "git p4" commands in place of "git" commands when operating in Perforce mode.

Test Plan:
  - Landed "master" onto itself, saw master update.
  - Landed "feature1" onto clean "master", saw master update.
  - Landed "feature2" onto dirty "master", saw master stay dirty.
  - Landed with "--hold", got sensible submit instructions.

Maniphest Tasks: T13434

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

src/land/ArcanistGitLandEngine.php

@@ -8,12 +8,45 @@ final class ArcanistGitLandEngine
   private $sourceCommit;
   private $mergedRef;
   private $restoreWhenDestroyed;
+  private $isGitPerforce;
+
+  private function setIsGitPerforce($is_git_perforce) {
+    $this->isGitPerforce = $is_git_perforce;
+    return $this;
+  }
+
+  private function getIsGitPerforce() {
+    return $this->isGitPerforce;
+  }
 
   public function parseArguments() {
+    $api = $this->getRepositoryAPI();
+
     $onto = $this->getEngineOnto();
     $this->setTargetOnto($onto);
 
     $remote = $this->getEngineRemote();
+
+    $is_pushable = $api->isPushableRemote($remote);
+    $is_perforce = $api->isPerforceRemote($remote);
+
+    if (!$is_pushable && !$is_perforce) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'No pushable remote "%s" exists. Use the "--remote" flag to choose '.
+          'a valid, pushable remote to land changes onto.',
+          $remote));
+    }
+
+    if ($is_perforce) {
+      $this->setIsGitPerforce(true);
+      $this->writeWarn(
+        pht('P4 MODE'),
+        pht(
+          'Operating in Git/Perforce mode after selecting a Perforce '.
+          'remote.'));
+    }
+
     $this->setTargetRemote($remote);
   }
 
@@ -132,25 +165,45 @@ final class ArcanistGitLandEngine
 
     $ref = $this->getTargetFullRef();
 
-    $this->writeInfo(
-      pht('FETCH'),
-      pht('Fetching %s...', $ref));
-
     // NOTE: Although this output isn't hugely useful, we need to passthru
     // instead of using a subprocess here because `git fetch` may prompt the
     // user to enter a password if they're fetching over HTTP with basic
     // authentication. See T10314.
 
-    $err = $api->execPassthru(
+    if ($this->getIsGitPerforce()) {
-      'fetch --quiet -- %s %s',
+      $this->writeInfo(
-      $this->getTargetRemote(),
+        pht('P4 SYNC'),
-      $this->getTargetOnto());
+        pht('Synchronizing "%s" from Perforce...', $ref));
 
-    if ($err) {
+      $sync_ref = sprintf(
-      throw new ArcanistUsageException(
+        'refs/remotes/%s/%s',
-        pht(
+        $this->getTargetRemote(),
-          'Fetch failed! Fix the error and run "%s" again.',
+        $this->getTargetOnto());
-          'arc land'));
+
+      $err = $api->execPassthru(
+        'p4 sync --silent --branch %R --',
+        $sync_ref);
+
+      if ($err) {
+        throw new ArcanistUsageException(
+          pht(
+            'Perforce sync failed! Fix the error and run "arc land" again.'));
+      }
+    } else {
+      $this->writeInfo(
+        pht('FETCH'),
+        pht('Fetching "%s"...', $ref));
+
+      $err = $api->execPassthru(
+        'fetch --quiet -- %s %s',
+        $this->getTargetRemote(),
+        $this->getTargetOnto());
+
+      if ($err) {
+        throw new ArcanistUsageException(
+          pht(
+            'Fetch failed! Fix the error and run "arc land" again.'));
+      }
     }
   }
 
@@ -220,21 +273,68 @@ final class ArcanistGitLandEngine
   private function pushChange() {
     $api = $this->getRepositoryAPI();
 
-    $this->writeInfo(
+    if ($this->getIsGitPerforce()) {
-      pht('PUSHING'),
+      $this->writeInfo(
-      pht('Pushing changes to "%s".', $this->getTargetFullRef()));
+        pht('SUBMITTING'),
+        pht('Submitting changes to "%s".', $this->getTargetFullRef()));
 
-    $err = $api->execPassthru(
+      $config_argv = array();
-      'push -- %s %s:%s',
-      $this->getTargetRemote(),
-      $this->mergedRef,
-      $this->getTargetOnto());
 
-    if ($err) {
+      // Skip the "git p4 submit" interactive editor workflow. We expect
-      throw new ArcanistUsageException(
+      // the commit message that "arc land" has built to be satisfactory.
-        pht(
+      $config_argv[] = '-c';
-          'Push failed! Fix the error and run "%s" again.',
+      $config_argv[] = 'git-p4.skipSubmitEdit=true';
-          'arc land'));
+
+      // Skip the "git p4 submit" confirmation prompt if the user does not edit
+      // the submit message.
+      $config_argv[] = '-c';
+      $config_argv[] = 'git-p4.skipSubmitEditCheck=true';
+
+      $flags_argv = array();
+
+      // Disable implicit "git p4 rebase" as part of submit. We're allowing
+      // the implicit "git p4 sync" to go through since this puts us in a
+      // state which is generally similar to the state after "git push", with
+      // updated remotes.
+
+      // We could do a manual "git p4 sync" with a more narrow "--branch"
+      // instead, but it's not clear that this is beneficial.
+      $flags_argv[] = '--disable-rebase';
+
+      // Detect moves and submit them to Perforce as move operations.
+      $flags_argv[] = '-M';
+
+      // If we run into a conflict, abort the operation. We expect users to
+      // fix conflicts and run "arc land" again.
+      $flags_argv[] = '--conflict=quit';
+
+      $err = $api->execPassthru(
+        '%LR p4 submit %LR --commit %s --',
+        $config_argv,
+        $flags_argv,
+        $this->mergedRef);
+
+      if ($err) {
+        throw new ArcanistUsageException(
+          pht(
+            'Submit failed! Fix the error and run "arc land" again.'));
+      }
+    } else {
+      $this->writeInfo(
+        pht('PUSHING'),
+        pht('Pushing changes to "%s".', $this->getTargetFullRef()));
+
+      $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 "arc land" again.'));
+      }
     }
   }
 
@@ -340,40 +440,54 @@ final class ArcanistGitLandEngine
       return;
     }
 
-    if (!$path->isConnectedToRemote()) {
+    $is_perforce = $this->getIsGitPerforce();
-      $this->writeInfo(
+
-        pht('UPDATE'),
+    if ($is_perforce) {
-        pht(
+      // If we're in Perforce mode, we don't expect to have a meaningful
-          'Local branch "%s" is not connected to a remote, staying on '.
+      // path to the remote: the "p4" remote is not a real remote, and
-          'detached HEAD.',
+      // "git p4" commands do not configure branch upstreams to provide
-          $local_branch));
+      // a path.
-      return;
+
+      // Just pretend the target branch is connected directly to the remote,
+      // since this is effectively the behavior of Perforce and appears to
+      // do the right thing.
+      $cascade_branches = array($local_branch);
+    } else {
+      if (!$path->isConnectedToRemote()) {
+        $this->writeInfo(
+          pht('UPDATE'),
+          pht(
+            'Local branch "%s" is not connected to a remote, staying on '.
+            'detached HEAD.',
+            $local_branch));
+        return;
+      }
+
+      $remote_remote = $path->getRemoteRemoteName();
+      $remote_branch = $path->getRemoteBranchName();
+
+      $remote_actual = $remote_remote.'/'.$remote_branch;
+      $remote_expect = $this->getTargetFullRef();
+      if ($remote_actual != $remote_expect) {
+        $this->writeInfo(
+          pht('UPDATE'),
+          pht(
+            'Local branch "%s" is connected to a remote ("%s") other than '.
+            'the target remote ("%s"), staying on detached HEAD.',
+            $local_branch,
+            $remote_actual,
+            $remote_expect));
+        return;
+      }
+
+      // If we get this far, we have a sequence of branches which ultimately
+      // connect to the remote. We're going to try to update them all in reverse
+      // order, from most-upstream to most-local.
+
+      $cascade_branches = $path->getLocalBranches();
+      $cascade_branches = array_reverse($cascade_branches);
     }
 
-    $remote_remote = $path->getRemoteRemoteName();
-    $remote_branch = $path->getRemoteBranchName();
-
-    $remote_actual = $remote_remote.'/'.$remote_branch;
-    $remote_expect = $this->getTargetFullRef();
-    if ($remote_actual != $remote_expect) {
-      $this->writeInfo(
-        pht('UPDATE'),
-        pht(
-          'Local branch "%s" is connected to a remote ("%s") other than '.
-          'the target remote ("%s"), staying on detached HEAD.',
-          $local_branch,
-          $remote_actual,
-          $remote_expect));
-      return;
-    }
-
-    // If we get this far, we have a sequence of branches which ultimately
-    // connect to the remote. We're going to try to update them all in reverse
-    // order, from most-upstream to most-local.
-
-    $cascade_branches = $path->getLocalBranches();
-    $cascade_branches = array_reverse($cascade_branches);
-
     // First, check if any of them are ahead of the remote.
 
     $ahead_of_remote = array();
@@ -436,7 +550,12 @@ final class ArcanistGitLandEngine
       }
     }
 
-    if ($cascade_targets) {
+    if ($is_perforce) {
+      // In Perforce, we've already set the remote to the right state with an
+      // implicit "git p4 sync" during "git p4 submit", and "git pull" isn't a
+      // meaningful operation. We're going to skip this step and jump down to
+      // the "git reset --hard" below to get everything into the right state.
+    } else if ($cascade_targets) {
       $this->writeInfo(
         pht('UPDATE'),
         pht(
@@ -453,7 +572,10 @@ final class ArcanistGitLandEngine
             $cascade_branch));
 
         $api->execxLocal('checkout %s --', $cascade_branch);
-        $api->execxLocal('pull --');
+        $api->execxLocal(
+          'pull %s %s --',
+          $this->getTargetRemote(),
+          $cascade_branch);
       }
 
       if (!$local_ahead) {
@@ -577,16 +699,27 @@ final class ArcanistGitLandEngine
   }
 
   private function didHoldChanges() {
-    $this->writeInfo(
+    if ($this->getIsGitPerforce()) {
-      pht('HOLD'),
+      $this->writeInfo(
-      pht(
+        pht('HOLD'),
-        'Holding change locally, it has not been pushed.'));
+        pht(
+          'Holding change locally, it has not been submitted.'));
 
-    $push_command = csprintf(
+      $push_command = csprintf(
-      '$ git push -- %R %R:%R',
+        '$ git p4 submit -M --commit %R --',
-      $this->getTargetRemote(),
+        $this->mergedRef);
-      $this->mergedRef,
+    } else {
-      $this->getTargetOnto());
+      $this->writeInfo(
+        pht('HOLD'),
+        pht(
+          'Holding change locally, it has not been pushed.'));
+
+      $push_command = csprintf(
+        '$ git push -- %R %R:%R',
+        $this->getTargetRemote(),
+        $this->mergedRef,
+        $this->getTargetOnto());
+    }
 
     $restore_command = csprintf(
       '$ git checkout %R --',
@@ -595,9 +728,9 @@ final class ArcanistGitLandEngine
     echo tsprintf(
       "\n%s\n\n".
       "%s\n\n".
-      "    %s\n\n".
+      "    **%s**\n\n".
       "%s\n\n".
-      "    %s\n\n".
+      "    **%s**\n\n".
       "%s\n",
       pht(
         'This local working copy now contains the merged changes in a '.
@@ -605,7 +738,7 @@ final class ArcanistGitLandEngine
       pht('You can push the changes manually with this command:'),
       $push_command,
       pht(
-        'You can go back to how things were before you ran `arc land` with '.
+        'You can go back to how things were before you ran "arc land" with '.
         'this command:'),
       $restore_command,
       pht(
@@ -723,11 +856,22 @@ final class ArcanistGitLandEngine
       return $remote;
     }
 
+    $remote = 'p4';
+    if ($api->isPerforceRemote($remote)) {
+      $this->writeInfo(
+        pht('REMOTE'),
+        pht(
+          'Using Perforce remote "%s". The existence of this remote implies '.
+          'this working copy was synchronized from a Perforce repository.',
+          $remote));
+      return $remote;
+    }
+
     $remote = 'origin';
     $this->writeInfo(
       pht('REMOTE'),
       pht(
-        'Using remote "%s", the default remote under git.',
+        'Using remote "%s", the default remote under Git.',
         $remote));
 
     return $remote;

src/repository/api/ArcanistGitAPI.php

@@ -1550,4 +1550,31 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return $path;
   }
 
+  public function isPerforceRemote($remote_name) {
+    // See T13434. In Perforce workflows, "git p4 clone" creates "p4" refs
+    // under "refs/remotes/", but does not define a real remote named "p4".
+
+    // We treat this remote as though it were a real remote during "arc land",
+    // but it does not respond to commands like "git remote show p4", so we
+    // need to handle it specially.
+
+    if ($remote_name !== 'p4') {
+      return false;
+    }
+
+    $remote_dir = $this->getMetadataPath().'/refs/remotes/p4';
+    if (!Filesystem::pathExists($remote_dir)) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function isPushableRemote($remote_name) {
+    list($err, $stdout) = $this->execManualLocal(
+      'remote get-url --push -- %s',
+      $remote_name);
+    return !$err;
+  }
+
 }

src/workflow/ArcanistLandWorkflow.php

@@ -245,6 +245,14 @@ EOTEXT
       // could probably be structured more cleanly.
 
       $engine->parseArguments();
+
+      // This must be configured or we fail later inside "buildEngineMessage()".
+      // This is less than ideal.
+      $this->ontoRemoteBranch = sprintf(
+        '%s/%s',
+        $engine->getTargetRemote(),
+        $engine->getTargetOnto());
+
       $this->requireCleanWorkingCopy();
       $engine->execute();