Write, Review, Merge, Publish: Phabricator Review Workflow

Write, Review, Merge, Publish: Phabricator Review Workflow(secure.phabricator.com)

28 points by Revisor 10 years ago | 7 comments

drewg123 10 years ago |

I recently started using Phabricator fairly heavily for a review of a large patchset, and I am less than impressed. It could be that the project where I'm using Phabricator didn't set up their review system properly, it could be related to them running svn, etc. And it could certainly be due to my having used Gerrit for 2+ years and Phabricator for a few months. That said..

The problem I have with Phabricator can be summed up in saying that it is a review system which is not tightly integrated with the repo, and that approach has inherent problems. If you actually want to apply the patchset under review in order to run it (or even look at it with other tools), arcanist does not seem to be helpful in finding the correct revision of the tree. You have to communicate with the author via side channels to determine what version of the tree they were running when the patch was created. When arc patch blows up, it is not clear how to recover (picture a git merge with no obvious way to resolve conflicts). I finally got a pointer to the author's git repo and cloned it..

I find it a bit disingenuous that the essay first compares Phabricator to Github pull based workflows, and to gerrit. They go on to criticize pull based workflows, but fail to mention that Gerrit is not a pull based workflow, and gives the same advantages of Phabricator (squashed, linear history). Gerrit has the advantage of being tightly integrated with a repo, and being able to easily fetch the author's work so you can look at it in context, test it, etc.

Game_Ender 10 years ago | |

I agree that Differential, the Phabricator code review application, does not handle patch sets with hundreds of files or many thousands of lines of well. They don't optimize for that case much because they believe in smaller changes [0].

I think the local patch problem you are experiencing is due to the way the author submitted their diff. Normally if you want to locally try code you run "arc patch D123" and it will create a branch off that revision and apply the patch for you. If you look at this review [1] you see it has a link to the repository "Repository rP Phabricator", and in the "Revision Update History" and "Local Commits" sections it has direct links to the base and parent commits. If the submitter based their diff off non-published commits this won't work.

You can also reduce the need to directly pull down code by integrating a CI tool, code coverage information, and screenshots into your review process. All of those Phabricator supports.

[0] https://secure.phabricator.com/book/phabflavor/article/recom...

[1] https://secure.phabricator.com/D15358

jlebar 10 years ago | |

I use phabricator for the llvm project (whose canonical repo is still svn), and I also find it pretty frustrating.

One thing I particularly dislike is that (as far as I can tell) there isn't a way to pull metadata down from phab into a git commit. Arcanist puts metadata (e.g. the list of reviewers) into my commit, but then before I actually push the commit, I want to update that list. As far as I can tell, I have to do this by hand, or otherwise write a script, which I may do, but definitely not in php because prejudice, but seriously ugh.

That said, there are bright spots. The tool for getting notified of new patches (herald, not pherald, sadly) is powerful. And phabricator integrates reasonably well with a mailing list.

I wish we would give up and use a pure git-based workflow. The war is over, let's get on with our lives.

techdragon 10 years ago | | |

I'm no php fan but to be fair to the guys at Phacility who drive the development of Phabricator... Their PHP is nothing like the garbage most of us think of when we think "PHP project" it's actually good code written to avoid the pitfalls and dangerous practices PHP makes alluringly easy to do instead of doing it "right"

djtide 10 years ago | | |

We have plans to support a pure git-based workflow. See https://secure.phabricator.com/T5000

brad0 10 years ago |

We use a very similar system at Amazon.

Our code review tool creates a diff and uploads it to a server. Once uploaded you publish the diff and wait for someone to approve it.

Our team squashes and rebases before pushing to master but I know of other teams that leave the history as is.

The only downside of the code review tool is that a large amount of changes is hard to grok. Breaking a single large code review into multiple small reviews generally counteracts that.

It took some getting used to but it's powerful once you understand the development flow.

brudgers 10 years ago |

Phabricator: http://phabricator.org/