Write, Review, Merge, Publish: Phabricator Review Workflow

Users coming to Phabricator from other software (like GitHub or Gerrit) are sometimes tripped up by the sequencing of Phabricator's default review workflow in Differential.

In some other tools, changes generally go through a write, publish, review, merge workflow. First, you write some changes locally. Next, you publish them by pushing them into a remote. They're reviewed (usually in a web UI) and, once accepted, merged into a production branch. In these flows, the changes that are merged are exactly the same as the changes that were published.

Phabricator's workflow is slightly different, and the default sequence is write, review, merge, publish. As above, you write some changes locally first, but then Phabricator's workflow diverges.

Phabricator philosophically considers review to be an important step in the development workflow, and for unreviewed changes to be unpublishable by default.

Conceptually, unreviewed changes don't count yet: they are transient, ephemeral, and totally mutable. The change may be wrong, or the approach may be wrong, there may be missing context, the change may be solving the wrong problem, and so on. Until changes survive review, and we want authors and reviewers to think of a proposed change as a starting point, not a finished product, and for all parties to enter the process with an expectation that changes will frequently undergo substantial revision.

There isn't much practical technical difference between this flow and other flows like the GitHub workflow, but there often is a practical social difference: when changes must overcome the activation energy of review to move forward and are mutable and unpublished until they do, authors are generally put in a position where there's a stronger social expectation that they'll make adjustments in response to feedback. Metaphorically, reviewers are providing feedback on a rough sketch, not critiquing a finished work which has already been framed and hung.

The Phabricator workflow and various product decisions within Differential are trying to reinforce this expectation of unreviewed code as a mutable draft with no persistence or particular value until it passes review.

The second step in the default Phabricator flow is review, which happens on unpublished changes. Unpublished changes are sent for review, usually with arc diff, and reviewers provide feedback. The author responds to review, makes updates or revisions, rewrites things entirely to solve the problem in a different way, splits the change apart, does a different change in different codebase, throws the proposal away completely, or whatever else represents the best solution to the problem. We want authors to feel like they're operating on unpublished code and freely rebase, reorder, remove, divide or abandon it. Throughout this process, there's no default published state that reviewers are pushing the author to deviate from or that the author is trying to tweak into becoming acceptable by appending tiny changes on top of.

In review, authors sometimes feel like they're being attacked when reviewers have feedback. This can go the other way too, where engaging as a reviewer feels like fighting the author rather than collaborating with them to solve a problem. In either case, review will be much less effective. For review to work well, it's important that authors be flexible and open to accommodating feedback.

A workflow that emphasizes the transient nature of unreviewed code can help with this by softening the emotional impact of negative feedback for authors and make discussion that leads to a major response, like discarding a change completely and pursuing a new direction instead, feel rewarding and productive instead of adversarial. By positioning review as a pre-publish step and framing unreviewed code as mutable and temporary, we're trying to make the social and emotional environment which review occurs in supportive of review that produces real changes.

Once a change survives review, it is merged and then published (usually, these both happen in a single step with arc land).

Here, Phabricator also deviates from tools that publish first: by default, it discards the path that was taken to arrive at the final change and squashes the entire change into a single commit. In general, this means that it discards checkpoint commits, rebases, squash-merges, and publishes the entire change as a single fast-forward commit on the target branch.

Phabricator does this partly because it can: nothing has been published yet, so the workflow is free to publish the change in whatever form it considers most-preferred. It could choose to retain all the checkpoint commits, but it also has the freedom to do something else.

Given this freedom, we consider linearized, rebased, fast-forward commits to be the most preferred form, and this is the default behavior of Phabricator.

Here's what a repository built with the Phabricator workflow usually looks like (here, Phabricator itself):

phabricator/ $ git log --graph --oneline
* 329d036 Use an extended policy to bind column and board policies together
* 3f50ba9 On workboards, put older milestone columns on the right
* 9bab3c2 Sort milestones by milestone number, not ID
* bec21d8 Don't try to import proxy columns
* 1e3a5bd Filter out archived projects from ProjectProfileView
* de4167d Put boundary spaces around crumb names so double-clicking doesn't flip out
* ca908d7 Don't autoname milestones, but do show the previous milestone name as a hint
* 68d0593 Don't show un-completeable results in people/project autocomplete
* 2f054ed Hide milestone columns when milestone is archived
* a5bbe25 Fix a couple of missing translation strings
...
* 01084bf Begin making card updates on boards independent of context
* 9ed9764 Replace height buffer behavior while dragging on workboards with infinite column height
* bc591b1 Mostly move workboard card moves to new Workboard code
...

Here's what a GitHub repository built with pull requests usually looks like (here, Rails):

