<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 22, 2020 at 5:19 PM David Greene <<a href="mailto:greened@obbligato.org">greened@obbligato.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hubert Tong <<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 approving a PR.<br>
<br>
Later conversation makes me wonder how often that really happens though.<br>
In a true patch series I would think it would be very rare to be able to<br>
land a later commit before an earlier commit.<br></blockquote><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - Comments on individual commits are easily lost.<br>
<br>
This is the piece that I feel needs more explanation. What does "lost"<br>
mean, exactly? I will comment more on your explanation below.<br></blockquote><div>"Lost" means that it becomes hard to find and track.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> To elaborate on the latter:<br>
> - GitHub would not show comments in-line if it unilaterally believes that<br>
> the comment no longer applies to the version of the code you are 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 worse<br>
to me.<br></blockquote><div>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.</div><div>
<div><br></div><div>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.<br></div>
</div><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - GitHub would proactively hide and collapse comments in the "conversation<br>
> view", especially if it believes (for such bad reasons as line 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.</blockquote><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">They may be collapsed. What does<br>
"hide" mean? Are they completely inaccessible?<br></blockquote><div>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".</div><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This is an important point. If comments are simply collapsed and I can<br>
easily get at them if I need to, that's not too much of a problem for<br>
me.</blockquote><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">After all, Phab auto-hides comments all the time if the<br>
conversation gets "too long."</blockquote><div>Not the inline comments, and for inline comments, Phab includes a bit of the comment even when collapsed. GitHub doesn't.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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></blockquote><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-David<br>
</blockquote></div></div>