Page MenuHomePhabricator

Formalize "reverts" and "resurrects" between revisions
Open, NormalPublic

Description

See T1751. Currently, revisions support a formal "parent/child" relationship ("Dxxx depends on Dyyy") but no other relationships.

Some other relationships we see in the wild include:

  • Reverts: Dyyy reverts Dxxx, undoing the changes it made.
  • Resurrects: Dxxx lands, then is later reverted by Dyyy. Later, the original author tries again with Dzzz.
  • Fixes: Dxxx lands, then Dyyy fixes a bug it introduced.

There's probably at least some value in formalizing these relationships (and maybe others), but adding support also increases complexity.

For some vaguely similar prior work, see T10034, which discusses some similar requests for Maniphest Tasks. In various requests, users proposed several new types of task relationships (including strict tree subtasks, related / see also, follower / follows). We generally focused on making the existing relationships more useful rather than adding new types of relationships (for example, "Related" is very similar to "Mentions") and this seems to have worked well.

I believe revisions with these relationships are fairly rare (from T1751, I calculated reverts in phabricator/ at 0.3% of commits) and there isn't all that much we can do with them automatically beyond surfacing them to users to make navigation easier, so I want to take a similarly cautious approach here.

See PHI331. An instance anticipates making some workflow changes which will result in more frequent revert/resurrect relationships.

With some frequency, users want to perform a resurrection by reopening the original review. For example:

  • Dxxx makes a change.
  • Dyyy reverts it.
  • Later, the author of Dxxx wants to try again. Their inclination is to reopen Dxxx rather than creating a new Dzzz.

I think this is generally bad and not something we want to support or encourage. It makes interactions with every connected system (like builds, reviewer acceptance rules, Herald) substantially more complex and unpredictable, and affects <1% of changes.

To provide some examples offhand, suppose Herald added alice when Dxxx was created, then she resigned. Should a hypothetical "resurrect" action add her again? Suppose bailey has a "send me one email" Herald rule. Should the rule be reset? Should a one-time build be reset? How should "Accepts" be reset? What about after T731? Should "Fixes Txxx" be reset? If the author specifies Reviewers: dog, should that reset the reviewers or add to the reviewers? Or should arc diff have a completely different workflow for resurrection? I think most of these questions don't have a simple obvious answer, and that users won't reasonably be able to predict what this operation does in many of these cases, and that the default behavior (without any special logic to deal with reopens) will be problematic in many cases.

If the author opens a new Dzzz, everyone will be able to predict how all the related systems will work and they'll generally all work correctly. Plus, this also accommodates multiple followups gracefully, while reopening the revision forever doesn't.

So I'm inclined to encourage "open a new revision", not "reopen the existing revision" as the fundamental behavior of the system, and then go from there. I think the biggest problem with this is just discovery: since some users are inclined to try to reopen (and the problems that "reopen" creates aren't inherently obvious) they may not discover that the "resurrect" flow exists.

Discovery is an issue here in general. We can make things more discoverable by adding explicit UI links in Edit Related Revisions..., but I'm hesitant to shove that full of links for features which affect <1% of changes.

We can use magic words (see T5132) but they have a major discoverability issue.

A possible approach here is T11934 ("/subscribe" style commands in all comment areas). We can provide !resurrect and the command list can list it so there's at least some way to figure out out. This is a very soft way to surface the feature, but use cases are rare anyway.


My current plan is to ease into this by making these changes:

  • Formalize Dxxx reverts Dyyy by recognizing "Reverts <revision or hash>". We recognize and parse this today, but only <hash>, and only write the edge in Diffusion.
  • Formalize Dxxx resurrects Dyyy by recognizing "Resurrects <revision or hash>", or some similar term.

If these get traction we can expand on this by making these edges more useful. If it doesn't, we haven't added too much complexity.