revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-19 13:22:42 +01:00
Code Issues Releases Wiki Activity

Guarantee that fields copied from diffs persist on revisions

Browse source 
Summary:
Fixes T4636. Currently, we copy fields from the diff to the revision during the external effect phase, but there's no guarantee that we persist the object after this phase.

(In practice, when Herald rules trigger they cause the object to persist on this install, which is why we don't see this issue.)

Instead, move the field copies to the internal phase, where persistence is guaranteed.

Also consolidate some of the diff loading.

Test Plan: Ran `arc diff`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: mbishopim3, epriestley

Maniphest Tasks: T4636

Differential Revision: https://secure.phabricator.com/D8610
This commit is contained in:
epriestley 2014-03-25 14:01:38 -07:00
parent 150a3adf2c
commit 2547a222d8
1 changed files with 26 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -183,6 +183,13 @@ final class DifferentialTransactionEditor
($object->getStatus() == $status_plan))) { ($object->getStatus() == $status_plan))) {
$object->setStatus($status_review); $object->setStatus($status_review);
} }
$diff = $this->requireDiff($xaction->getNewValue());
$object->setLineCount($diff->getLineCount());
$object->setRepositoryPHID($diff->getRepositoryPHID());
$object->setArcanistProjectPHID($diff->getArcanistProjectPHID());
// TODO: Update the `diffPHID` once we add that. // TODO: Update the `diffPHID` once we add that.
return; return;
case DifferentialTransaction::TYPE_ACTION: case DifferentialTransaction::TYPE_ACTION:
@ -304,7 +311,7 @@ final class DifferentialTransactionEditor
$maniphest = 'PhabricatorApplicationManiphest'; $maniphest = 'PhabricatorApplicationManiphest';
if (PhabricatorApplication::isClassInstalled($maniphest)) { if (PhabricatorApplication::isClassInstalled($maniphest)) {
$diff = $this->loadDiff($xaction->getNewValue()); $diff = $this->requireDiff($xaction->getNewValue());
$branch = $diff->getBranch(); $branch = $diff->getBranch();
// No "$", to allow for branches like T123_demo. // No "$", to allow for branches like T123_demo.
@ -475,15 +482,12 @@ final class DifferentialTransactionEditor
return; return;
case DifferentialTransaction::TYPE_UPDATE: case DifferentialTransaction::TYPE_UPDATE:
// Now that we're inside the transaction, do a final check. // Now that we're inside the transaction, do a final check.
$diff = $this->loadDiff($xaction->getNewValue()); $diff = $this->requireDiff($xaction->getNewValue());
// TODO: It would be slightly cleaner to just revalidate this // TODO: It would be slightly cleaner to just revalidate this
// transaction somehow using the same validation code, but that's // transaction somehow using the same validation code, but that's
// not easy to do at the moment. // not easy to do at the moment.
if (!$diff) {
throw new Exception(pht('Diff does not exist!'));
} else {
$revision_id = $diff->getRevisionID(); $revision_id = $diff->getRevisionID();
if ($revision_id && ($revision_id != $object->getID())) { if ($revision_id && ($revision_id != $object->getID())) {
throw new Exception( throw new Exception(
@ -491,15 +495,9 @@ final class DifferentialTransactionEditor
'Diff is already attached to another revision. You lost '. 'Diff is already attached to another revision. You lost '.
'a race?')); 'a race?'));
} }
}
$diff->setRevisionID($object->getID()); $diff->setRevisionID($object->getID());
$diff->save(); $diff->save();
$object->setLineCount($diff->getLineCount());
$object->setRepositoryPHID($diff->getRepositoryPHID());
$object->setArcanistProjectPHID($diff->getArcanistProjectPHID());
return; return;
} }
@ -560,7 +558,7 @@ final class DifferentialTransactionEditor
foreach ($xactions as $xaction) { foreach ($xactions as $xaction) {
switch ($xaction->getTransactionType()) { switch ($xaction->getTransactionType()) {
case DifferentialTransaction::TYPE_UPDATE: case DifferentialTransaction::TYPE_UPDATE:
$diff = $this->loadDiff($xaction->getNewValue(), true); $diff = $this->requireDiff($xaction->getNewValue(), true);
// Update these denormalized index tables when we attach a new // Update these denormalized index tables when we attach a new
// diff to a revision. // diff to a revision.
@ -1122,7 +1120,7 @@ final class DifferentialTransactionEditor
} }
if ($update_xaction) { if ($update_xaction) {
$diff = $this->loadDiff($update_xaction->getNewValue(), true); $diff = $this->requireDiff($update_xaction->getNewValue(), true);
$body->addTextSection( $body->addTextSection(
pht('AFFECTED FILES'), pht('AFFECTED FILES'),
@ -1324,6 +1322,15 @@ final class DifferentialTransactionEditor
return $query->executeOne(); return $query->executeOne();
} }
private function requireDiff($phid, $need_changesets = false) {
$diff = $this->loadDiff($phid, $need_changesets);
if (!$diff) {
throw new Exception(pht('Diff "%s" does not exist!', $phid));
}
return $diff;
}
/* -( Herald Integration )------------------------------------------------- */ /* -( Herald Integration )------------------------------------------------- */
protected function shouldApplyHeraldRules( protected function shouldApplyHeraldRules(