(stable) Promote 2019 Week 44

src/land/ArcanistGitLandEngine.php

@@ -8,6 +8,55 @@ 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.'));
+
+      if (!$this->getShouldSquash()) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Perforce mode does not support the "merge" land strategy. '.
+            'Use the "squash" land strategy when landing to a Perforce '.
+            'remote (you can use "--squash" to select this strategy).'));
+      }
+    }
+
+    $this->setTargetRemote($remote);
+  }
 
   public function execute() {
     $this->verifySourceAndTargetExist();
@@ -97,6 +146,23 @@ final class ArcanistGitLandEngine
       'rev-parse --verify %s',
       $this->getTargetFullRef());
 
+    if ($err) {
+      $this->writeWarn(
+        pht('TARGET'),
+        pht(
+          'No local ref exists for branch "%s" in remote "%s", attempting '.
+          'fetch...',
+          $this->getTargetOnto(),
+          $this->getTargetRemote()));
+
+      $api->execManualLocal(
+        'fetch %s %s --',
+        $this->getTargetRemote(),
+        $this->getTargetOnto());
+
+      list($err) = $api->execManualLocal(
+        'rev-parse --verify %s',
+        $this->getTargetFullRef());
       if ($err) {
         throw new Exception(
           pht(
@@ -105,6 +171,14 @@ final class ArcanistGitLandEngine
             $this->getTargetRemote()));
       }
 
+      $this->writeInfo(
+        pht('FETCHED'),
+        pht(
+          'Fetched branch "%s" from remote "%s".',
+          $this->getTargetOnto(),
+          $this->getTargetRemote()));
+    }
+
     list($err, $stdout) = $api->execManualLocal(
       'rev-parse --verify %s',
       $this->getSourceRef());
@@ -124,15 +198,35 @@ 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.
 
+    if ($this->getIsGitPerforce()) {
+      $this->writeInfo(
+        pht('P4 SYNC'),
+        pht('Synchronizing "%s" from Perforce...', $ref));
+
+      $sync_ref = sprintf(
+        'refs/remotes/%s/%s',
+        $this->getTargetRemote(),
+        $this->getTargetOnto());
+
+      $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(),
@@ -141,8 +235,8 @@ final class ArcanistGitLandEngine
       if ($err) {
         throw new ArcanistUsageException(
           pht(
-          'Fetch failed! Fix the error and run "%s" again.',
+            'Fetch failed! Fix the error and run "arc land" again.'));
-          'arc land'));
+      }
     }
   }
 
@@ -212,6 +306,53 @@ final class ArcanistGitLandEngine
   private function pushChange() {
     $api = $this->getRepositoryAPI();
 
+    if ($this->getIsGitPerforce()) {
+      $this->writeInfo(
+        pht('SUBMITTING'),
+        pht('Submitting changes to "%s".', $this->getTargetFullRef()));
+
+      $config_argv = array();
+
+      // Skip the "git p4 submit" interactive editor workflow. We expect
+      // the commit message that "arc land" has built to be satisfactory.
+      $config_argv[] = '-c';
+      $config_argv[] = 'git-p4.skipSubmitEdit=true';
+
+      // 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 %R --',
+        $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()));
@@ -225,8 +366,8 @@ final class ArcanistGitLandEngine
       if ($err) {
         throw new ArcanistUsageException(
           pht(
-          'Push failed! Fix the error and run "%s" again.',
+            'Push failed! Fix the error and run "arc land" again.'));
-          'arc land'));
+      }
     }
   }
 
@@ -332,6 +473,19 @@ final class ArcanistGitLandEngine
       return;
     }
 
+    $is_perforce = $this->getIsGitPerforce();
+
+    if ($is_perforce) {
+      // If we're in Perforce mode, we don't expect to have a meaningful
+      // path to the remote: the "p4" remote is not a real remote, and
+      // "git p4" commands do not configure branch upstreams to provide
+      // a path.
+
+      // 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'),
@@ -365,6 +519,7 @@ final class ArcanistGitLandEngine
 
       $cascade_branches = $path->getLocalBranches();
       $cascade_branches = array_reverse($cascade_branches);
+    }
 
     // First, check if any of them are ahead of the remote.
 
