Page MenuHomePhabricator

"Close Revision" actions are executed by the Message worker, and do not execute if a previously discovered commit becomes reachable from a permanent branch
Open, NormalPublic

Description

See PHI1804. Previously, see T13284, which has a more detailed discussion of the general case of this behavior. Broadly, the setup here is:

  • commit C is pushed to non-permanent branch "epriestley-tmp-123";
  • later, commit C is pushed to permanent branch "master".

After T13284, we correctly reprocess "C" when it becomes reachable. However, the reprocessing effectively skips the "Message" step, which has some side effects beyond parsing the message -- notably, closing related revisions and tasks.

This means that revisions related to C are not closed.

The cleanest fix is to move these actions to "Publish", since they're really publish actions anyway and other publishing stuff already happens there.

The major issue I see with this is that these actions currently depend on a DiffusionCommitRef object, which is available in the "Message" step but not in the "Publish" step. These actions also have some side effects that impact commit data and are not "pure" publish actions.

Event Timeline

epriestley triaged this task as Normal priority.Thu, Jul 9, 10:13 PM
epriestley created this task.

A second issue is that some of the "publish" behavior depends on the commit/revision relationship. This can still be sequenced correctly entirely in the publish daemon, but "close related revisions" and "publish the commit" are not entirely independent.

The broader structural issue here is:

  • Only the Message daemon currently builds a DiffusionCommitRef. This object is expensive to build (requires a low-level repository call) and it's desirable to build it only once (and not rebuild it in the Publish daemon).
  • The DiffusionCommitRef currently has unique information which is not preserved on the commit object or commit data object, notably the commit hashes. In Git, this is significant because it is the only source of the tree hash.
  • The code which finds a revision for a given commit currently depends on being passed a DiffusionCommitRef object (which it uses to do hash queries).

So the "find a corresponding revision" code currently must live in the Message daemon.

I'd like to generally make this adjustment:

  • In the Message daemon, store the DiffusionCommitRef on the commit data object.
  • Later, reconstruct it from the commit data.

This is largely simple enough, but the CommitData object is a huge mess right now. A naive implementation of "save/load the CommitRef" would possibly put the 5th or 6th copy of some pieces of data onto the Commit/CommitData pair. I'd like to walk this object back into the realm of reason before making it more complex.


A simpler approach would be to keep the "find a revision" code in the Message daemon and only move the "close the revision" code to the Publish daemon. However, "find a revision" is really a publishing action (commits that are unpublished do not associated with a revision) so I'm less fond of this approach.


A cross-cutting issue here is that none of these objects support non-UTF8 data. For now, I'm not going to try to deal with that, and continue forcing everything to UTF-8. The original CommitRef is built over Conduit anyway so this has a complex dependency on T13337.