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

On Wed, Jan 22, 2020 at 5:19 PM David Greene <greened at obbligato.org>
wrote:
> Hubert Tong <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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/4da84038/attachment.html>

Hubert Tong <hubert.reinterpretcast at gmail.com> writes:
> 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.
Thanks for relating your experience, it's very helpful!  I will play
with GitHub a bit and see what it does.  From your description it does
sound like Phab does a better job of keeping comments more accessible.
Is that worth the cost of maintaining a server/software changes and
using a very niche product with not as much general familiarity and
tooling around it?  I don't know.

                           -David

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!

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

Fedor Sergeev via llvm-dev <llvm-dev at lists.llvm.org> writes:
> 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?
I think we have reason to at least try.  They've implemented other stuff
for us.
> 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.
My guess is someone with greater public visibility/clout than myself
filing bugs and pushing this along would make better progress.

So yeah, I know I'm also asking someone else to do the hard job.  But I
am happy to help in any way I can.

I did find these.  This is an unofficial issue tracker for github.com.
The official way to communicate is apparently e-mailing
support at github.com, which is not very great.  People who send the e-mail
are also posting to this issue tracker so others know what has been sent
and whether there have been any responses from GitHub.

Ability to approve each commit in a PR
https://github.com/isaacs/github/issues/1376

Do not auto dismiss PR comments on code change
https://github.com/isaacs/github/issues/1159
Partially addressed:
https://github.blog/changelog/2018-09-25-precision-review-comment-outdating/

Mark pull request as depending on another
https://github.com/isaacs/github/issues/959
Some workarounds:
https://github.com/ryanhiebert/probot-chain
https://github.com/z0al/dep
And maybe official support:
https://twitter.com/natfriedman/status/1170804894241972224
https://unhashable.com/stacked-pull-requests-keeping-github-diffs-small/


                  -David

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>