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

[Fedor Sergeev via llvm-dev]

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