Index: src/applications/differential/editor/DifferentialTransactionEditor.php =================================================================== --- src/applications/differential/editor/DifferentialTransactionEditor.php +++ src/applications/differential/editor/DifferentialTransactionEditor.php @@ -4,6 +4,26 @@ extends PhabricatorApplicationTransactionEditor { private $heraldEmailPHIDs; + private $changedPriorToCommitURI; + private $isCloseByCommit; + + public function setIsCloseByCommit($is_close_by_commit) { + $this->isCloseByCommit = $is_close_by_commit; + return $this; + } + + public function getIsCloseByCommit() { + return $this->isCloseByCommit; + } + + public function setChangedPriorToCommitURI($uri) { + $this->changedPriorToCommitURI = $uri; + return $this; + } + + public function getChangedPriorToCommitURI() { + return $this->changedPriorToCommitURI; + } public function getTransactionTypes() { $types = parent::getTransactionTypes(); @@ -158,7 +178,9 @@ case PhabricatorTransactions::TYPE_EDGE: return; case DifferentialTransaction::TYPE_UPDATE: - $object->setStatus($status_review); + if (!$this->getIsCloseByCommit()) { + $object->setStatus($status_review); + } // TODO: Update the `diffPHID` once we add that. return; case DifferentialTransaction::TYPE_ACTION: @@ -209,6 +231,12 @@ $results = parent::expandTransaction($object, $xaction); switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_UPDATE: + if ($this->getIsCloseByCommit()) { + // Don't bother with any of this if this update is a side effect of + // commit detection. + break; + } + $new_accept = DifferentialReviewerStatus::STATUS_ACCEPTED; $new_reject = DifferentialReviewerStatus::STATUS_REJECTED; $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER; @@ -784,19 +812,22 @@ break; case DifferentialAction::ACTION_CLOSE: + // We force revisions closed when we discover a corresponding commit. + // In this case, revisions are allowed to transition to closed from + // any state. This is an automated action taken by the daemons. - // TODO: Permit the daemons to take this action in all cases. - - if (!$actor_is_author && !$always_allow_close) { - return pht( - "You can not close this revision because you do not own it. To ". - "close a revision, you must be its owner."); - } + if (!$this->getIsCloseByCommit()) { + if (!$actor_is_author && !$always_allow_close) { + return pht( + "You can not close this revision because you do not own it. To ". + "close a revision, you must be its owner."); + } - if ($revision_status != $status_accepted) { - return pht( - "You can not close this revision because it has not been ". - "accepted. You can only close accepted revisions."); + if ($revision_status != $status_accepted) { + return pht( + "You can not close this revision because it has not been ". + "accepted. You can only close accepted revisions."); + } } break; } @@ -909,6 +940,13 @@ } } + $changed_uri = $this->getChangedPriorToCommitURI(); + if ($changed_uri) { + $body->addTextSection( + pht('CHANGED PRIOR TO COMMIT'), + $changed_uri); + } + if ($inlines) { $body->addTextSection( pht('INLINE COMMENTS'), @@ -1099,7 +1137,9 @@ foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_UPDATE: - return true; + if (!$this->getIsCloseByCommit()) { + return true; + } } } Index: src/applications/metamta/contentsource/PhabricatorContentSource.php =================================================================== --- src/applications/metamta/contentsource/PhabricatorContentSource.php +++ src/applications/metamta/contentsource/PhabricatorContentSource.php @@ -12,6 +12,7 @@ const SOURCE_CONSOLE = 'console'; const SOURCE_HERALD = 'herald'; const SOURCE_LEGACY = 'legacy'; + const SOURCE_DAEMON = 'daemon'; private $source; private $params = array(); @@ -72,6 +73,7 @@ self::SOURCE_CONSOLE => pht('Console'), self::SOURCE_LEGACY => pht('Legacy'), self::SOURCE_HERALD => pht('Herald'), + self::SOURCE_DAEMON => pht('Daemons'), self::SOURCE_UNKNOWN => pht('Old World'), ); } Index: src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php =================================================================== --- src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -78,16 +78,18 @@ $should_autoclose = $repository->shouldAutocloseCommit($commit, $data); if ($revision_id) { - $lock = PhabricatorGlobalLock::newLock(get_class($this).':'.$revision_id); - $lock->lock(5 * 60); - // TODO: Check if a more restrictive viewer could be set here - $revision = id(new DifferentialRevisionQuery()) + $revision_query = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->needRelationships(true) ->needReviewerStatus(true) - ->executeOne(); + ->needActiveDiffs(true); + + // TODO: Remove this once we swap to new CustomFields. This is only + // required by the old FieldSpecifications, lower on in this worker. + $revision_query->needRelationships(true); + + $revision = $revision_query->executeOne(); if ($revision) { $commit_drev = PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV; @@ -112,29 +114,10 @@ $committer_phid, $author_phid, $revision->getAuthorPHID()); + $actor = id(new PhabricatorUser()) ->loadOneWhere('phid = %s', $actor_phid); - $diff = $this->attachToRevision($revision, $actor_phid); - - $editor = new DifferentialCommentEditor( - $revision, - DifferentialAction::ACTION_CLOSE); - $editor->setActor($actor); - $editor->setIsDaemonWorkflow(true); - - $vs_diff = $this->loadChangedByCommit($diff); - if ($vs_diff) { - $data->setCommitDetail('vsDiff', $vs_diff->getID()); - - $changed_by_commit = PhabricatorEnv::getProductionURI( - '/D'.$revision->getID(). - '?vs='.$vs_diff->getID(). - '&id='.$diff->getID(). - '#toc'); - $editor->setChangedByCommit($changed_by_commit); - } - $commit_name = $repository->formatCommitName( $commit->getCommitIdentifier()); @@ -148,24 +131,77 @@ $data->getAuthorName(), $actor); - $info = array(); - $info[] = "authored by {$author_name}"; if ($committer_name && ($committer_name != $author_name)) { - $info[] = "committed by {$committer_name}"; + $message = pht( + 'Closed by commit %s (authored by %s, committed by %s).', + $commit_name, + $author_name, + $committer_name); + } else { + $message = pht( + 'Closed by commit %s (authored by %s).', + $commit_name, + $author_name); } - $info = implode(', ', $info); - $editor - ->setMessage("Closed by commit {$commit_name} ({$info}).") - ->save(); - } + $diff = $this->generateFinalDiff($revision, $actor_phid); - } + $vs_diff = $this->loadChangedByCommit($revision, $diff); + $changed_uri = null; + if ($vs_diff) { + $data->setCommitDetail('vsDiff', $vs_diff->getID()); - $lock->unlock(); + $changed_uri = PhabricatorEnv::getProductionURI( + '/D'.$revision->getID(). + '?vs='.$vs_diff->getID(). + '&id='.$diff->getID(). + '#toc'); + } + + $xactions = array(); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(DifferentialTransaction::TYPE_ACTION) + ->setNewValue(DifferentialAction::ACTION_CLOSE); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setIgnoreOnNoEffect(true) + ->setNewValue($diff->getPHID()); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->setIgnoreOnNoEffect(true) + ->attachComment( + id(new DifferentialTransactionComment()) + ->setContent($message)); + + $content_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_DAEMON, + array()); + + $editor = id(new DifferentialTransactionEditor()) + ->setActor($actor) + ->setContinueOnMissingFields(true) + ->setContentSource($content_source) + ->setChangedPriorToCommitURI($changed_uri) + ->setIsCloseByCommit(true); + + 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. + } + } + } } if ($should_autoclose) { + // TODO: When this is moved to CustomFields, remove the additional + // call above in query construction. $fields = DifferentialFieldSelector::newSelector() ->getFieldSpecifications(); foreach ($fields as $key => $field) { @@ -200,7 +236,7 @@ return '@'.$handle->getName(); } - private function attachToRevision( + private function generateFinalDiff( DifferentialRevision $revision, $actor_phid) { @@ -232,7 +268,7 @@ } $diff = DifferentialDiff::newFromRawChanges($changes) - ->setRevisionID($revision->getID()) + ->setRepositoryPHID($this->repository->getPHID()) ->setAuthorPHID($actor_phid) ->setCreationMethod('commit') ->setSourceControlSystem($this->repository->getVersionControlSystem()) @@ -265,18 +301,19 @@ // TODO: Attach binary files. - $revision->setLineCount($diff->getLineCount()); - return $diff->save(); } - private function loadChangedByCommit(DifferentialDiff $diff) { + private function loadChangedByCommit( + DifferentialRevision $revision, + DifferentialDiff $diff) { + $repository = $this->repository; $vs_changesets = array(); $vs_diff = id(new DifferentialDiff())->loadOneWhere( 'revisionID = %d AND creationMethod != %s ORDER BY id DESC LIMIT 1', - $diff->getRevisionID(), + $revision->getID(), 'commit'); foreach ($vs_diff->loadChangesets() as $changeset) { $path = $changeset->getAbsoluteRepositoryPath($repository, $vs_diff);