[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Fedor Sergeev via llvm-dev
llvm-dev at lists.llvm.org
Thu Jan 23 21:54:06 PST 2020
On 1/24/20 6:45 AM, Mehdi AMINI wrote:
>
>
> On Thu, Jan 23, 2020 at 2:29 PM Fedor Sergeev <fedor.sergeev at azul.com
> <mailto:fedor.sergeev at azul.com>> wrote:
>
>
>
> 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).
>
>
> I don't really understand the link between email review and Herald: I
> use Herald mainly to manage subscribing myself to specific revision on
> Phabricator based on author/file path/description so that I don't have
> to look into the full list for all LLVM projects and components. How
> do you do this with pull-request?
Eeek... thats what you get when posting late at night :-/. Mixed up two
different issues.
And yes, I do rely on subscription by Herald, so in my book it is quite
high on the list.
Still I can see workarounds that I could apply myself here (e.g.
external scanning of pull-requests).
>>
>> 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.
>
>
> Absolutely!
> I was just expressing caution about using this list for another
> purpose than what you mention here: getting GH to fix some of these
> issues may not be enough to unblock a migration.
Understood.
My primary concern is to make sure GH is good enough *when* we switch, I
dont need to hurry here :)
regards,
Fedor.
>
> --
> Mehdi
>
>
>
> 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/a0a17f74/attachment-0001.html>
More information about the llvm-dev
mailing list