Simplify Repository remote and local command construction

Summary:
This cleans up some garbage:

  - We were specifying environmental variables with `X=y git ...`, but now have `setEnv()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setEnv()`.
  - We were specifying the working directory with `(cd %s && git ...)`, but now have `setCWD()` on both `ExecFuture` and `PhutilExecPassthru`. Use `setCWD()`.
  - We were specifying the Git credentials with `ssh-agent -c (ssh-add ... && git ...)`. We can do this more cleanly with `GIT_SSH`. Use `GIT_SSH`.
  - Since we have to write a script for `GIT_SSH` anyway, use the same script for Subversion and Mercurial.

This fixes two specific issues:

  - Previously, we were not able to set `-o StrictHostKeyChecking=no` on Git commands, so the first time you cloned a git repo the daemons would generally prompt you to add `github.com` or whatever to `known_hosts`. Since this was non-interactive, things would mysteriously hang, in effect. With `GIT_SSH`, we can specify the flag, reducing the number of ways things can go wrong.
  - This adds `LANG=C`, which probably (?) forces the language to English for all commands. Apparently you need to install special language packs or something, so I don't know that this actually works, but at least two users with non-English languages have claimed it does (see <https://github.com/facebook/arcanist/pull/114> for a similar issue in Arcanist).

At some point in the future I might want to combine the Arcanist code for command execution with the Phabricator code for command execution (they share some stuff like LANG and HGPLAIN). However, credential management is kind of messy, so I'm adopting a "wait and see" approach for now. I expect to split this at least somewhat in the future, for Drydock/Automerge if nothing else.

Also I'm not sure if we use the passthru stuff at all anymore, I may just be able to delete that. I'll check in a future diff.

Test Plan: Browsed and pulled Git, Subversion and Mercurial repositories.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2230

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

bin/ssh-connect

@@ -0,0 +1 @@
+../scripts/ssh/ssh-connect.php

scripts/ssh/ssh-connect.php

@@ -0,0 +1,71 @@
+#!/usr/bin/env php
+<?php
+
+// This is a wrapper script for Git, Mercurial, and Subversion. It primarily
+// serves to inject "-o StrictHostKeyChecking=no" into the SSH arguments.
+
+$root = dirname(dirname(dirname(__FILE__)));
+require_once $root.'/scripts/__init_script__.php';
+
+$target_name = getenv('PHABRICATOR_SSH_TARGET');
+if (!$target_name) {
+  throw new Exception(pht("No 'PHABRICATOR_SSH_TARGET' in environment!"));
+}
+
+$repository = id(new PhabricatorRepositoryQuery())
+  ->setViewer(PhabricatorUser::getOmnipotentUser())
+  ->withCallsigns(array($target_name))
+  ->executeOne();
+if (!$repository) {
+  throw new Exception(pht('No repository with callsign "%s"!', $target_name));
+}
+
+$pattern = array();
+$arguments = array();
+
+$pattern[] = 'ssh';
+
+$pattern[] = '-o';
+$pattern[] = 'StrictHostKeyChecking=no';
+
+$login = $repository->getSSHLogin();
+if (strlen($login)) {
+  $pattern[] = '-l';
+  $pattern[] = '%P';
+  $arguments[] = new PhutilOpaqueEnvelope($login);
+}
+
+$ssh_identity = null;
+
+$key = $repository->getDetail('ssh-key');
+$keyfile = $repository->getDetail('ssh-keyfile');
+if ($keyfile) {
+  $ssh_identity = $keyfile;
+} else if ($key) {
+  $tmpfile = new TempFile('phabricator-repository-ssh-key');
+  chmod($tmpfile, 0600);
+  Filesystem::writeFile($tmpfile, $key);
+  $ssh_identity = (string)$tmpfile;
+}
+
+if ($ssh_identity) {
+  $pattern[] = '-i';
+  $pattern[] = '%P';
+  $arguments[] = new PhutilOpaqueEnvelope($keyfile);
+}
+
+$pattern[] = '--';
+
+$passthru_args = array_slice($argv, 1);
+foreach ($passthru_args as $passthru_arg) {
+  $pattern[] = '%s';
+  $arguments[] = $passthru_arg;
+}
+
+$pattern = implode(' ', $pattern);
+array_unshift($arguments, $pattern);
+
+$err = newv('PhutilExecPassthru', $arguments)
+  ->execute();
+
+exit($err);

