[cfe-dev] [llvm-dev] Phabricator Maintenance

[James Henderson via cfe-dev]

On Fri, 26 Jun 2020 at 09:53, Manuel Jacob <me at manueljacob.de> wrote:

> On 2020-06-26 09:53, James Henderson via llvm-dev wrote:
> > Relatedly, Phabricator doesn't stop you continuing a comment chain for
> > reasons I have yet to follow, which Github sometimes does.
> >
> > Some others:
> > 1) I believe Github also doesn't have an easy way to respond to
> > multiple
> > comments simultaneously, if you are not in "review" mode, (which is
> > always
> > the case if you are replying to out-of-line comments).
> > 2) Typically in our Phabricator, the description and summary of a patch
> > is
> > considered to be roughly what goes in the final commit message (same
> > with
> > Github). However, updating said commit message isn't possible to my
> > knowledge without force-pushing in Github (which as discussed elsewhere
> > in
> > the thread causes other problems).
> > 3) Marking comments as "Done" in Phabricator does not auto-hide them,
> > whereas in Github marking a comment as "Resolved" does. Spoken from
> > experience, this leads to situations in Github where a developer thinks
> > they've resolved a reviewers comments, marks them as Resolved, but
> > actually
> > they weren't. The reviewer in turn then has to browse the comments in
> > the
> > summary page, to see if they have any unaddressed comments that were
> > supposedly resolved, which given the limited context available there is
> > a
> > non-trivial task sometimes.
>
> In practice, I’ve found that these two functionalities are used in
> different ways. In Phabricator, the implementer marks the comments as
> done. In GitHub, the reviewer marks comments are resolved. Actually, I
> would like to have both, but I’m not aware of a review software that has
> both.
>

That might be a cultural thing then, and would need documenting under some
sort of "Best Practices when Reviewing" doc, since I ran into this exact
problem in our downstream Github repo, where we use PRs for reviews.


>
> > There are probably others, but those are the ones not already mentioned
> > that I can come up with so far.
> >
> > P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to
> > working with Github towards getting the feature parity with Phabricator
> > sorted, I don't want to switch until the two are on a par with each
> > other,
> > which in my opinion is not yet, so being forced to do so prematurely
> > due to
> > lack of maintainers would have made me very sad (P.P.S thank you Manuel
> > for
> > Phabricator in the first place!).
> >
> > On Thu, 25 Jun 2020 at 22:39, Hubert Tong via cfe-dev <
> > cfe-dev at lists.llvm.org> wrote:
> >
> >> On Thu, Jun 25, 2020 at 12:29 PM Mehdi AMINI via llvm-dev <
> >> llvm-dev at lists.llvm.org> wrote:
> >>
> >>>
> >>> Re: patch chains - perhaps we can ask github on support for that?
> >>>> I think what would help is somebody providing a doc with the
> >>>> features
> >>>> that phab provides that github PR doesn't, so we can get some
> >>>> consensus on
> >>>> what the diff is.
> >>>>
> >>>
> >>> Indeed, I'll try to start a doc on this.
> >>>
> >> Thanks. Some things that are high on my list:
> >> Phabricator has persistent per-user inline comment collapsing.
> >> Phabricator can show all inline comments over the lifetime of the
> >> patch
> >> in-line (even if the placement is not perfect) in the full diff view.
> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >>
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200626/aa4d4fba/attachment.html>