rails/ $ git log --oneline --graph
* b635eea use kwargs to avoid hash slicing
*   8c53b41 Merge pull request #23611 from abhishekjain16/routes_options
|\  
| * 4e3931a Fixes routes to match verbs and path with -g option
* | da1fbb9 Add fixes accidentally removed.
* | 354fb73 Flesh out request encoding + response parsing changelog entry.
* |   d50d709 Merge pull request #23642 from tvon/chore/fix-path-omission
|\ \  
| * | 1a61496 Use correct path in documentation.
* | |   db64cba Merge pull request #23641 from abhishekjain16/docs_fix
|\ \ \  
| |/ /  
|/| |   
| * | 9a2ca9c [ci skip] Fix enqueuing spelling to maintain consistency
|/ /  
* |   c63f58d Merge pull request #23639 from Gaurav2728/update_change_log_entry_action_pack
|\ \  
| * | 33e202d use rails instead of rake
|/ /  
...
| |/ / / / / / / /  
|/| | | | | | | |   
| * | | | | | | | 03980b2 Converting backtrace to strings before calling set_backtrace
* | | | | | | | |   2db347b Merge pull request #23395 from PareshGupta/remove-unused-constant
|\ \ \ \ \ \ \ \ \  
...

This difference is fractal: one level deeper, the entire commit message in a Phabricator repository is structured and meaningful and provides context about the change.

Phabricator can be configured to produce repositories like GitHub (almost -- we don't currently have an option to make half of the commit subjects meaningless merges), but we think the former history is the better default by an enormous margin.

Some criticisms of this approach include:

Squashing Discards Information: By default, we throw away any checkpoint commits, and they are never published. This discards some information about the path that was taken to arrive at a change.

My experience is that this information is almost never useful to have published in the repository, and that the original review in Differential is a much better source for this information when it is desired: it retains the actual change history that would be represented in checkpoint commits, it does a better job of capturing the context and rationale for a path in comments and discussion, it can describe the path across totally different changes if an earlier approach is abandoned, and it can even cross repositories.

Particularly, my experience is that the value of this information is much lower than the value of having a linear, readable repository history with human-readable commit subjects, especially at scale.

Sometimes, authors are attached to checkpoint commits for relatively non-technical reasons. But this attachment will reduce the effectiveness of review: if an author is already attached to their changes, the review process will be more adversarial. A workflow which always throws checkpoint commits away can help them feel transient and unworthy of attachment. Put another way, the squash workflow is way more zen.

Backups, Multiple Development Environments, Sharing Code: Some users have use cases where they want to publish changes for a reason unrelated to review: most often they want a backup, they want to work on unreviewed code from multiple development environments, or they want to share a temporary branch that a group of people work on together.

Phabricator allows you to fetch changes with arc patch, which may be suitable. But there's also no reason you can't push changes somewhere in addition to going through the Phabricator flow, although you may have to change some settings.

These are use cases we generally haven't seen much interest in direct support for. At least personally, I don't destroy enough hardware for a backup process that I have to trigger manually to ever pay for itself, and we haven't seen reports of users losing work for lack of these backups (it could generally be recovered with arc patch or from staging areas anyway).

Multiple development environments don't seem to be common (most organizations seem to use laptops or dedicated remote development hosts).

The "a bunch of developers do stuff together and then kind of review it together" flow is so informal and ad-hoc that I think it doesn't really make sense to try to put tooling on top of it. If that's what makes sense for your project, just go with whatever works, man. As above, there's no reason you can't do this alongside the Phabricator review workflow.

Requires a CLI Tool: This ain't great. I think the arc tool adds a great deal of value once you get familiar with it and configure it properly, but it definitely raises the barrier to entry for new installs and users.

While you don't technically need to use arc, there's a huge jump in result quality between "copy-paste diffs into the web UI" and arc. I'd like to smooth this out in the future and offer a pure git approach between "copy/paste" and arc (see T5000), but this is complex and primarily solves an onboarding problem. I expect users to switch to arc once they become comfortable with Phabricator even once we support a pure git flow: we can never put things like interactive lint autopatching on a pure git flow, and that feature is sufficiently valuable on its own to justify the existence of arc.

Written by epriestley on Feb 14 2016, 12:55 AM.
Overengineer
Subscribers
Info-Screen, iamvoid, ablekh, chad
Projects
None
Tokens
"Like" token, awarded by Info-Screen."Orange Medal" token, awarded by 20after4."Like" token, awarded by vinzent."Mountain of Wealth" token, awarded by asherkin."Mountain of Wealth" token, awarded by greggrossmeier.
chad added a comment.Feb 14 2016, 1:57 AM

I need to finish my social sharing diff for Phame

ablekh added a comment.EditedFeb 28 2016, 11:30 AM

The onboarding would be much easier, if Phabricator would offer a clear and detailed single document, describing and explaining the full Phabricator-based development process (along with rationale, advantages and disadvantages at each step). Something similar to this nice post, but more clear and more comprehensive.

In J766#294, @ablekh wrote:

The onboarding would be much easier, if Phabricator would offer a clear and detailed single document, describing and explaining the full Phabricator-based development process (along with rationale, advantages and disadvantages at each step). Something similar to this nice post, but more clear and more comprehensive.

use and choose a tool after evaluating it rather than looking at point by point comparison or look up for other people's experience.

@iamvoid I think that you don't really understand the meaning of the term "onboarding" - it refers to a process that occurs after evaluation. Thus, you comment doesn't make sense, in my opinion. Any quality product or project should have a solid documentation, including parts, covering an onboarding process. The existence of the document above supports my argument.