src/applications/repository/storage/PhabricatorRepository.php

@@ -194,108 +194,179 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $uri;
   }
 
+
+/* -(  Remote Command Execution  )------------------------------------------- */
+
+
   public function execRemoteCommand($pattern /* , $arg, ... */) {
     $args = func_get_args();
-    $args = $this->formatRemoteCommand($args);
+    return $this->newRemoteCommandFuture($args)->resolve();
-    return call_user_func_array('exec_manual', $args);
   }
 
   public function execxRemoteCommand($pattern /* , $arg, ... */) {
     $args = func_get_args();
-    $args = $this->formatRemoteCommand($args);
+    return $this->newRemoteCommandFuture($args)->resolvex();
-    return call_user_func_array('execx', $args);
   }
 
   public function getRemoteCommandFuture($pattern /* , $arg, ... */) {
     $args = func_get_args();
-    $args = $this->formatRemoteCommand($args);
+    return $this->newRemoteCommandFuture($args);
-    return newv('ExecFuture', $args);
   }
 
   public function passthruRemoteCommand($pattern /* , $arg, ... */) {
     $args = func_get_args();
-    $args = $this->formatRemoteCommand($args);
+    return $this->newRemoteCommandPassthru($args)->execute();
-    return call_user_func_array('phutil_passthru', $args);
   }
 
-  public function execLocalCommand($pattern /* , $arg, ... */) {
+  private function newRemoteCommandFuture(array $argv) {
-    $this->assertLocalExists();
+    $argv = $this->formatRemoteCommand($argv);
+    $future = newv('ExecFuture', $argv);
+    $future->setEnv($this->getRemoteCommandEnvironment());
+    return $future;
+  }
 
+  private function newRemoteCommandPassthru(array $argv) {
+    $argv = $this->formatRemoteCommand($argv);
+    $passthru = newv('PhutilExecPassthru', $argv);
+    $passthru->setEnv($this->getRemoteCommandEnvironment());
+    return $passthru;
+  }
+
+
+/* -(  Local Command Execution  )-------------------------------------------- */
+
+
+  public function execLocalCommand($pattern /* , $arg, ... */) {
     $args = func_get_args();
-    $args = $this->formatLocalCommand($args);
+    return $this->newLocalCommandFuture($args)->resolve();
-    return call_user_func_array('exec_manual', $args);
   }
 
   public function execxLocalCommand($pattern /* , $arg, ... */) {
-    $this->assertLocalExists();
-
     $args = func_get_args();
-    $args = $this->formatLocalCommand($args);
+    return $this->newLocalCommandFuture($args)->resolvex();
-    return call_user_func_array('execx', $args);
   }
 
   public function getLocalCommandFuture($pattern /* , $arg, ... */) {
-    $this->assertLocalExists();
-
     $args = func_get_args();
-    $args = $this->formatLocalCommand($args);
+    return $this->newLocalCommandFuture($args);
-    return newv('ExecFuture', $args);
   }
 
   public function passthruLocalCommand($pattern /* , $arg, ... */) {
-    $this->assertLocalExists();
-
     $args = func_get_args();
-    $args = $this->formatLocalCommand($args);
+    return $this->newLocalCommandPassthru($args)->execute();
-    return call_user_func_array('phutil_passthru', $args);
   }
 
+  private function newLocalCommandFuture(array $argv) {
+    $this->assertLocalExists();
+
+    $argv = $this->formatLocalCommand($argv);
+    $future = newv('ExecFuture', $argv);
+    $future->setEnv($this->getLocalCommandEnvironment());
+
+    if ($this->usesLocalWorkingCopy()) {
+      $future->setCWD($this->getLocalPath());
+    }
+
+    return $future;
+  }
+
+  private function newLocalCommandPassthru(array $argv) {
+    $this->assertLocalExists();
+
+    $argv = $this->formatLocalCommand($argv);
+    $future = newv('PhutilExecPassthru', $argv);
+    $future->setEnv($this->getLocalCommandEnvironment());
+
+    if ($this->usesLocalWorkingCopy()) {
+      $future->setCWD($this->getLocalPath());
+    }
+
+    return $future;
+  }
+
+
+/* -(  Command Infrastructure  )--------------------------------------------- */
+
+
+  private function getSSHWrapper() {
+    $root = dirname(phutil_get_library_root('phabricator'));
+    return $root.'/bin/ssh-connect';
+  }
+
+  private function getCommonCommandEnvironment() {
+    $env = array(
+      // NOTE: Force the language to "C", which overrides locale settings.
+      // This makes stuff print in English instead of, e.g., French, so we can
+      // parse the output of some commands, error messages, etc.
+      'LANG' => 'C',
+    );
+
+    switch ($this->getVersionControlSystem()) {
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+        // NOTE: See T2965. Some time after Git 1.7.5.4, Git started fataling if
+        // it can not read $HOME. For many users, $HOME points at /root (this
+        // seems to be a default result of Apache setup). Instead, explicitly
+        // point $HOME at a readable, empty directory so that Git looks for the
+        // config file it's after, fails to locate it, and moves on. This is
+        // really silly, but seems like the least damaging approach to
+        // mitigating the issue.
+
+        $root = dirname(phutil_get_library_root('phabricator'));
+        $env['HOME'] = $root.'/support/empty/';
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+        // NOTE: This overrides certain configuration, extensions, and settings
+        // which make Mercurial commands do random unusual things.
+        $env['HGPLAIN'] = 1;
+        break;
+      default:
+        throw new Exception("Unrecognized version control system.");
+    }
+
+    return $env;
+  }
+
+  private function getLocalCommandEnvironment() {
+    return $this->getCommonCommandEnvironment();
+  }
+
+  private function getRemoteCommandEnvironment() {
+    $env = $this->getCommonCommandEnvironment();
+
+    if ($this->shouldUseSSH()) {
+      // NOTE: This is read by `bin/ssh-connect`, and tells it which credentials
+      // to use.
+      $env['PHABRICATOR_SSH_TARGET'] = $this->getCallsign();
+      switch ($this->getVersionControlSystem()) {
+        case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+          // Force SVN to use `bin/ssh-connect`.
+          $env['SVN_SSH'] = $this->getSSHWrapper();
+          break;
+        case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+          // Force Git to use `bin/ssh-connect`.
+          $env['GIT_SSH'] = $this->getSSHWrapper();
+          break;
+        case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+          // We force Mercurial through `bin/ssh-connect` too, but it uses a
+          // command-line flag instead of an environmental variable.
+          break;
+        default:
+          throw new Exception("Unrecognized version control system.");
+      }
+    }
+
+    return $env;
+  }
 
   private function formatRemoteCommand(array $args) {
     $pattern = $args[0];
     $args = array_slice($args, 1);
 
-    $empty = $this->getEmptyReadableDirectoryPath();
-
-    if ($this->shouldUseSSH()) {
-      switch ($this->getVersionControlSystem()) {
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-          $pattern = "SVN_SSH=%s svn --non-interactive {$pattern}";
-          array_unshift(
-            $args,
-            csprintf(
-              'ssh -l %P -i %P',
-              new PhutilOpaqueEnvelope($this->getSSHLogin()),
-              new PhutilOpaqueEnvelope($this->getSSHKeyfile())));
-          break;
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-          $command = call_user_func_array(
-            'csprintf',
-            array_merge(
-              array(
-                "(ssh-add %P && HOME=%s git {$pattern})",
-                new PhutilOpaqueEnvelope($this->getSSHKeyfile()),
-                $empty,
-              ),
-              $args));
-          $pattern = "ssh-agent sh -c %s";
-          $args = array($command);
-          break;
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-          $pattern = "hg --config ui.ssh=%s {$pattern}";
-          array_unshift(
-            $args,
-            csprintf(
-              'ssh -l %P -i %P',
-              new PhutilOpaqueEnvelope($this->getSSHLogin()),
-              new PhutilOpaqueEnvelope($this->getSSHKeyfile())));
-          break;
-        default:
-          throw new Exception("Unrecognized version control system.");
-      }
-    } else if ($this->shouldUseHTTP()) {
     switch ($this->getVersionControlSystem()) {
       case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        if ($this->shouldUseHTTP()) {
           $pattern =
             "svn ".
             "--non-interactive ".
@@ -308,14 +379,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
             $args,
             new PhutilOpaqueEnvelope($this->getDetail('http-login')),
             new PhutilOpaqueEnvelope($this->getDetail('http-pass')));
-          break;
-        default:
-          throw new Exception(
-            "No support for HTTP Basic Auth in this version control system.");
-      }
         } else if ($this->shouldUseSVNProtocol()) {
-      switch ($this->getVersionControlSystem()) {
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
           $pattern =
             "svn ".
             "--non-interactive ".
@@ -327,27 +391,26 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
             $args,
             new PhutilOpaqueEnvelope($this->getDetail('http-login')),
             new PhutilOpaqueEnvelope($this->getDetail('http-pass')));
-            break;
-        default:
-          throw new Exception(
-            "SVN protocol is SVN only.");
-      }
         } else {
-      switch ($this->getVersionControlSystem()) {
-        case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
           $pattern = "svn --non-interactive {$pattern}";
+        }
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-          $pattern = "HOME=%s git {$pattern}";
+        $pattern = "git {$pattern}";
-          array_unshift($args, $empty);
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+        if ($this->shouldUseSSH()) {
+          $pattern = "hg --config ui.ssh=%s {$pattern}";
+          array_unshift(
+            $args,
+            $this->getSSHWrapper());
+        } else {
           $pattern = "hg {$pattern}";
+        }
         break;
       default:
         throw new Exception("Unrecognized version control system.");
     }
-    }
 
     array_unshift($args, $pattern);
 
