Make all parsers use credentials

Summary:
We need to issue all commands as $repository->junk() so we can pick up
credentials. Some of this stuff predates that change landing.

(I removed the "https" vs "svn+ssh" fallback code since it's specific to
Facebook, affected a tiny number of commits, is basically an SVN bug with UTF-8
handling and HTTP support, and doesn't make sense in the general case. The user
has the tools they need to force it via "reparse.php" if it's really an issue.)

Test Plan: Created new authenticated-remote mercurial and git repositories and
pulled/discovered them with credentials.

Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 970

src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php

@@ -32,10 +32,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
 
     $repository_phid = $repository->getPHID();
 
-    $repo_base = $repository->getDetail('local-path');
+    list($stdout) = $repository->execxLocalCommand(
-    list($stdout) = execx(
+      'branch -r --verbose --no-abbrev');
-      '(cd %s && git branch -r --verbose --no-abbrev)',
-      $repo_base);
 
     $branches = DiffusionGitBranchQuery::parseGitRemoteBranchOutput($stdout);
 
@@ -57,7 +55,6 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
     $insert = array();
 
     $repository = $this->getRepository();
-    $repo_base = $repository->getDetail('local-path');
 
     $discover[] = $commit;
     $insert[] = $commit;
@@ -66,9 +63,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
 
     while (true) {
       $target = array_pop($discover);
-      list($parents) = execx(
+      list($parents) = $repository->execxLocalCommand(
-        '(cd %s && git log -n1 --pretty="%%P" %s)',
+        'log -n1 --pretty="%%P" %s',
-        $repo_base,
         $target);
       $parents = array_filter(explode(' ', trim($parents)));
       foreach ($parents as $parent) {
@@ -93,9 +89,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon
 
     while (true) {
       $target = array_pop($insert);
-      list($epoch) = execx(
+      list($epoch) = $repository->execxLocalCommand(
-        '(cd %s && git log -n1 --pretty="%%at" %s)',
+        'log -n1 --pretty="%%at" %s',
-        $repo_base,
         $target);
       $epoch = trim($epoch);
 

src/applications/repository/daemon/commitdiscovery/git/__init__.php

@@ -10,7 +10,5 @@ phutil_require_module('phabricator', 'applications/diffusion/query/branch/git');
 phutil_require_module('phabricator', 'applications/repository/constants/repositorytype');
 phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/base');
 
-phutil_require_module('phutil', 'future/exec');
-
 
 phutil_require_source('PhabricatorRepositoryGitCommitDiscoveryDaemon.php');

src/applications/repository/daemon/commitdiscovery/mercurial/PhabricatorRepositoryMercurialCommitDiscoveryDaemon.php

@@ -29,7 +29,6 @@ class PhabricatorRepositoryMercurialCommitDiscoveryDaemon
 
     $repository_phid = $repository->getPHID();
 
-    $repo_base = $repository->getDetail('local-path');
     list($stdout) = $repository->execxLocalCommand('branches');
 
     $branches = ArcanistMercurialParser::parseMercurialBranches($stdout);

src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php

@@ -29,7 +29,7 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon
 
     $uri = $this->getBaseSVNLogURI();
     list($xml) = $repository->execxRemoteCommand(
-        ' log --xml --quiet --limit 1 %s@HEAD',
+      'log --xml --quiet --limit 1 %s@HEAD',
       $uri);
 
     $results = $this->parseSVNLogXML($xml);

src/applications/repository/daemon/gitfetch/PhabricatorRepositoryGitFetchDaemon.php

@@ -23,12 +23,22 @@ final class PhabricatorRepositoryGitFetchDaemon
     return PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;
   }
 
-  protected function executeCreate($remote_uri, $local_path) {
+  protected function executeCreate(
-    execx('git clone %s %s', $remote_uri, rtrim($local_path, '/'));
+    PhabricatorRepository $repository,
+    $local_path) {
+
+    $repository->execxRemoteCommand(
+      'clone %s %s',
+      $repository->getRemoteURI(),
+      rtrim($local_path, '/'));
   }
 
-  protected function executeUpdate($remote_uri, $local_path) {
+  protected function executeUpdate(
-    execx('(cd %s && git fetch --all)', $local_path);
+    PhabricatorRepository $repository,
+    $local_path) {
+
+    $repository->execxLocalCommand(
+      'fetch --all');
   }
 
 }

src/applications/repository/daemon/gitfetch/__init__.php

@@ -9,7 +9,5 @@
 phutil_require_module('phabricator', 'applications/repository/constants/repositorytype');
 phutil_require_module('phabricator', 'applications/repository/daemon/pulllocal');
 
-phutil_require_module('phutil', 'future/exec');
-
 
 phutil_require_source('PhabricatorRepositoryGitFetchDaemon.php');

src/applications/repository/daemon/mercurialpull/PhabricatorRepositoryMercurialPullDaemon.php

@@ -23,12 +23,20 @@ final class PhabricatorRepositoryMercurialPullDaemon
     return PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL;
   }
 
-  protected function executeCreate($remote_uri, $local_path) {
+  protected function executeCreate(
-    execx('hg clone %s %s', $remote_uri, rtrim($local_path, '/'));
+    PhabricatorRepository $repository,
+    $local_path) {
+    $repository->execxRemoteCommand(
+      'clone %s %s',
+      $repository->getRemoteURI(),
+      rtrim($local_path, '/'));
   }
 
-  protected function executeUpdate($remote_uri, $local_path) {
+  protected function executeUpdate(
-    execx('(cd %s && hg pull -u)', $local_path);
+    PhabricatorRepository $repository,
+    $local_path) {
+    $repository->execxLocalCommand(
+      'pull -u');
   }
 
 }

src/applications/repository/daemon/mercurialpull/__init__.php

@@ -9,7 +9,5 @@
 phutil_require_module('phabricator', 'applications/repository/constants/repositorytype');
 phutil_require_module('phabricator', 'applications/repository/daemon/pulllocal');
 
-phutil_require_module('phutil', 'future/exec');
-
 
 phutil_require_source('PhabricatorRepositoryMercurialPullDaemon.php');

src/applications/repository/daemon/pulllocal/PhabricatorRepositoryPullLocalDaemon.php

@@ -20,8 +20,12 @@ abstract class PhabricatorRepositoryPullLocalDaemon
   extends PhabricatorRepositoryDaemon {
 
   abstract protected function getSupportedRepositoryType();
-  abstract protected function executeCreate($remote_uri, $local_path);
+  abstract protected function executeCreate(
-  abstract protected function executeUpdate($remote_uri, $local_path);
+    PhabricatorRepository $repository,
+    $local_path);
+  abstract protected function executeUpdate(
+    PhabricatorRepository $repository,
+    $local_path);
 
   final public function run() {
     $repository = $this->loadRepository();
@@ -45,7 +49,6 @@ abstract class PhabricatorRepositoryPullLocalDaemon
     }
 
     $local_path = $repository->getDetail('local-path');
-    $remote_uri = $repository->getDetail('remote-uri');
 
     if (!$local_path) {
       throw new Exception("No local path is available for this repository.");
@@ -53,13 +56,10 @@ abstract class PhabricatorRepositoryPullLocalDaemon
 
     while (true) {
       if (!Filesystem::pathExists($local_path)) {
-        if (!$remote_uri) {
-          throw new Exception("No remote URI is available.");
-        }
         execx('mkdir -p %s', dirname($local_path));
-        $this->executeCreate($remote_uri, $local_path);
+        $this->executeCreate($repository, $local_path);
       } else {
-        $this->executeUpdate($remote_uri, $local_path);
+        $this->executeUpdate($repository, $local_path);
       }
       $this->sleep($repository->getDetail('pull-frequency', 15));
     }

src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php

@@ -67,26 +67,10 @@ abstract class PhabricatorRepositoryCommitParserWorker
       $verbose = '--verbose';
     }
 
-    try {
     list($xml) = $this->repository->execxRemoteCommand(
       "log --xml {$verbose} --limit 1 %s@%d",
       $uri,
       $revision);
-    } catch (CommandException $ex) {
-      // HTTPS is generally faster and more reliable than svn+ssh, but some
-      // commit messages with non-UTF8 text can't be retrieved over HTTPS, see
-      // Facebook rE197184 for one example. Make an attempt to fall back to
-      // svn+ssh if we've failed outright to retrieve the message.
-      $fallback_uri = new PhutilURI($uri);
-      if ($fallback_uri->getProtocol() != 'https') {
-        throw $ex;
-      }
-      $fallback_uri->setProtocol('svn+ssh');
-      list($xml) = execx(
-        "svn log --xml {$verbose} --limit 1 --non-interactive %s@%d",
-        $fallback_uri,
-        $revision);
-    }
 
     // Subversion may send us back commit messages which won't parse because
     // they have non UTF-8 garbage in them. Slam them into valid UTF-8.

src/applications/repository/worker/base/__init__.php

@@ -11,8 +11,6 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository
 phutil_require_module('phabricator', 'infrastructure/daemon/workers/worker');
 phutil_require_module('phabricator', 'storage/queryfx');
 
-phutil_require_module('phutil', 'future/exec');
-phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/repository/worker/commitchangeparser/git/PhabricatorRepositoryGitCommitChangeParserWorker.php

@@ -30,12 +30,11 @@ class PhabricatorRepositoryGitCommitChangeParserWorker
       return;
     }
 
-    $local_path = $repository->getDetail('local-path');
+    // NOTE: "--pretty=format: " is to disable log output, we only want the
-
+    // part we get from "--raw".
-    list($raw) = execx(
+    list($raw) = $repository->execLocalCommand(
-      '(cd %s && git log -n1 -M -C -B --find-copies-harder --raw -t '.
+      'log -n1 -M -C -B --find-copies-harder --raw -t '.
-        '--abbrev=40 --pretty=format: %s)',
+        '--abbrev=40 --pretty=format: %s',
-      $local_path,
       $commit->getCommitIdentifier());
 
     $changes = array();

src/applications/repository/worker/commitchangeparser/git/__init__.php

@@ -11,7 +11,5 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository
 phutil_require_module('phabricator', 'applications/repository/worker/commitchangeparser/base');
 phutil_require_module('phabricator', 'storage/queryfx');
 
-phutil_require_module('phutil', 'future/exec');
-
 
 phutil_require_source('PhabricatorRepositoryGitCommitChangeParserWorker.php');