Allow SVN repositories to import subdirectories instead of the entire repository

Summary:
See T325. While this is a touch hacky it ends up being fairly clean, and we can
now do initial imports much more quickly and this actually cleaned up some of
the code. I also made the repository edit interface a little less foreboding.

@tuomaspelkonen, did you get anywhere with that bug you were chasing down a
couple days ago? We can hold this if it throws a wrench into stuff you're
working on.

Test Plan:
  - Imported a subdirectory of a midsized SVN project (jQuery UI).
  - Commit discovery for ~3500/4500 commits took just a few seconds.
  - Commit discovery correctly ignored commits which didn't affect this
directory.
  - Commit discovery correctly stopped at commit 13.
  - Browse interface shows an incomplete listing, but that's fine, and
everything is otherwise functionally correct. We can add a note or something
later ("this is a view of commits affecting a subdirectory, some paths aren't
available"), but this behavior probably won't be too startling to users.
  - Edited Git and SVN repositories to test form logic.

Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran, Girish
Commenters: tuomaspelkonen
CC: jcleveley, aran, jungejason, tuomaspelkonen
Differential Revision: 696

src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php

@@ -32,7 +32,6 @@ class PhabricatorRepositoryEditController
   public function processRequest() {
 
     $request = $this->getRequest();
-    $user = $request->getUser();
 
     $repository = id(new PhabricatorRepository())->load($this->id);
     if (!$repository) {
@@ -52,7 +51,7 @@ class PhabricatorRepositoryEditController
         $repository->save();
       }
 
-      $views['github'] = 'Github';
+      $views['github'] = 'GitHub';
     }
 
     $this->repository = $repository;
@@ -96,11 +95,6 @@ class PhabricatorRepositoryEditController
     $repository = $this->repository;
     $repository_id = $repository->getID();
 
-    $type_map = array(
-      'svn' => 'Subversion',
-      'git' => 'Git',
-    );
-
     $errors = array();
 
     $e_name = true;
@@ -223,7 +217,9 @@ class PhabricatorRepositoryEditController
       $tracking = ($request->getStr('tracking') == 'enabled' ? true : false);
       $repository->setDetail('tracking-enabled', $tracking);
       $repository->setDetail('remote-uri', $request->getStr('uri'));
-      $repository->setDetail('local-path', $request->getStr('path'));
+      if ($is_git) {
+        $repository->setDetail('local-path', $request->getStr('path'));
+      }
       $repository->setDetail(
         'pull-frequency',
         max(1, $request->getInt('frequency')));
@@ -246,6 +242,11 @@ class PhabricatorRepositoryEditController
 
       if ($is_svn) {
         $repository->setUUID($request->getStr('uuid'));
+        $subpath = ltrim($request->getStr('svn-subpath'), '/');
+        if ($subpath) {
+          $subpath = rtrim($subpath, '/').'/';
+        }
+        $repository->setDetail('svn-subpath', $subpath);
       }
 
       $repository->setDetail(
@@ -262,16 +263,18 @@ class PhabricatorRepositoryEditController
           !preg_match('@/$@', $repository->getDetail('remote-uri'))) {
 
           $e_uri = 'Invalid';
-          $errors[] = 'Subversion Repository URI must end in a slash ("/").';
+          $errors[] = 'Subversion Repository Root must end in a slash ("/").';
         } else {
           $e_uri = null;
         }
 
-        if (!$repository->getDetail('local-path')) {
+        if ($is_git) {
-          $e_path = 'Required';
+          if (!$repository->getDetail('local-path')) {
-          $errors[] = "Local path is required.";
+            $e_path = 'Required';
-        } else {
+            $errors[] = "Local path is required.";
-          $e_path = null;
+          } else {
+            $e_path = null;
+          }
         }
       }
 
@@ -296,29 +299,23 @@ class PhabricatorRepositoryEditController
         'before changes will take effect.');
     }
 
-    $uri_caption = null;
-    $path_caption = null;
-
-
     switch ($repository->getVersionControlSystem()) {
       case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
         $is_git = true;
-        $uri_caption =
-          'The user the tracking daemon runs as must have permission to '.
-          '<tt>git clone</tt> from this URI.';
-        $path_caption =
-          'Directory where the daemon should look to find a copy of the '.
-          'repository (or create one if it does not yet exist). The daemon '.
-          'will regularly pull remote changes into this working copy.';
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
         $is_svn = true;
-        $uri_caption =
-          'The user the tracking daemon runs as must have permission to '.
-          '<tt>svn log</tt> from this URI.';
         break;
     }
 
