Phabricator and Bugzilla

[smaug]

Hi all,

just some random notes about Phabricator.

I've been reviewing now a bunch of patches in Phabricator and the initial feeling from
reviewer's point of view is that it is ok. Not great, but ok.
MozReview's interdiff, when it works, is easier to use or at least to discover than Phabricator's.
MozReview does also show the method name in which the code lives. This latter thing is actually a big
+ to MozReview. (Same works in Bugzilla too when reviewing raw -P patches at least. No idea about splinter, since I never use it).
If Pabricator has the feature, I haven't found it yet - its UI is a tad confusing occasionally.

What is currently rather broken is the flag synchronization between bugzilla and Phabricator. One doesn't see in Bugzilla whether review has been
asked or whether review has been denied (r-). I believe people asking reviews from me set the r? flag manually in bugzilla.
Having an easy way to see that r- has been given to a patch is very valuable to the reviewer when looking at the bug again.
Oddly enough, approving the patch in Phabricator does set r+ in Bugzilla.

Not mirroring comments from Phabricator to Bugzilla has already made following reasoning for certain code changes harder.
It is so easy to include non-code level information in review comments. So far I haven't had a case where I would have needed to look at the blame and
try to find relevant information both in bugzilla and in phabricator, but that will obviously happen if we don't do the mirroring and it will slow
down reviewing in the future (since looking at the history/reasoning for the code changes is a big part of code reviewing).

I'm sure I haven't found all the tricks Phabricator has to help reviewing. Is there some reviewing-in-Phabricator-for-dummies doc?


I haven't asked reviews in Phabricator (nor in MozReview), so can't comment on how they compare from patch author's point of view.



br,



-Olli

[Mark Côté]

Regarding comment and flag mirroring, we've discussed this before:

https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/e3Pi-_FpBgAJ
https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/tsF7UfxvBgAJ

Given that Phabricator is still new, I don't see any reason to reopen that discussion at this point, aside from noting that we have work in progress to include Phabricator requests in BMO's My Dashboard and notifications indicator (https://bugzilla.mozilla.org/show_bug.cgi?id=1440828).

As for interdiffs, feel free to file a bug with any problems you see. We have a good relationship with upstream and can pass those on. Similarly with method names (which has been mentioned before but I can't find where at the moment).

There is official documentation at https://secure.phabricator.com/book/phabricator/ which is linked from our Mozilla-specific docs (http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html) which in turn is linked in the left-hand menu in Phabricator. We can expand our own docs as needed if there are areas that are particularly confusing due to, say, expectations carried over from our other code-review tools.

Mark

[Ben Kelly]

On Sat, Mar 31, 2018, 10:09 AM Mark Côté <mc...@mozilla.com> wrote:

> Regarding comment and flag mirroring, we've discussed this before:
>
>
> https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/e3Pi-_FpBgAJ
>
> https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/tsF7UfxvBgAJ
>
> Given that Phabricator is still new, I don't see any reason to reopen that
> discussion at this point, aside from noting that we have work in progress
> to include Phabricator requests in BMO's My Dashboard and notifications
> indicator (https://bugzilla.mozilla.org/show_bug.cgi?id=1440828).
>

What about comment mirroring? On my mobile so I haven't read all the past
threads, but my recollection is that your team did not want to implement
that feature. Personally, this is a huge concern for me.

Thanks.

Ben

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Mark Côté]

As I indicated, those posts go into detail on why we are avoiding both
comment and more complicated flag mirroring.


Mark

On Sat, Mar 31, 2018 at 10:14 AM, Ben Kelly <bke...@mozilla.com> wrote:

> On Sat, Mar 31, 2018, 10:09 AM Mark Côté <mc...@mozilla.com> wrote:
>

>> Regarding comment and flag mirroring, we've discussed this before:
>>
>> https://groups.google.com/d/msg/mozilla.dev.platform/
>> Y8kInYxo8UU/e3Pi-_FpBgAJ
>> https://groups.google.com/d/msg/mozilla.dev.platform/
>> Y8kInYxo8UU/tsF7UfxvBgAJ
>>
>> Given that Phabricator is still new, I don't see any reason to reopen
>> that discussion at this point, aside from noting that we have work in
>> progress to include Phabricator requests in BMO's My Dashboard and
>> notifications indicator (https://bugzilla.mozilla.org/
>> show_bug.cgi?id=1440828).
>>
>

> What about comment mirroring? On my mobile so I haven't read all the past
> threads, but my recollection is that your team did not want to implement
> that feature. Personally, this is a huge concern for me.
>
> Thanks.
>
> Ben
>
>

[Randell Jesup]

>As I indicated, those posts go into detail on why we are avoiding both
>comment and more complicated flag mirroring.
>
>Mark

There's no obvious discussion of "flags" in the linked discussions you
gave; I find only a couple of references to "flag" - in a question from
gps. Given how long the thread is (52 posts), perhaps you can point to
something more specific? Thanks

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[Mark Côté]

On Monday, 9 April 2018 15:29:35 UTC-4, Randell Jesup wrote:
> >As I indicated, those posts go into detail on why we are avoiding both
> >comment and more complicated flag mirroring.
> >
> >Mark
>
> There's no obvious discussion of "flags" in the linked discussions you
> gave; I find only a couple of references to "flag" - in a question from
> gps. Given how long the thread is (52 posts), perhaps you can point to
> something more specific? Thanks

gps's post is what I was talking about. It doesn't go into many details, but, to reiterate his main point, the mapping of disparate review systems is difficult and has lots of edge cases. To add a bit of specifics, the models behind review flags in BMO, which are independent objects associated with specific attachments, is very different from the way Phabricator treats reviews, which are particular states of a revision. It's doubtful we could ever have a perfect translation, so it would always be some sort of a hack. We've determined that a better solution is to expose Phabricator requests in Bugzilla as notifications and dashboards, as I linked in one of my replies above.

Mark