Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13975024
D8441.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
11 KB
Referenced Files
None
Subscribers
None
D8441.diff
View Options
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);
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Oct 19 2024, 8:22 AM (4 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6719967
Default Alt Text
D8441.diff (11 KB)
Attached To
Mode
D8441: Use TransactionEditor when closing revisions in response to commits
Attached
Detach File
Event Timeline
Log In to Comment