@@ -358,21 +421,15 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     $pattern = $args[0];
     $args = array_slice($args, 1);
 
-    $empty = $this->getEmptyReadableDirectoryPath();
-
     switch ($this->getVersionControlSystem()) {
       case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-        $pattern = "(cd %s && svn --non-interactive {$pattern})";
+        $pattern = "svn --non-interactive {$pattern}";
-        array_unshift($args, $this->getLocalPath());
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-        $pattern = "(cd %s && HOME=%s git {$pattern})";
+        $pattern = "git {$pattern}";
-        array_unshift($args, $this->getLocalPath(), $empty);
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-        $hgplain = (phutil_is_windows() ? "set HGPLAIN=1 &&" : "HGPLAIN=1");
+        $pattern = "hg {$pattern}";
-        $pattern = "(cd %s && {$hgplain} hg {$pattern})";
-        array_unshift($args, $this->getLocalPath());
         break;
       default:
         throw new Exception("Unrecognized version control system.");
@@ -383,42 +440,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $args;
   }
 
-  private function getEmptyReadableDirectoryPath() {
+  public function getSSHLogin() {
-    // See T2965. Some time after Git 1.7.5.4, Git started fataling if it can
-    // not read $HOME. For many users, $HOME points at /root (this seems to be
-    // a default result of Apache setup). Instead, explicitly point $HOME at a
-    // readable, empty directory so that Git looks for the config file it's
-    // after, fails to locate it, and moves on. This is really silly, but seems
-    // like the least damaging approach to mitigating the issue.
-    $root = dirname(phutil_get_library_root('phabricator'));
-    return $root.'/support/empty/';
-  }
-
-  private function getSSHLogin() {
     return $this->getDetail('ssh-login');
   }
 
-  private function getSSHKeyfile() {
-    if ($this->sshKeyfile === null) {
-      $key = $this->getDetail('ssh-key');
-      $keyfile = $this->getDetail('ssh-keyfile');
-      if ($keyfile) {
-        // Make sure we can read the file, that it exists, etc.
-        Filesystem::readFile($keyfile);
-        $this->sshKeyfile = $keyfile;
-      } else if ($key) {
-        $keyfile = new TempFile('phabricator-repository-ssh-key');
-        chmod($keyfile, 0600);
-        Filesystem::writeFile($keyfile, $key);
-        $this->sshKeyfile = $keyfile;
-      } else {
-        $this->sshKeyfile = '';
-      }
-    }
-
-    return (string)$this->sshKeyfile;
-  }
-
   public function getURI() {
     return '/diffusion/'.$this->getCallsign().'/';
   }
@@ -642,12 +667,16 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
     $protocol = $this->getRemoteProtocol();
     if ($this->isSSHProtocol($protocol)) {
-      return (bool)$this->getSSHKeyfile();
+      $key = $this->getDetail('ssh-key');
-    } else {
+      $keyfile = $this->getDetail('ssh-keyfile');
-      return false;
+      if ($key || $keyfile) {
+        return true;
       }
     }
 
+    return false;
+  }
+
 
   /**
    * Determine if we should connect to the remote using HTTP flags and