[cfe-dev] Expect new Phabricator code review test runs

Manuel Klimek klimek at google.com
Thu Aug 16 00:30:56 PDT 2012


Hi,

last month we tried Phabricator for code reviews and got some great
feedback on what's missing. The main issue was that email
notifications were really bad. We've worked on that, and would like to
run some more tests by using it and gather some more feedback from the
community.

So, if you see problems with Phabricator emails on cfe-commits, feel
free to send angry emails with feature requests my way :)

New features:
1. Stripped down email content to the bare minimum:
We tried to make it look as much like what normal patch emails look
like as possible.
2. Added unified diff context for comment mails:
In Phabricator, you can comment on ranges in the code by
click-and-dragging over the line numbers in the review UI; phabricator
will now include a unified diff of the range you selected (or the line
you clicked on) in the comment mail. Unified diffs will span the range
+-1 line.
3. Added comment history in comment mails:
For each comment in the current mail, we include the history of
comments made in that line / range in the comment mail, so it's easy
to follow comment conversations made in phabricator from the mailing
list.

Missing features:
- replying via mail to add inline comments does not work
- phabricator does not automatically detect the context of a file (from the vcs)
- post-commit review is missing the features we added for the
pre-commit workflow

If phabricator gets approved for wider use, I'll write up a more
detailed document with the common workflows for clang/llvm. Until
then, I'd like to keep the number of reviews done via phabrictor
small, so that we can stop at any time if we discover critical bugs.

If you see phabricator reviews going by, we'd appreciate if you'd take
a moment to look whether there are still problems we need to resolve
before allowing use of phabricator more widely. Also, feel free to
click the link to phabricator and join the review :)

Cheers,
/Manuel



More information about the cfe-dev mailing list