+    $doc_href = PhabricatorEnv::getDoclink('article/Diffusion_User_Guide.html');
+    $user_guide_link = phutil_render_tag(
+      'a',
+      array(
+        'href' => $doc_href,
+      ),
+      'Diffusion User Guide');
+
     $form = new AphrontFormView();
     $form
       ->setUser($user)
@@ -328,7 +325,8 @@ class PhabricatorRepositoryEditController
         'repositories, importing commits as they happen and notifying '.
         'Differential, Diffusion, Herald, and other services. To enable '.
         'tracking for a repository, configure it here and then start (or '.
-        'restart) the daemons (TODO: explain this).</p>')
+        'restart) the daemons. More information is available in the '.
+        '<strong>'.$user_guide_link.'</strong>.</p>')
       ->appendChild(
         id(new AphrontFormStaticControl())
           ->setLabel('Repository')
@@ -344,21 +342,67 @@ class PhabricatorRepositoryEditController
           ->setValue(
             $repository->getDetail('tracking-enabled')
               ? 'enabled'
-              : 'disabled'))
+              : 'disabled'));
+
+    $uri_label = 'Repository URI';
+    if ($is_git) {
+      $instructions =
+        'NOTE: The user the tracking daemon runs as must have permission to '.
+        '<tt>git clone</tt> from this URI.';
+      $form->appendChild(
+        '<p class="aphront-form-instructions">'.$instructions.'</p>');
+    } else if ($is_svn) {
+      $instructions =
+        'Enter the <strong>Repository Root</strong> for this SVN repository. '.
+        'You can figure this out by running <tt>svn info</tt> and looking at '.
+        'the value in the <tt>Repository Root</tt> field. It should be a URI '.
+        'and look like <tt>http://svn.example.org/svn/</tt> or '.
+        '<tt>svn+ssh://svn.example.com/svnroot/</tt>.'.
+        '<br /><br />'.
+        'NOTE: The user the daemons run as must be able to execute '.
+        '<tt>svn log</tt> against this URI.';
+      $form->appendChild(
+        '<p class="aphront-form-instructions">'.$instructions.'</p>');
+      $uri_label = 'Repository Root';
+    }
+
+    $form
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setName('uri')
-          ->setLabel('URI')
+          ->setLabel($uri_label)
           ->setValue($repository->getDetail('remote-uri'))
-          ->setError($e_uri)
+          ->setError($e_uri));
-          ->setCaption($uri_caption))
+
-      ->appendChild(
+    if ($is_git) {
+      $form->appendChild(
+        '<p class="aphront-form-instructions">Select a path on local disk '.
+        'which the daemons should <tt>git clone</tt> the repository into. '.
+        'This must be readable and writable by the daemons, and readable by '.
+        'the webserver. The daemons will <tt>git fetch</tt> and keep this '.
+        'repository up to date.</p>');
+      $form->appendChild(
         id(new AphrontFormTextControl())
           ->setName('path')
           ->setLabel('Local Path')
           ->setValue($repository->getDetail('local-path'))
-          ->setError($e_path)
+          ->setError($e_path));
-          ->setCaption($path_caption))
+    } else if ($is_svn) {
+      $form->appendChild(
+        '<p class="aphront-form-instructions">If you only want to parse one '.
+        'subpath of the repository, specify it here, relative to the '.
+        'repository root (e.g., <tt>trunk/</tt> or <tt>projects/wheel/</tt>). '.
+        'If you want to parse multiple subdirectories, create a separate '.
+        'Phabricator repository for each one.</p>');
+      $form->appendChild(
+        id(new AphrontFormTextControl())
+          ->setName('svn-subpath')
+          ->setLabel('Subpath')
+          ->setValue($repository->getDetail('svn-subpath'))
+          ->setError($e_path));
+    }
+
+    $form
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setName('frequency')

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

@@ -27,51 +27,96 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon
       throw new Exception("Repository is not a svn repository.");
     }
 
