Move commit attachment to a separate CLI command

Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.

This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.

Test Plan:
  - Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
  - Faked the message parser to make sure stuff still worked there.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

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

src/__phutil_library_map__.php

@@ -2143,6 +2143,7 @@ phutil_register_library_map(array(
     'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php',
     'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php',
     'PhabricatorDifferentialApplication' => 'applications/differential/application/PhabricatorDifferentialApplication.php',
+    'PhabricatorDifferentialAttachCommitWorkflow' => 'applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php',
     'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php',
     'PhabricatorDifferentialExtractWorkflow' => 'applications/differential/management/PhabricatorDifferentialExtractWorkflow.php',
     'PhabricatorDifferentialManagementWorkflow' => 'applications/differential/management/PhabricatorDifferentialManagementWorkflow.php',
@@ -6379,6 +6380,7 @@ phutil_register_library_map(array(
     'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorSettingsPanel',
     'PhabricatorDifferenceEngine' => 'Phobject',
     'PhabricatorDifferentialApplication' => 'PhabricatorApplication',
+    'PhabricatorDifferentialAttachCommitWorkflow' => 'PhabricatorDifferentialManagementWorkflow',
     'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDifferentialExtractWorkflow' => 'PhabricatorDifferentialManagementWorkflow',
     'PhabricatorDifferentialManagementWorkflow' => 'PhabricatorManagementWorkflow',

src/applications/differential/engine/DifferentialDiffExtractionEngine.php

@@ -87,4 +87,174 @@ final class DifferentialDiffExtractionEngine extends Phobject {
     return $diff->save();
   }
 
+  public function isDiffChangedBeforeCommit(
+    PhabricatorRepositoryCommit $commit,
+    DifferentialDiff $old,
+    DifferentialDiff $new) {
+
+    $viewer = $this->getViewer();
+    $repository = $commit->getRepository();
+    $identifier = $commit->getCommitIdentifier();
+
+    $vs_changesets = array();
+    foreach ($old->getChangesets() as $changeset) {
+      $path = $changeset->getAbsoluteRepositoryPath($repository, $old);
+      $path = ltrim($path, '/');
+      $vs_changesets[$path] = $changeset;
+    }
+
+    $changesets = array();
+    foreach ($new->getChangesets() as $changeset) {
+      $path = $changeset->getAbsoluteRepositoryPath($repository, $new);
+      $path = ltrim($path, '/');
+      $changesets[$path] = $changeset;
+    }
+
+    if (array_fill_keys(array_keys($changesets), true) !=
+        array_fill_keys(array_keys($vs_changesets), true)) {
+      return true;
+    }
+
+    $file_phids = array();
+    foreach ($vs_changesets as $changeset) {
+      $metadata = $changeset->getMetadata();
+      $file_phid = idx($metadata, 'new:binary-phid');
+      if ($file_phid) {
+        $file_phids[$file_phid] = $file_phid;
+      }
+    }
+
+    $files = array();
+    if ($file_phids) {
+      $files = id(new PhabricatorFileQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs($file_phids)
+        ->execute();
+      $files = mpull($files, null, 'getPHID');
+    }
+
+    foreach ($changesets as $path => $changeset) {
+      $vs_changeset = $vs_changesets[$path];
+
+      $file_phid = idx($vs_changeset->getMetadata(), 'new:binary-phid');
+      if ($file_phid) {
+        if (!isset($files[$file_phid])) {
+          return true;
+        }
+
+        $drequest = DiffusionRequest::newFromDictionary(array(
+          'user' => $viewer,
+          'repository' => $repository,
+          'commit' => $identifier,
+          'path' => $path,
+        ));
+
+        $corpus = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->loadFileContent()
+          ->getCorpus();
+
+        if ($files[$file_phid]->loadFileData() != $corpus) {
+          return true;
+        }
+      } else {
+        $context = implode("\n", $changeset->makeChangesWithContext());
+        $vs_context = implode("\n", $vs_changeset->makeChangesWithContext());
+
+        // We couldn't just compare $context and $vs_context because following
+        // diffs will be considered different:
+        //
+        //   -(empty line)
+        //   -echo 'test';
+        //    (empty line)
+        //
+        //    (empty line)
+        //   -echo "test";
+        //   -(empty line)
+
+        $hunk = id(new DifferentialModernHunk())->setChanges($context);
+        $vs_hunk = id(new DifferentialModernHunk())->setChanges($vs_context);
+        if ($hunk->makeOldFile() != $vs_hunk->makeOldFile() ||
+            $hunk->makeNewFile() != $vs_hunk->makeNewFile()) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
+  public function updateRevisionWithCommit(
+    DifferentialRevision $revision,
+    PhabricatorRepositoryCommit $commit,
+    array $more_xactions,
+    PhabricatorContentSource $content_source) {
+
+    $viewer = $this->getViewer();
+    $result_data = array();
+
+    $new_diff = $this->newDiffFromCommit($commit);
+
+    $old_diff = $revision->getActiveDiff();
+    $changed_uri = null;
+    if ($old_diff) {
+      $old_diff = id(new DifferentialDiffQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($old_diff->getID()))
+        ->needChangesets(true)
+        ->executeOne();
+      if ($old_diff) {
+        $has_changed = $this->isDiffChangedBeforeCommit(
+          $commit,
+          $old_diff,
+          $new_diff);
+        if ($has_changed) {
+          $result_data['vsDiff'] = $old_diff->getID();
+
+          $revision_monogram = $revision->getMonogram();
+          $old_id = $old_diff->getID();
+          $new_id = $new_diff->getID();
+
+          $changed_uri = "/{$revision_monogram}?vs={$old_id}&id={$new_id}#toc";
+          $changed_uri = PhabricatorEnv::getProductionURI($changed_uri);
+        }
+      }
+    }
+
+    $xactions = array();
+
+    $xactions[] = id(new DifferentialTransaction())
+      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setIgnoreOnNoEffect(true)
+      ->setNewValue($new_diff->getPHID())
+      ->setMetadataValue('isCommitUpdate', true);
+
+    foreach ($more_xactions as $more_xaction) {
+      $xactions[] = $more_xaction;
+    }
+
+    $editor = id(new DifferentialTransactionEditor())
+      ->setActor($viewer)
+      ->setContinueOnMissingFields(true)
+      ->setContentSource($content_source)
+      ->setChangedPriorToCommitURI($changed_uri)
+      ->setIsCloseByCommit(true);
+
+    $author_phid = $this->getAuthorPHID();
+    if ($author_phid !== null) {
+      $editor->setActingAsPHID($author_phid);
+    }
+
+    try {
+      $editor->applyTransactions($revision, $xactions);
+    } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
+      // NOTE: We've marked transactions other than the CLOSE transaction
+      // as ignored when they don't have an effect, so this means that we
+      // lost a race to close the revision. That's perfectly fine, we can
+      // just continue normally.
+    }
+
+    return $result_data;
+  }
+
 }

src/applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php

@@ -0,0 +1,90 @@
+<?php
+
+final class PhabricatorDifferentialAttachCommitWorkflow
+  extends PhabricatorDifferentialManagementWorkflow {
+
+  protected function didConstruct() {
+    $this
+      ->setName('attach-commit')
+      ->setExamples('**attach-commit** __commit__ __revision__')
+      ->setSynopsis(pht('Forcefully attach a commit to a revision.'))
+      ->setArguments(
+        array(
+          array(
+            'name' => 'argv',
+            'wildcard' => true,
+            'help' => pht('Commit, and a revision to attach it to.'),
+          ),
+        ));
+  }
+
+  public function execute(PhutilArgumentParser $args) {
+    $viewer = $this->getViewer();
+
+    $argv = $args->getArg('argv');
+    if (count($argv) !== 2) {
+      throw new PhutilArgumentUsageException(
+        pht('Specify a commit and a revision to attach it to.'));
+    }
+
+    $commit_name = head($argv);
+    $revision_name = last($argv);
+
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withIdentifiers(array($commit_name))
+      ->executeOne();
+    if (!$commit) {
+      throw new PhutilArgumentUsageException(
+        pht('Commit "%s" does not exist.', $commit_name));
+    }
+
+    $revision = id(new PhabricatorObjectQuery())
+      ->setViewer($viewer)
+      ->withNames(array($revision_name))
+      ->executeOne();
+
+    if (!$revision) {
+      throw new PhutilArgumentUsageException(
+        pht('Revision "%s" does not exist.', $revision_name));
+    }
+
+    if (!($revision instanceof DifferentialRevision)) {
+      throw new PhutilArgumentUsageException(
+        pht('Object "%s" must be a Differential revision.', $revision_name));
+    }
+
+    // Reload the revision to get the active diff.
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($revision->getID()))
+      ->needActiveDiffs(true)
+      ->executeOne();
+
+    $differential_phid = id(new PhabricatorDifferentialApplication())
+      ->getPHID();
+
+    $extraction_engine = id(new DifferentialDiffExtractionEngine())
+      ->setViewer($viewer)
+      ->setAuthorPHID($differential_phid);
+
+    $content_source = PhabricatorContentSource::newForSource(
+      PhabricatorContentSource::SOURCE_CONSOLE,
+      array());
+
+    $extraction_engine->updateRevisionWithCommit(
+      $revision,
+      $commit,
+      array(),
+      $content_source);
+
+    echo tsprintf(
+      "%s\n",
+      pht(
+        'Attached "%s" to "%s".',
+        $commit->getMonogram(),
+        $revision->getMonogram()));
+  }
+
+
+}

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -216,50 +216,24 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
               $low_level_query->getRevisionMatchData());
           }
 
-          $diff = id(new DifferentialDiffExtractionEngine())
+          $extraction_engine = id(new DifferentialDiffExtractionEngine())
             ->setViewer($actor)
-            ->setAuthorPHID($acting_as_phid)
+            ->setAuthorPHID($acting_as_phid);
-            ->newDiffFromCommit($commit);
-
-          $vs_diff = $this->loadChangedByCommit($revision, $diff);
-          $changed_uri = null;
-          if ($vs_diff) {
-            $data->setCommitDetail('vsDiff', $vs_diff->getID());
-
-            $changed_uri = PhabricatorEnv::getProductionURI(
-              '/D'.$revision->getID().
-              '?vs='.$vs_diff->getID().
-              '&id='.$diff->getID().
-              '#toc');
-          }
-
-          $xactions = array();
-          $xactions[] = id(new DifferentialTransaction())
-            ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-            ->setIgnoreOnNoEffect(true)
-            ->setNewValue($diff->getPHID())
-            ->setMetadataValue('isCommitUpdate', true);
-          $xactions[] = $commit_close_xaction;
 
           $content_source = PhabricatorContentSource::newForSource(
             PhabricatorContentSource::SOURCE_DAEMON,
             array());
 
-          $editor = id(new DifferentialTransactionEditor())
+          $update_data = $extraction_engine->updateRevisionWithCommit(
-            ->setActor($actor)
+            $revision,
-            ->setActingAsPHID($acting_as_phid)
+            $commit,
-            ->setContinueOnMissingFields(true)
+            array(
-            ->setContentSource($content_source)
+              $commit_close_xaction,
-            ->setChangedPriorToCommitURI($changed_uri)
+            ),
-            ->setIsCloseByCommit(true);
+            $content_source);
 
-          try {
+          foreach ($update_data as $key => $value) {
-            $editor->applyTransactions($revision, $xactions);
+            $data->setCommitDetail($key, $value);
-          } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
-            // NOTE: We've marked transactions other than the CLOSE transaction
-            // as ignored when they don't have an effect, so this means that we
-            // lost a race to close the revision. That's perfectly fine, we can
-            // just continue normally.
           }
         }
       }
@@ -280,112 +254,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
   }
 
