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