Arcanist - make Patch workflow automagically apply dependencies

Summary: Nice title. Ref T479.

Test Plan:
Actually, help on that? I want to make sure I properly build up the "depends" on data. Is it as simple as

  -- observe at some commit hash RAZZMATAZZ
  -- git checkout -B "foo"
  -- <work>
  -- git commit -m "stash"
  -- arc diff -> yields DX
  -- git checkout -B "foo_prime"
  -- <work>
  -- git commit -m "stash"
  -- arc diff -> yield DY
  -- git checkout RAZZMATAZZ
  -- arc patch DY
  -- get prompted in workflow, agree
  -- git log and observe DX and DY applied

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, davidressman

Maniphest Tasks: T479

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

src/__phutil_library_map__.php

@@ -57,6 +57,7 @@ phutil_register_library_map(array(
     'ArcanistDiffWorkflow' => 'workflow/ArcanistDiffWorkflow.php',
     'ArcanistDifferentialCommitMessage' => 'differential/ArcanistDifferentialCommitMessage.php',
     'ArcanistDifferentialCommitMessageParserException' => 'differential/ArcanistDifferentialCommitMessageParserException.php',
+    'ArcanistDifferentialDependencyGraph' => 'differential/ArcanistDifferentialDependencyGraph.php',
     'ArcanistDifferentialRevisionHash' => 'differential/constants/ArcanistDifferentialRevisionHash.php',
     'ArcanistDifferentialRevisionStatus' => 'differential/constants/ArcanistDifferentialRevisionStatus.php',
     'ArcanistDownloadWorkflow' => 'workflow/ArcanistDownloadWorkflow.php',
@@ -222,6 +223,7 @@ phutil_register_library_map(array(
     'ArcanistDiffUtilsTestCase' => 'ArcanistTestCase',
     'ArcanistDiffWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistDifferentialCommitMessageParserException' => 'Exception',
+    'ArcanistDifferentialDependencyGraph' => 'AbstractDirectedGraph',
     'ArcanistDownloadWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistEventType' => 'PhutilEventType',
     'ArcanistExportWorkflow' => 'ArcanistBaseWorkflow',

src/differential/ArcanistDifferentialDependencyGraph.php

@@ -0,0 +1,72 @@
+<?php
+
+final class ArcanistDifferentialDependencyGraph extends AbstractDirectedGraph {
+
+  private $repositoryAPI;
+  private $conduit;
+  private $startPHID;
+
+  public function setStartPHID($start_phid) {
+    $this->startPHID = $start_phid;
+    return $this;
+  }
+  public function getStartPHID() {
+    return $this->startPHID;
+  }
+
+  public function setRepositoryAPI(ArcanistRepositoryAPI $repository_api) {
+    $this->repositoryAPI = $repository_api;
+    return $this;
+  }
+  public function getRepositoryAPI() {
+    return $this->repositoryAPI;
+  }
+
+  public function setConduit(ConduitClient $conduit) {
+    $this->conduit = $conduit;
+    return $this;
+  }
+  public function getConduit() {
+    return $this->conduit;
+  }
+
+  protected function loadEdges(array $nodes) {
+    $repository_api = $this->getRepositoryAPI();
+
+    $dependencies = $this->getConduit()->callMethodSynchronous(
+      'differential.query',
+      array(
+        'phids' => $nodes,
+      ));
+
+    $edges = array();
+    foreach ($dependencies as $dependency) {
+      $dependency_revision = $this->getCommitHashFromDict($dependency);
+      if ($repository_api->hasLocalCommit($dependency_revision)) {
+        $edges[$dependency['phid']] = array();
+        continue;
+      }
+      $auxillary = idx($dependency, 'auxiliary', array());
+      $edges[$dependency['phid']] = idx(
+        $auxillary,
+        'phabricator:depends-on',
+        array());
+    }
+    return $edges;
+  }
+
+  private function getCommitHashFromDict($dict) {
+    $api = $this->getRepositoryAPI();
+    $hashes = idx($dict, 'hashes', array());
+    if ($api instanceof ArcanistGitAPI) {
+      $key = ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT;
+    } else if ($api instanceof ArcanistMercurialAPI) {
+      $key = ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT;
+    } else {
+      $key = null;
+    }
+
+    return idx($hashes, $key);
+  }
+
+}

src/workflow/ArcanistPatchWorkflow.php

@@ -93,6 +93,15 @@ EOTEXT
           "Normally under git/hg, if the patch is successful, the changes ".
           "are committed to the working copy. This flag prevents the commit.",
       ),
+      'skip-dependencies' => array(
+        'supports' => array(
+          'git', 'hg'
+        ),
+        'help' =>
+          'Normally, if a patch has dependencies that are not present in the '.
+          'working copy, arc tries to apply them as well. This flag prevents '.
+          'such work.',
+      ),
       'nobranch' => array(
         'supports' => array(
           'git', 'hg'
@@ -186,12 +195,7 @@ EOTEXT
   }
 
   private function shouldCommit() {
-    $no_commit = $this->getArgument('nocommit', false);
+    return !$this->getArgument('nocommit', false);
-    if ($no_commit) {
-      return false;
-    }
-
-    return true;
   }
 
   private function canBranch() {
@@ -285,29 +289,6 @@ EOTEXT
     return $bookmark_name;
   }
 
-  private function hasBaseRevision(ArcanistBundle $bundle) {
-    $base_revision = $bundle->getBaseRevision();
-    $repository_api = $this->getRepositoryAPI();
-
-    // verify the base revision is valid
-    if ($repository_api instanceof ArcanistGitAPI) {
-      // in a working copy that uses the git-svn bridge, the base revision might
-      // be a svn uri instead of a git ref
-
-      // NOTE: Use 'cat-file', not 'rev-parse --verify', because 'rev-parse'
-      // always "verifies" any properly-formatted commit even if it does not
-      // exist.
-      list($err) = $repository_api->execManualLocal(
-        'cat-file -t %s',
-        $base_revision);
-      return !$err;
-    } else if ($repository_api instanceof ArcanistMercurialAPI) {
-      return $repository_api->hasLocalCommit($base_revision);
-    }
-
-    return false;
-  }
-
   private function createBranch(ArcanistBundle $bundle, $has_base_revision) {
     $repository_api = $this->getRepositoryAPI();
     if ($repository_api instanceof ArcanistGitAPI) {
@@ -354,6 +335,10 @@ EOTEXT
     return $branch_name;
   }
 
+  private function shouldApplyDependencies() {
+    return !$this->getArgument('skip-dependencies', false);
+  }
+
   private function shouldUpdateWorkingCopy() {
     return $this->getArgument('update', false);
   }
@@ -423,24 +408,24 @@ EOTEXT
       $bundle->setEncoding($try_encoding);
     }
 
-    $force = $this->getArgument('force', false);
+    $sanity_check = !$this->getArgument('force', false);
-    if ($force) {
-      // force means don't do any sanity checks about the patch
-    } else {
-      $this->sanityCheck($bundle);
-    }
 
-    // we should update the working copy before we do ANYTHING else
+    // we should update the working copy before we do ANYTHING else to
+    // the working copy
     if ($this->shouldUpdateWorkingCopy()) {
       $this->updateWorkingCopy();
     }
 
-    $repository_api = $this->getRepositoryAPI();
+    if ($sanity_check) {
+      $this->requireCleanWorkingCopy();
+    }
 
-    $has_base_revision = $this->hasBaseRevision($bundle);
+    $repository_api = $this->getRepositoryAPI();
-    if ($this->shouldCommit() &&
+    $has_base_revision = $repository_api->hasLocalCommit(
-        $this->canBranch() &&
+      $bundle->getBaseRevision());
-        ($this->shouldBranch() || $has_base_revision)) {
+    if ($this->canBranch() &&
+         ($this->shouldBranch() ||
+         ($this->shouldCommit() && $has_base_revision))) {
 
       if ($repository_api instanceof ArcanistGitAPI) {
         $original_branch = $repository_api->getBranchName();
@@ -460,6 +445,13 @@ EOTEXT
 
       $new_branch = $this->createBranch($bundle, $has_base_revision);
     }
+    if (!$has_base_revision && $this->shouldApplyDependencies()) {
+      $this->applyDependencies($bundle);
+    }
+
+    if ($sanity_check) {
+      $this->sanityCheck($bundle);
+    }
 
     if ($repository_api instanceof ArcanistSubversionAPI) {
       $patch_err = 0;
@@ -716,13 +708,16 @@ EOTEXT
         $verb = 'applied';
       }
 
-      if ($this->shouldCommit() && $this->canBranch() &&
+      if ($this->canBranch() &&
-          !$this->shouldBranch() && $has_base_revision) {
+          !$this->shouldBranch() &&
+          $this->shouldCommit() && $has_base_revision) {
         $repository_api->execxLocal('checkout %s', $original_branch);
         $ex = null;
         try {
           $repository_api->execxLocal('cherry-pick %s', $new_branch);
-        } catch (Exception $ex) {}
+        } catch (Exception $ex) {
+          // do nothing
+        }
         $repository_api->execxLocal('branch -D %s', $new_branch);
         if ($ex) {
           echo phutil_console_format(
@@ -858,15 +853,72 @@ EOTEXT
     return array('ARGUMENT');
   }
 
+  private function applyDependencies(ArcanistBundle $bundle) {
+    // check for (and automagically apply on the user's be-hest) any revisions
+    // this patch depends on
+    $graph = $this->buildDependencyGraph($bundle);
+    if ($graph) {
+      $start_phid = $graph->getStartPHID();
+      $cycle_phids = $graph->detectCycles($start_phid);
+      if ($cycle_phids) {
+        $phids = array_keys($graph->getNodes());
+        $issue = 'The dependencies for this patch have a cycle. Applying them '.
+                 'is not guaranteed to work. Continue anyway?';
+        $okay = phutil_console_confirm($issue, true);
+      } else {
+        $phids = $graph->getTopographicallySortedNodes();
+        $phids = array_reverse($phids);
+        $okay = true;
+      }
+
+      if (!$okay) {
+        return;
+      }
+
+      $dep_on_revs = $this->getConduit()->callMethodSynchronous(
+        'differential.query',
+        array(
+          'phids' => $phids,
+          'arcanistProjects' => array($bundle->getProjectID())
+        ));
+      $revs = array();
+      foreach ($dep_on_revs as $dep_on_rev) {
+        $revs[$dep_on_rev['phid']] = 'D'.$dep_on_rev['id'];
+      }
+      // order them in case we got a topological sort earlier
+      $revs = array_select_keys($revs, $phids);
+      if (!empty($revs)) {
+        $base_args = array(
+          '--force',
+          '--skip-dependencies',
+          '--nobranch');
+        if (!$this->shouldCommit()) {
+          $base_args[] = '--nocommit';
+        }
+
+        foreach ($revs as $phid => $diff_id) {
+          // we'll apply this, the actual patch, later
+          // this should be the last in the list
+          if ($phid == $start_phid) {
+            continue;
+          }
+          $args = $base_args;
+          $args[] = $diff_id;
+          $apply_workflow = $this->buildChildWorkflow(
+            'patch',
+            $args);
+          $apply_workflow->run();
+        }
+      }
+    }
+  }
+
   /**
    * Do the best we can to prevent PEBKAC and id10t issues.
    */
   private function sanityCheck(ArcanistBundle $bundle) {
     $repository_api = $this->getRepositoryAPI();
 
-    // Require clean working copy
-    $this->requireCleanWorkingCopy();
-
     // Check to see if the bundle's project id matches the working copy
     // project id
     $bundle_project_id = $bundle->getProjectID();
@@ -953,8 +1005,6 @@ EOTEXT
         }
       }
     }
-
-    // TODO -- more sanity checks here
   }
 
   /**
@@ -1007,4 +1057,35 @@ EOTEXT
     }
     return $found_revision;
   }
+
+  private function buildDependencyGraph(ArcanistBundle $bundle) {
+    $graph = null;
+    if ($this->getRepositoryAPI() instanceof ArcanistSubversionAPI) {
+      return $graph;
+    }
+    $revision_id = $bundle->getRevisionID();
+    if ($revision_id) {
+      $revisions = $this->getConduit()->callMethodSynchronous(
+        'differential.query',
+        array(
+          'ids' => array($revision_id),
+        ));
+      if ($revisions) {
+        $revision = head($revisions);
+        $rev_auxiliary = idx($revision, 'auxiliary', array());
+        $phids = idx($rev_auxiliary, 'phabricator:depends-on', array());
+        if ($phids) {
+          $revision_phid = $revision['phid'];
+          $graph = id(new ArcanistDifferentialDependencyGraph())
+            ->setConduit($this->getConduit())
+            ->setRepositoryAPI($this->getRepositoryAPI())
+            ->setStartPHID($revision_phid)
+            ->addNodes(array($revision_phid => $phids))
+            ->loadGraph();
+        }
+      }
+    }
+
+    return $graph;
+  }
 }