<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 23, 2020 at 2:29 PM Fedor Sergeev <<a href="mailto:fedor.sergeev@azul.com" target="_blank">fedor.sergeev@azul.com</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">
<div bgcolor="#FFFFFF">
<br>
<br>
<div>On 1/24/20 1:11 AM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite">
On Thu, Jan 23, 2020 at 9:30 AM Fedor Sergeev via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
wrote:<br>
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<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>
</div>
</div>
</blockquote>
Thanks for mentioning that, though I did read that other thread and
while I agree that something like Herald is important<br>
for many folks here wrt their review workflow organization, still
would we magically get GitHub-Herald,<br>
these obstacles in navigation through the review comments would
still be largerly in the way of big patch series.<br>
<br>
Email is good for notification purposes, and I do like getting it
myself.<br>
But I doubt there are many people out there who review nontrivial
changes by looking at diffs in their e-mail client,<br>
so in my book Herald issue comes second<br>
(that is for sure my subjective view, yet I believe it is somewhat
reasonable).<br></div></blockquote><div><br></div><div>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?</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"><div bgcolor="#FFFFFF"><blockquote type="cite"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><br>
<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>
</div>
</div>
</blockquote>
I dont think that waiting for a fully exhaustive list is required to
start bothering GitHub about<br>
pain points which have already been identified as important for the
project.<br></div></blockquote><div><br></div><div>Absolutely!</div><div>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.</div><div><br></div><div>-- </div><div>Mehdi</div><div><br></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"><div bgcolor="#FFFFFF">
<br>
regards,<br>
Fedor.<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<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>
</blockquote>
<br>
</div>
</blockquote></div></div>