-    $repository_phid = $repository->getPHID();
+    $uri = $this->getBaseSVNLogURI();
-
-    $uri = $repository->getDetail('remote-uri');
     list($xml) = execx(
       'svn log --xml --non-interactive --quiet --limit 1 %s@HEAD',
       $uri);
 
-    // TODO: We need to slam the XML output into valid UTF-8.
+    $results = $this->parseSVNLogXML($xml);
-
+    $commit = key($results);
-    $log = new SimpleXMLElement($xml);
+    $epoch  = reset($results);
-    $entry = $log->logentry[0];
-    $commit = (int)$entry['revision'];
 
     if ($this->isKnownCommit($commit)) {
       return false;
     }
 
-    $this->discoverCommit($commit);
+    $this->discoverCommit($commit, $epoch);
 
     return true;
   }
 
-  private function discoverCommit($commit) {
+  private function discoverCommit($commit, $epoch) {
-    $discover = array();
+    $uri = $this->getBaseSVNLogURI();
-    $largest_known = $commit - 1;
+
-    while ($largest_known > 0 && !$this->isKnownCommit($largest_known)) {
+    $discover = array(
-      $largest_known--;
+      $commit => $epoch,
+    );
+    $upper_bound = $commit;
+
+    $limit = 1;
+    while ($upper_bound > 1 && !$this->isKnownCommit($upper_bound)) {
+      // Find all the unknown commits on this path. Note that we permit
+      // importing an SVN subdirectory rather than the entire repository, so
+      // commits may be nonsequential.
+      list($err, $xml, $stderr) = exec_manual(
+        'svn log --xml --non-interactive --quiet --limit %d %s@%d',
+        $limit,
+        $uri,
+        $upper_bound - 1);
+      if ($err) {
+        if (preg_match('/path not found/', $stderr)) {
+          // We've gone all the way back through history and this path was not
+          // affected by earlier commits.
+          break;
+        } else {
+          throw new Exception("svn log error #{$err}: {$stderr}");
+        }
+      }
+      $discover += $this->parseSVNLogXML($xml);
+
+      $upper_bound = min(array_keys($discover));
+
+      // Discover 2, 4, 8, ... 256 logs at a time. This allows us to initially
+      // import large repositories fairly quickly, while pulling only as much
+      // data as we need in the common case (when we've already imported the
+      // repository and are just grabbing one commit at a time).
+      $limit = min($limit * 2, 256);
     }
 
-    $repository = $this->getRepository();
+    // NOTE: We do writes only after discovering all the commits so that we're
-    $uri = $repository->getDetail('remote-uri');
+    // never left in a state where we've missed commits -- if the discovery
+    // script terminates it can always resume and restore the import to a good
+    // state. This is also why we sort the discovered commits so we can do
+    // writes forward from the smallest one.
 
-    for ($ii = $largest_known + 1; $ii <= $commit; $ii++) {
+    ksort($discover);
-      list($xml) = execx(
+    foreach ($discover as $commit => $epoch) {
-        'svn log --xml --non-interactive --quiet --limit 1 %s@%d',
+      $this->recordCommit($commit, $epoch);
-        $uri,
-        $ii);
-      $log = new SimpleXMLElement($xml);
-      $entry = $log->logentry[0];
-
-      $identifier = (int)$entry['revision'];
-      $epoch = (int)strtotime((string)$entry->date[0]);
-
-      $this->recordCommit($identifier, $epoch);
     }
   }
 
+  private function parseSVNLogXML($xml) {
+    $xml = phutil_utf8ize($xml);
+
+    $result = array();
+
+    $log = new SimpleXMLElement($xml);
+    foreach ($log->logentry as $entry) {
+      $commit = (int)$entry['revision'];
+      $epoch  = (int)strtotime((string)$entry->date[0]);
+      $result[$commit] = $epoch;
+    }
+
+    return $result;
+  }
+
+
+  private function getBaseSVNLogURI() {
+    $repository = $this->getRepository();
+
+    $uri = $repository->getDetail('remote-uri');
+    $subpath = $repository->getDetail('svn-subpath');
+
+    return $uri.$subpath;
+  }
 }

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

@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/repository/constants/reposito
 phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/base');
 
 phutil_require_module('phutil', 'future/exec');
+phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('PhabricatorRepositorySvnCommitDiscoveryDaemon.php');