Page MenuHomePhabricator

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

Authored by epriestley on May 22 2019, 9:31 PM.

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.