diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1020,6 +1020,7 @@ 'DiffusionURIEditor' => 'applications/diffusion/editor/DiffusionURIEditor.php', 'DiffusionURITestCase' => 'applications/diffusion/request/__tests__/DiffusionURITestCase.php', 'DiffusionUpdateCoverageConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php', + 'DiffusionUpdateObjectAfterCommitWorker' => 'applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php', 'DiffusionView' => 'applications/diffusion/view/DiffusionView.php', 'DivinerArticleAtomizer' => 'applications/diviner/atomizer/DivinerArticleAtomizer.php', 'DivinerAtom' => 'applications/diviner/atom/DivinerAtom.php', @@ -6680,6 +6681,7 @@ 'DiffusionURIEditor' => 'PhabricatorApplicationTransactionEditor', 'DiffusionURITestCase' => 'PhutilTestCase', 'DiffusionUpdateCoverageConduitAPIMethod' => 'DiffusionConduitAPIMethod', + 'DiffusionUpdateObjectAfterCommitWorker' => 'PhabricatorWorker', 'DiffusionView' => 'AphrontView', 'DivinerArticleAtomizer' => 'DivinerAtomizer', 'DivinerAtom' => 'Phobject', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -315,6 +315,7 @@ $editor = id(new DifferentialTransactionEditor()) ->setActor($viewer) ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) ->setContentSource($content_source) ->setChangedPriorToCommitURI($changed_uri) ->setIsCloseByCommit(true); @@ -324,14 +325,7 @@ $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. - } + $editor->applyTransactions($revision, $xactions); } private function loadConcerningBuilds(DifferentialRevision $revision) { diff --git a/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php b/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php new file mode 100644 --- /dev/null +++ b/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php @@ -0,0 +1,214 @@ +getViewer(); + $data = $this->getTaskData(); + + $commit_phid = idx($data, 'commitPHID'); + if (!$commit_phid) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No "commitPHID" in task data.')); + } + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withPHIDs(array($commit_phid)) + ->needIdentities(true) + ->executeOne(); + if (!$commit) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Unable to load commit "%s".', + $commit_phid)); + } + + $object_phid = idx($data, 'objectPHID'); + if (!$object_phid) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No "objectPHID" in task data.')); + } + + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($object_phid)) + ->executeOne(); + if (!$object) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Unable to load object "%s".', + $object_phid)); + } + + $properties = idx($data, 'properties', array()); + $this->properties = $properties; + + if ($object instanceof ManiphestTask) { + $this->updateTask($commit, $object); + } else if ($object instanceof DifferentialRevision) { + $this->updateRevision($commit, $object); + } + } + + protected function getUpdateProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + protected function getActingPHID(PhabricatorRepositoryCommit $commit) { + if ($commit->hasCommitterIdentity()) { + return $commit->getCommitterIdentity()->getIdentityDisplayPHID(); + } + + if ($commit->hasAuthorIdentity()) { + return $commit->getAuthorIdentity()->getIdentityDisplayPHID(); + } + + return id(new PhabricatorDiffusionApplication())->getPHID(); + } + + protected function loadActingUser($acting_phid) { + // If we we were able to identify an author or committer for the commit, we + // try to act as that user when affecting other objects, like tasks marked + // with "Fixes Txxx". + + // This helps to prevent mistakes where a user accidentally writes the + // wrong task IDs and affects tasks they can't see (and thus can't undo the + // status changes for). + + // This is just a guard rail, not a security measure. An attacker can still + // forge another user's identity trivially by forging author or committer + // email addresses. + + // We also let commits with unrecognized authors act on any task to make + // behavior less confusing for new installs, and any user can craft a + // commit with an unrecognized author and committer. + + $viewer = $this->getViewer(); + + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + if (phid_get_type($acting_phid) === $user_type) { + $acting_user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($acting_phid)) + ->executeOne(); + if ($acting_user) { + return $acting_user; + } + } + + return $viewer; + } + + private function updateTask( + PhabricatorRepositoryCommit $commit, + ManiphestTask $task) { + + $acting_phid = $this->getActingPHID($commit); + $acting_user = $this->loadActingUser($acting_phid); + + $commit_phid = $commit->getPHID(); + + $xactions = array(); + + $xactions[] = $this->newEdgeTransaction( + $task, + $commit, + ManiphestTaskHasCommitEdgeType::EDGECONST); + + $status = $this->getUpdateProperty('status'); + if ($status) { + $xactions[] = $task->getApplicationTransactionTemplate() + ->setTransactionType(ManiphestTaskStatusTransaction::TRANSACTIONTYPE) + ->setMetadataValue('commitPHID', $commit_phid) + ->setNewValue($status); + } + + $content_source = $this->newContentSource(); + + $editor = $task->getApplicationTransactionEditor() + ->setActor($acting_user) + ->setActingAsPHID($acting_phid) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->setUnmentionablePHIDMap( + array( + $commit_phid => $commit_phid, + )); + + $editor->applyTransactions($task, $xactions); + } + + private function updateRevision( + PhabricatorRepositoryCommit $commit, + DifferentialRevision $revision) { + + // Reload the revision to get the active diff, which is currently required + // by "updateRevisionWithCommit()". + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($this->getViewer()) + ->withIDs(array($revision->getID())) + ->needActiveDiffs(true) + ->executeOne(); + + $acting_phid = $this->getActingPHID($commit); + $acting_user = $this->loadActingUser($acting_phid); + + $xactions = array(); + + $xactions[] = $this->newEdgeTransaction( + $revision, + $commit, + DiffusionCommitHasRevisionEdgeType::EDGECONST); + + $match_data = $this->getUpdateProperty('revisionMatchData'); + + $type_close = DifferentialRevisionCloseTransaction::TRANSACTIONTYPE; + $xactions[] = $revision->getApplicationTransactionTemplate() + ->setTransactionType($type_close) + ->setNewValue(true) + ->setMetadataValue('isCommitClose', true) + ->setMetadataValue('revisionMatchData', $match_data) + ->setMetadataValue('commitPHID', $commit->getPHID()); + + $extraction_engine = id(new DifferentialDiffExtractionEngine()) + ->setViewer($acting_user) + ->setAuthorPHID($acting_phid); + + $content_source = $this->newContentSource(); + + $extraction_engine->updateRevisionWithCommit( + $revision, + $commit, + $xactions, + $content_source); + } + + private function newEdgeTransaction( + $object, + PhabricatorRepositoryCommit $commit, + $edge_type) { + + $commit_phid = $commit->getPHID(); + + return $object->getApplicationTransactionTemplate() + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $edge_type) + ->setNewValue( + array( + '+' => array( + $commit_phid => $commit_phid, + ), + )); + } + + +} diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php --- a/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php @@ -311,19 +311,22 @@ ); if ($all_from_repo && !$force_local) { - foreach ($classes as $class) { - PhabricatorWorker::scheduleTask( - $class, - $spec, - array( - 'priority' => PhabricatorWorker::PRIORITY_IMPORT, - )); - } + $background = true; } else { - foreach ($classes as $class) { - $worker = newv($class, array($spec)); - $worker->executeTask(); - } + $background = false; + } + + if (!$background) { + PhabricatorWorker::setRunAllTasksInProcess(true); + } + + foreach ($classes as $class) { + PhabricatorWorker::scheduleTask( + $class, + $spec, + array( + 'priority' => PhabricatorWorker::PRIORITY_IMPORT, + )); } $progress->update(1); diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -148,25 +148,6 @@ $author_phid); } - $differential_app = 'PhabricatorDifferentialApplication'; - $revision_id = null; - $low_level_query = null; - if (PhabricatorApplication::isClassInstalled($differential_app)) { - $low_level_query = id(new DiffusionLowLevelCommitFieldsQuery()) - ->setRepository($repository) - ->withCommitRef($ref); - $field_values = $low_level_query->execute(); - $revision_id = idx($field_values, 'revisionID'); - - if (!empty($field_values['reviewedByPHIDs'])) { - $data->setCommitDetail( - 'reviewerPHID', - reset($field_values['reviewedByPHIDs'])); - } - - $data->setCommitDetail('differential.revisionID', $revision_id); - } - if ($author_phid != $commit->getAuthorPHID()) { $commit->setAuthorPHID($author_phid); } @@ -191,92 +172,15 @@ $should_autoclose = $force_autoclose || $repository->shouldAutocloseCommit($commit); - // When updating related objects, we'll act under an omnipotent user to // ensure we can see them, but take actions as either the committer or // author (if we recognize their accounts) or the Diffusion application // (if we do not). - $actor = PhabricatorUser::getOmnipotentUser(); - $acting_as_phid = nonempty( - $committer_phid, - $author_phid, - id(new PhabricatorDiffusionApplication())->getPHID()); - - $acting_user = $this->loadActingUser($actor, $acting_as_phid); - - $conn_w = id(new DifferentialRevision())->establishConnection('w'); - - // NOTE: The `differential_commit` table has a unique ID on `commitPHID`, - // preventing more than one revision from being associated with a commit. - // Generally this is good and desirable, but with the advent of hash - // tracking we may end up in a situation where we match several different - // revisions. We just kind of ignore this and pick one, we might want to - // revisit this and do something differently. (If we match several revisions - // someone probably did something very silly, though.) - - $revision = null; - if ($revision_id) { - $revision_query = id(new DifferentialRevisionQuery()) - ->withIDs(array($revision_id)) - ->setViewer($actor) - ->needReviewers(true) - ->needActiveDiffs(true); - - $revision = $revision_query->executeOne(); - - if ($revision) { - $commit_drev = DiffusionCommitHasRevisionEdgeType::EDGECONST; - id(new PhabricatorEdgeEditor()) - ->addEdge($commit->getPHID(), $commit_drev, $revision->getPHID()) - ->save(); - - $should_close = !$revision->isPublished() && $should_autoclose; - if ($should_close) { - $type_close = DifferentialRevisionCloseTransaction::TRANSACTIONTYPE; - - $commit_close_xaction = id(new DifferentialTransaction()) - ->setTransactionType($type_close) - ->setNewValue(true); - - $commit_close_xaction->setMetadataValue( - 'commitPHID', - $commit->getPHID()); - - if ($low_level_query) { - $commit_close_xaction->setMetadataValue( - 'revisionMatchData', - $low_level_query->getRevisionMatchData()); - $data->setCommitDetail( - 'revisionMatchData', - $low_level_query->getRevisionMatchData()); - } - - $extraction_engine = id(new DifferentialDiffExtractionEngine()) - ->setViewer($actor) - ->setAuthorPHID($acting_as_phid); - - $content_source = $this->newContentSource(); - - $extraction_engine->updateRevisionWithCommit( - $revision, - $commit, - array( - $commit_close_xaction, - ), - $content_source); - } - } - } - if ($should_autoclose) { - $this->closeTasks( - $actor, - $acting_as_phid, - $repository, - $commit, - $message, - $acting_user); + $actor = PhabricatorUser::getOmnipotentUser(); + $this->closeRevisions($actor, $ref, $commit, $data); + $this->closeTasks($actor, $ref, $commit, $data); } $data->save(); @@ -295,28 +199,64 @@ ->execute(); } - private function closeTasks( + private function closeRevisions( PhabricatorUser $actor, - $acting_as, - PhabricatorRepository $repository, + DiffusionCommitRef $ref, PhabricatorRepositoryCommit $commit, - $message, - PhabricatorUser $acting_user = null) { + PhabricatorRepositoryCommitData $data) { - // If we we were able to identify an author for the commit, we try to act - // as that user when loading tasks marked with "Fixes Txxx". This prevents - // mistakes where a user accidentally writes the wrong task IDs and affects - // tasks they can't see (and thus can't undo the status changes for). + $differential = 'PhabricatorDifferentialApplication'; + if (!PhabricatorApplication::isClassInstalled($differential)) { + return; + } + + $repository = $commit->getRepository(); + + $field_query = id(new DiffusionLowLevelCommitFieldsQuery()) + ->setRepository($repository) + ->withCommitRef($ref); + + $field_values = $field_query->execute(); + + $revision_id = idx($field_values, 'revisionID'); + if (!$revision_id) { + return; + } - // This is just a guard rail, not a security measure. An attacker can still - // forge another user's identity trivially by forging author or committer - // emails. We also let commits with unrecognized authors act on any task to - // make behavior less confusing for new installs. + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($actor) + ->withIDs(array($revision_id)) + ->executeOne(); + if (!$revision) { + return; + } - if (!$acting_user) { - $acting_user = $actor; + // NOTE: This is very old code from when revisions had a single reviewer. + // It still powers the "Reviewer (Deprecated)" field in Herald, but should + // be removed. + if (!empty($field_values['reviewedByPHIDs'])) { + $data->setCommitDetail( + 'reviewerPHID', + head($field_values['reviewedByPHIDs'])); } + $match_data = $field_query->getRevisionMatchData(); + + $data->setCommitDetail('differential.revisionID', $revision_id); + $data->setCommitDetail('revisionMatchData', $match_data); + + $properties = array( + 'revisionMatchData' => $match_data, + ); + $this->queueObjectUpdate($commit, $revision, $properties); + } + + private function closeTasks( + PhabricatorUser $actor, + DiffusionCommitRef $ref, + PhabricatorRepositoryCommit $commit, + PhabricatorRepositoryCommitData $data) { + $maniphest = 'PhabricatorManiphestApplication'; if (!PhabricatorApplication::isClassInstalled($maniphest)) { return; @@ -324,11 +264,12 @@ $prefixes = ManiphestTaskStatus::getStatusPrefixMap(); $suffixes = ManiphestTaskStatus::getStatusSuffixMap(); + $message = $data->getCommitMessage(); $matches = id(new ManiphestCustomFieldStatusParser()) ->parseCorpus($message); - $task_statuses = array(); + $task_map = array(); foreach ($matches as $match) { $prefix = phutil_utf8_strtolower($match['prefix']); $suffix = phutil_utf8_strtolower($match['suffix']); @@ -340,89 +281,44 @@ foreach ($match['monograms'] as $task_monogram) { $task_id = (int)trim($task_monogram, 'tT'); - $task_statuses[$task_id] = $status; + $task_map[$task_id] = $status; } } - if (!$task_statuses) { + if (!$task_map) { return; } $tasks = id(new ManiphestTaskQuery()) - ->setViewer($acting_user) - ->withIDs(array_keys($task_statuses)) - ->needProjectPHIDs(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) + ->setViewer($actor) + ->withIDs(array_keys($task_map)) ->execute(); - foreach ($tasks as $task_id => $task) { - $xactions = array(); - - $edge_type = ManiphestTaskHasCommitEdgeType::EDGECONST; - $edge_xaction = id(new ManiphestTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) - ->setMetadataValue('edge:type', $edge_type) - ->setNewValue( - array( - '+' => array( - $commit->getPHID() => $commit->getPHID(), - ), - )); - - $status = $task_statuses[$task_id]; - if ($status) { - if ($task->getStatus() != $status) { - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType( - ManiphestTaskStatusTransaction::TRANSACTIONTYPE) - ->setMetadataValue('commitPHID', $commit->getPHID()) - ->setNewValue($status); - - $edge_xaction->setMetadataValue('commitPHID', $commit->getPHID()); - } - } - - $xactions[] = $edge_xaction; - - $content_source = $this->newContentSource(); + $status = $task_map[$task_id]; - $editor = id(new ManiphestTransactionEditor()) - ->setActor($actor) - ->setActingAsPHID($acting_as) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true) - ->setUnmentionablePHIDMap( - array($commit->getPHID() => $commit->getPHID())) - ->setContentSource($content_source); + $properties = array( + 'status' => $status, + ); - $editor->applyTransactions($task, $xactions); + $this->queueObjectUpdate($commit, $task, $properties); } } - private function loadActingUser(PhabricatorUser $viewer, $user_phid) { - if (!$user_phid) { - return null; - } - - $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; - if (phid_get_type($user_phid) != $user_type) { - return null; - } - - $user = id(new PhabricatorPeopleQuery()) - ->setViewer($viewer) - ->withPHIDs(array($user_phid)) - ->executeOne(); - if (!$user) { - return null; - } - - return $user; + private function queueObjectUpdate( + PhabricatorRepositoryCommit $commit, + $object, + array $properties) { + + $this->queueTask( + 'DiffusionUpdateObjectAfterCommitWorker', + array( + 'commitPHID' => $commit->getPHID(), + 'objectPHID' => $object->getPHID(), + 'properties' => $properties, + ), + array( + 'priority' => PhabricatorWorker::PRIORITY_DEFAULT, + )); } - }