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

Drop the "update revision with commit diff" transaction if the revision is already closed

Browse source 
Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.

We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)

To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.

This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.

Test Plan:
  - Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
  - Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
  - After: repeated runs had no effects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13290

Differential Revision: https://secure.phabricator.com/D20545
This commit is contained in:
epriestley 2019-05-22 14:25:03 -07:00
parent 1eff4fdca3
commit b95bf722d5
1 changed files with 20 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
View file

@ -10,6 +10,26 @@ final class DifferentialRevisionUpdateTransaction
return $object->getActiveDiffPHID(); return $object->getActiveDiffPHID();
} }
public function generateNewValue($object, $value) {
// See T13290. If we're updating the revision in response to a commit but
// the revision is already closed, return the old value so we no-op this
// transaction. We don't want to attach more than one commit-diff to a
// revision.
// Although we can try to bail out earlier so we don't generate this
// transaction in the first place, we may race another worker and end up
// trying to apply it anyway. Here, we have a lock on the object and can
// be certain about the object state.
if ($this->isCommitUpdate()) {
if ($object->isClosed()) {
return $this->generateOldValue($object);
}
}
return $value;
}
public function applyInternalEffects($object, $value) { public function applyInternalEffects($object, $value) {
$should_review = $this->shouldRequestReviewAfterUpdate($object); $should_review = $this->shouldRequestReviewAfterUpdate($object);
if ($should_review) { if ($should_review) {