[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Fedor Sergeev via llvm-dev
llvm-dev at lists.llvm.org
Thu Jan 23 14:29:40 PST 2020
On 1/24/20 1:11 AM, Mehdi AMINI wrote:
> On Thu, Jan 23, 2020 at 9:30 AM Fedor Sergeev via llvm-dev
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>
> Hubert, thank you so much for the detailed bisection of what I
> believe
> is the most
> problematic GitHub PR =<= Phab difference in terms of review process!
>
>
> This thread has been focused on a specific aspect of the tooling /
> process, the thread we had two months ago mentioned another important
> blocker which was the support for Herald rules.
Thanks for mentioning that, though I did read that other thread and
while I agree that something like Herald is important
for many folks here wrt their review workflow organization, still would
we magically get GitHub-Herald,
these obstacles in navigation through the review comments would still be
largerly in the way of big patch series.
Email is good for notification purposes, and I do like getting it myself.
But I doubt there are many people out there who review nontrivial
changes by looking at diffs in their e-mail client,
so in my book Herald issue comes second
(that is for sure my subjective view, yet I believe it is somewhat
reasonable).
>
> It is also quite possible that there are other issues, I would be
> cautious at relying too much on the current thread to consider having
> an exhaustive list of possible issues.
I dont think that waiting for a fully exhaustive list is required to
start bothering GitHub about
pain points which have already been identified as important for the project.
regards,
Fedor.
>
> Best,
>
> --
> Mehdi
>
>
> Summarizing, I see two main pain points in GitHub PR experience as
> explained here
> that beg for improvement:
> 1. low information density of "conversation" view
>
> 2. inconvenient navigation/discovery of comments for multiple
> commits
> in presence of force push
>
> As I see it, these two points will constantly get in the way of
> review
> process for nontrivial
> patch series (and somehow I believe that quality of review for
> nontrivial patches will have the
> biggest impact on the quality of the project itself).
>
> Do we have a good reason to believe that LLVM as a community has
> enough
> weight
> to nudge Github to really improve on these two points?
>
> If yes, then can we start moving there? File bugs or whatnot...
> It would be a very nontrivial effort for me to even describe the
> problem
> in comprehensible
> (and non-offensive ;) way, so I know I'm asking somebody else to
> do the
> hard job.
>
> best regards,
> Fedor.
>
> On 1/23/20 3:02 AM, Hubert Tong via llvm-dev wrote:
> > On Wed, Jan 22, 2020 at 5:19 PM David Greene
> <greened at obbligato.org <mailto:greened at obbligato.org>
> > <mailto:greened at obbligato.org <mailto:greened at obbligato.org>>>
> wrote:
> >
> > Hubert Tong <hubert.reinterpretcast at gmail.com
> <mailto:hubert.reinterpretcast at gmail.com>
> > <mailto:hubert.reinterpretcast at gmail.com
> <mailto:hubert.reinterpretcast at gmail.com>>> writes:
> >
> > > It was already mentioned earlier in the thread that:
> > > - The individual commits cannot be approved in the sense of
> > approving a PR.
> >
> > Later conversation makes me wonder how often that really happens
> > though.
> > In a true patch series I would think it would be very rare to be
> > able to
> > land a later commit before an earlier commit.
> >
> > I had provided the use case for a patch series where being able
> to do
> > so is possible (the case of related patches with little semantic
> > ordering but is high on syntactic merge conflicts). Whether or not
> > such a patch series qualifies as a "true" patch series might be a
> > philosophical discussion that could be had, but the practicality of
> > keeping a common dependency order was presented.
> >
> >
> > > - Comments on individual commits are easily lost.
> >
> > This is the piece that I feel needs more explanation. What does
> > "lost"
> > mean, exactly? I will comment more on your explanation below.
> >
> > "Lost" means that it becomes hard to find and track.
> >
> >
> > > To elaborate on the latter:
> > > - GitHub would not show comments in-line if it unilaterally
> > believes that
> > > the comment no longer applies to the version of the code
> you are
> > looking at.
> >
> > Ok, but doesn't Phab do the same? Or does Phab attach the
> comment to
> > some potentially completely inappropriate line? The latter seems
> > worse
> > to me.
> >
> > Phab does the latter, but you can (1) find the comment, and (2)
> find
> > its original context. The fact that it was on an older version
> of the
> > code is expressed by the shading/colour and an icon. It does not
> make
> > the comment hard to find like GitHub does. At some point, if a
> force
> > push in involved, the GitHub comments for the "replaced" commits
> can
> > only be found in the long conversation view. You might have no
> chance
> > of seeing them presented in-line. In other words, if a single PR is
> > treated as a patch series, updating an early commit might orphan
> all
> > the in-line comments on the latter commits if a force push is used.
> >
> > The long conversation view suffers from low information density
> (needs
> > lots of scrolling) and for single-PR-as-patch-series means that
> > comments on different commits are interleaved. More to the
> point: The
> > long conversation view lacks focus and context.
> >
> > Replies to older comments in GitHub have a tendency to be hard to
> > notice as well. A comment that needs pinging because people
> missed it
> > the first time can be pinged using the reply functionality in Phab
> > effectively. Doing the same in GitHub might leave the ping
> someplace
> > where people won't see it unless if they were notified. Even if
> they
> > were notified, they might not be able to find it easily except
> through
> > the notification.
> >
> >
> > > - GitHub would proactively hide and collapse comments in the
> > "conversation
> > > view", especially if it believes (for such bad reasons as line
> > noise in
> > > later commits) that an earlier comment is not relevant.
> >
> > I'm reading this as comments aren't "lost" in the sense that
> they are
> > completely deleted from the PR.
> >
> > Yes, but comments presented without any context is not great.
> Even if
> > the context has not been deleted due to force push, seeing older
> > comments in any sort of context using GitHub has required
> opening more
> > versions of the file in my experience than with Phab (where the
> > comments appear "near enough" fairly often).
> >
> > They may be collapsed. What does
> > "hide" mean? Are they completely inaccessible?
> >
> > GitHub has several layers of making comments hard to find or
> notice.
> > Blocks of comments may become "hidden conversations" in the
> > conversation view (including completely new ones, e.g., if a
> reviewer
> > does their first pass over a patch and had a lot of comments to
> make).
> > This need to "load more" is presumably designed to save on server
> > bandwidth. Individual comments may be hidden (similar to collapsing
> > inline comments in Phab) as well (the comment text is folded away)
> > because they are presumably "outdated" or "resolved".
> >
> > With GitHub, "anyone" can cause comments to become less noticeable.
> > With Phab, it saves the state of whether inline comments are
> collapsed
> > by you for you.
> >
> >
> > This is an important point. If comments are simply
> collapsed and
> > I can
> > easily get at them if I need to, that's not too much of a
> problem for
> > me.
> >
> > I have not found it to be easy to get to them. When requesting to
> > "show" everything, the hidden conversations remain hidden. I had to
> > scroll through and "load" the blocks one by one.
> >
> > After all, Phab auto-hides comments all the time if the
> > conversation gets "too long."
> >
> > Not the inline comments, and for inline comments, Phab includes
> a bit
> > of the comment even when collapsed. GitHub doesn't.
> >
> > I am frequently annoyed by having to
> > un-collapse them. Sounds like I would be annoyed in the
> same way with
> > GitHub, so nothing really gained or lost to me.
> >
> > Uncollapsing in Phab has been much easier for me than on GitHub.
> Also,
> > for similarly sized reviews, I've had to uncollapse on GitHub more
> > often than on Phab.
> >
> >
> > -David
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto: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/llvm-dev/attachments/20200124/80c128bc/attachment.html>
More information about the llvm-dev
mailing list