-
-  private function loadChangedByCommit(
-    DifferentialRevision $revision,
-    DifferentialDiff $diff) {
-
-    $repository = $this->repository;
-
-    $vs_diff = id(new DifferentialDiffQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withRevisionIDs(array($revision->getID()))
-      ->needChangesets(true)
-      ->setLimit(1)
-      ->executeOne();
-    if (!$vs_diff) {
-      return null;
-    }
-
-    if ($vs_diff->getCreationMethod() == 'commit') {
-      return null;
-    }
-
-    $vs_changesets = array();
-    foreach ($vs_diff->getChangesets() as $changeset) {
-      $path = $changeset->getAbsoluteRepositoryPath($repository, $vs_diff);
-      $path = ltrim($path, '/');
-      $vs_changesets[$path] = $changeset;
-    }
-
-    $changesets = array();
-    foreach ($diff->getChangesets() as $changeset) {
-      $path = $changeset->getAbsoluteRepositoryPath($repository, $diff);
-      $path = ltrim($path, '/');
-      $changesets[$path] = $changeset;
-    }
-
-    if (array_fill_keys(array_keys($changesets), true) !=
-        array_fill_keys(array_keys($vs_changesets), true)) {
-      return $vs_diff;
-    }
-
-    $file_phids = array();
-    foreach ($vs_changesets as $changeset) {
-      $metadata = $changeset->getMetadata();
-      $file_phid = idx($metadata, 'new:binary-phid');
-      if ($file_phid) {
-        $file_phids[$file_phid] = $file_phid;
-      }
-    }
-
-    $files = array();
-    if ($file_phids) {
-      $files = id(new PhabricatorFileQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withPHIDs($file_phids)
-        ->execute();
-      $files = mpull($files, null, 'getPHID');
-    }
-
-    foreach ($changesets as $path => $changeset) {
-      $vs_changeset = $vs_changesets[$path];
-
-      $file_phid = idx($vs_changeset->getMetadata(), 'new:binary-phid');
-      if ($file_phid) {
-        if (!isset($files[$file_phid])) {
-          return $vs_diff;
-        }
-        $drequest = DiffusionRequest::newFromDictionary(array(
-          'user' => PhabricatorUser::getOmnipotentUser(),
-          'repository' => $this->repository,
-          'commit' => $this->commit->getCommitIdentifier(),
-          'path' => $path,
-        ));
-        $corpus = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
-          ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->loadFileContent()
-          ->getCorpus();
-        if ($files[$file_phid]->loadFileData() != $corpus) {
-          return $vs_diff;
-        }
-      } else {
-        $context = implode("\n", $changeset->makeChangesWithContext());
-        $vs_context = implode("\n", $vs_changeset->makeChangesWithContext());
-
-        // We couldn't just compare $context and $vs_context because following
-        // diffs will be considered different:
-        //
-        //   -(empty line)
-        //   -echo 'test';
-        //    (empty line)
-        //
-        //    (empty line)
-        //   -echo "test";
-        //   -(empty line)
-
-        $hunk = id(new DifferentialModernHunk())->setChanges($context);
-        $vs_hunk = id(new DifferentialModernHunk())->setChanges($vs_context);
-        if ($hunk->makeOldFile() != $vs_hunk->makeOldFile() ||
-            $hunk->makeNewFile() != $vs_hunk->makeNewFile()) {
-          return $vs_diff;
-        }
-      }
-    }
-
-    return null;
-  }
-
   private function resolveUserPHID(
     PhabricatorRepositoryCommit $commit,
     $user_name) {