From b95bf722d55e66ca0dcbfe8d1bd7c416cf5528e5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 22 May 2019 14:25:03 -0700
Subject: [PATCH] Drop the "update revision with commit diff" transaction if
 the revision is already closed

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
---
 .../DifferentialRevisionUpdateTransaction.php | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
index 38742357de..4ca03e37e2 100644
--- a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
@@ -10,6 +10,26 @@ final class DifferentialRevisionUpdateTransaction
     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) {
     $should_review = $this->shouldRequestReviewAfterUpdate($object);
     if ($should_review) {