@@ -428,7 +583,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(
@@ -445,7 +605,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) {
@@ -569,6 +732,16 @@ final class ArcanistGitLandEngine
   }
 
   private function didHoldChanges() {
+    if ($this->getIsGitPerforce()) {
+      $this->writeInfo(
+        pht('HOLD'),
+        pht(
+          'Holding change locally, it has not been submitted.'));
+
+      $push_command = csprintf(
+        '$ git p4 submit -M --commit %R --',
+        $this->mergedRef);
+    } else {
       $this->writeInfo(
         pht('HOLD'),
         pht(
@@ -579,6 +752,7 @@ final class ArcanistGitLandEngine
         $this->getTargetRemote(),
         $this->mergedRef,
         $this->getTargetOnto());
+    }
 
     $restore_command = csprintf(
       '$ git checkout %R --',
@@ -587,9 +761,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 '.
@@ -597,7 +771,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(
@@ -615,4 +789,125 @@ 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 = '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.',
+        $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/repository/api/ArcanistGitAPI.php

@@ -464,15 +464,27 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
    */
   public function getFullGitDiff($base, $head = null) {
     $options = $this->getDiffFullOptions();
+    $config_options = array();
+
+    // See T13432. Disable the rare "diff.suppressBlankEmpty" configuration
+    // option, which discards the " " (space) change type prefix on unchanged
+    // blank lines. At time of writing the parser does not handle these
+    // properly, but generating a more-standard diff is generally desirable
+    // even if a future parser handles this case more gracefully.
+
+    $config_options[] = '-c';
+    $config_options[] = 'diff.suppressBlankEmpty=false';
 
     if ($head !== null) {
       list($stdout) = $this->execxLocal(
-        "diff {$options} %s %s --",
+        "%LR diff {$options} %s %s --",
+        $config_options,
         $base,
         $head);
     } else {
       list($stdout) = $this->execxLocal(
-        "diff {$options} %s --",
+        "%LR diff {$options} %s --",
+        $config_options,
         $base);
     }
 
@@ -1550,4 +1562,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

@@ -44,10 +44,10 @@ EOTEXT
 
   public function getCommandHelp() {
     return phutil_console_format(<<<EOTEXT
-          Supports: git, hg
+          Supports: git, git/p4, hg
 
           Publish an accepted revision after review. This command is the last
-          step in the standard Differential pre-publish code review workflow.
+          step in the standard Differential code review workflow.
 
           This command merges and pushes changes associated with an accepted
           revision that are currently sitting in __ref__, which is usually the
@@ -57,6 +57,9 @@ EOTEXT
           Under Git: branches, tags, and arbitrary commits (detached HEADs)
           may be landed.
 
+          Under Git/Perforce: branches, tags, and arbitrary commits may
+          be submitted.
+
           Under Mercurial: branches and bookmarks may be landed, but only
           onto a target of the same type. See T3855.
 
@@ -66,7 +69,8 @@ EOTEXT
           A target branch is selected by examining these sources in order:
 
             - the **--onto** flag;
-            - the upstream of the current branch, recursively (Git only);
+            - the upstream of the branch targeted by the land operation,
+              recursively (Git only);
             - the __arc.land.onto.default__ configuration setting;
             - or by falling back to a standard default:
               - "master" in Git;
@@ -76,6 +80,8 @@ EOTEXT
 
             - the **--remote** flag;
             - the upstream of the current branch, recursively (Git only);
+            - the special "p4" remote which indicates a repository has
+              been synchronized with Perforce (Git only);
             - or by falling back to a standard default:
               - "origin" in Git;
               - the default remote in Mercurial.
@@ -159,7 +165,7 @@ EOTEXT
       'remote' => array(
         'param' => 'origin',
         'help' => pht(
-          "Push to a remote other than the default ('origin' in git)."),
+          'Push to a remote other than the default.'),
       ),
       'merge' => array(
         'help' => pht(
@@ -224,23 +230,36 @@ 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 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();
 
       if (!$should_hold && !$this->preview) {
@@ -337,123 +356,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;