llvm dev - Jan 2020 - [cfe-dev] Phabricator -> GitHub PRs?

On Thu, Jan 23, 2020 at 9:30 AM Fedor Sergeev via llvm-dev <
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.

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.

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>> wrote:
> >
> >     Hubert Tong <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
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-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/llvm-dev/attachments/20200123/d502e2a4/attachment.html>

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>

On Thu, Jan 23, 2020 at 2:29 PM Fedor Sergeev <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> 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?

>
> 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.

-- 
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>> wrote:
>> >
>> >     Hubert Tong <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
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-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/llvm-dev/attachments/20200123/e74efebb/attachment.html>