Expect new Phabricator code review test runs

[Manuel Klimek]

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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Konstantin Tokarev]

16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
> 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.

I'm a bit surprised that you being @google.com don't promote Gerrit [1].

[1] http://code.google.com/p/gerrit/

--
Regards,
Konstantin
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Manuel Klimek]

On Thu, Aug 16, 2012 at 2:14 PM, Konstantin Tokarev <[hidden email]> wrote:

> 16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
>> 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.
>
> I'm a bit surprised that you being @google.com don't promote Gerrit [1].

We want to use what fits the project best. Gerrit is a great tool for
a lot of use cases, unfortunately it doesn't fit the clang/llvm
workflow well. Of course I looked into Gerrit / Rietveld etc, but all
of them have serious downsides that seem a lot harder to fix.
Phabricator is the first one that actually looks like it comes close
to what clang/llvm would need (and even there we're not sure yet).

Cheers,
/Manuel
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Thiago Farina]

On Thu, Aug 16, 2012 at 9:35 AM, Manuel Klimek <[hidden email]> wrote:

> On Thu, Aug 16, 2012 at 2:14 PM, Konstantin Tokarev <[hidden email]> wrote:
>> 16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
>>> 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.
>>
>> I'm a bit surprised that you being @google.com don't promote Gerrit [1].
>
> We want to use what fits the project best. Gerrit is a great tool for
> a lot of use cases, unfortunately it doesn't fit the clang/llvm
> workflow well. Of course I looked into Gerrit / Rietveld etc, but all
> of them have serious downsides

Could you develop on those downsides?

> that seem a lot harder to fix.
If it scales for the size of chromium/ code base why it wouldn't fit for clang?

So yes, I'm prompting rietveld/codereview.chromium.org as I've been
working with it for 3 years and I have been very happy with it. It
works PRETTY WELL!
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Manuel Klimek]

On Sat, Aug 18, 2012 at 11:19 PM, Thiago Farina <[hidden email]> wrote:

> On Thu, Aug 16, 2012 at 9:35 AM, Manuel Klimek <[hidden email]> wrote:
>> On Thu, Aug 16, 2012 at 2:14 PM, Konstantin Tokarev <[hidden email]> wrote:
>>> 16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
>>>> 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.
>>>
>>> I'm a bit surprised that you being @google.com don't promote Gerrit [1].
>>
>> We want to use what fits the project best. Gerrit is a great tool for
>> a lot of use cases, unfortunately it doesn't fit the clang/llvm
>> workflow well. Of course I looked into Gerrit / Rietveld etc, but all
>> of them have serious downsides
> Could you develop on those downsides?

Yes. There are two questions though: what's the effort, and would
upstream gerrit accept patches that for example enable a post-review
subversion browser for post-commit reviews (I use phab to browse
revisions now because it's better than the websvn stuff). Adding all
that to gerrit would seem very expensive - if you want to try
implementing this & set up a gerrit instance that you think would work
for clang/llvm, feel free to do so.

>> that seem a lot harder to fix.
> If it scales for the size of chromium/ code base why it wouldn't fit for clang?

It's not a problem with scale. In fact, scale is the least of the
problems for clang/llvm code reviews.

> So yes, I'm prompting rietveld/codereview.chromium.org as I've been
> working with it for 3 years and I have been very happy with it. It
> works PRETTY WELL!

Yes, it might have worked pretty well for you. Stock phab without any
changes worked "pretty well" for me, too, but the community jumped at
it when we first tried it out. People are different.

Cheers,
/Manuel
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Konstantin Tokarev]

19.08.2012, 12:05, "Manuel Klimek" <[hidden email]>:

> On Sat, Aug 18, 2012 at 11:19 PM, Thiago Farina <[hidden email]> wrote:
>
>>  On Thu, Aug 16, 2012 at 9:35 AM, Manuel Klimek <[hidden email]> wrote:
>>>  On Thu, Aug 16, 2012 at 2:14 PM, Konstantin Tokarev <[hidden email]> wrote:
>>>>  16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
>>>>>  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.
>>>>  I'm a bit surprised that you being @google.com don't promote Gerrit [1].
>>>  We want to use what fits the project best. Gerrit is a great tool for
>>>  a lot of use cases, unfortunately it doesn't fit the clang/llvm
>>>  workflow well. Of course I looked into Gerrit / Rietveld etc, but all
>>>  of them have serious downsides
>>  Could you develop on those downsides?
>
> Yes. There are two questions though: what's the effort, and would
> upstream gerrit accept patches that for example enable a post-review
> subversion browser for post-commit reviews (I use phab to browse
> revisions now because it's better than the websvn stuff).

Not sure about Rietveld, but in Gerrit every change which went through review
system could be reviewed post-commit as well.

--
Regards,
Konstantin
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

[Thiago Farina]

On Sun, Aug 19, 2012 at 8:29 AM, Konstantin Tokarev <[hidden email]> wrote:

>
>
> 19.08.2012, 12:05, "Manuel Klimek" <[hidden email]>:
>> On Sat, Aug 18, 2012 at 11:19 PM, Thiago Farina <[hidden email]> wrote:
>>
>>>  On Thu, Aug 16, 2012 at 9:35 AM, Manuel Klimek <[hidden email]> wrote:
>>>>  On Thu, Aug 16, 2012 at 2:14 PM, Konstantin Tokarev <[hidden email]> wrote:
>>>>>  16.08.2012, 11:30, "Manuel Klimek" <[hidden email]>:
>>>>>>  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.
>>>>>  I'm a bit surprised that you being @google.com don't promote Gerrit [1].
>>>>  We want to use what fits the project best. Gerrit is a great tool for
>>>>  a lot of use cases, unfortunately it doesn't fit the clang/llvm
>>>>  workflow well. Of course I looked into Gerrit / Rietveld etc, but all
>>>>  of them have serious downsides
>>>  Could you develop on those downsides?
>>
>> Yes. There are two questions though: what's the effort, and would
>> upstream gerrit accept patches that for example enable a post-review
>> subversion browser for post-commit reviews (I use phab to browse
>> revisions now because it's better than the websvn stuff).
>
> Not sure about Rietveld, but in Gerrit every change which went through review
> system could be reviewed post-commit as well.
>

you can still browse reviews that were already committed without any
problems in rietveld/codeview.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev