<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 1/24/20 6:45 AM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CANF-O=bpBNARVquWe2xFypP--bCEGOkgj0zt6g_9JQEczEaMSQ@mail.gmail.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<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" moz-do-not-send="true">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"
moz-do-not-send="true">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>
Eeek... thats what you get when posting late at night :-/. Mixed up
two different issues.<br>
And yes, I do rely on subscription by Herald, so in my book it is
quite high on the list.<br>
Still I can see workarounds that I could apply myself here (e.g.
external scanning of pull-requests).<br>
<br>
<blockquote type="cite"
cite="mid:CANF-O=bpBNARVquWe2xFypP--bCEGOkgj0zt6g_9JQEczEaMSQ@mail.gmail.com">
<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">
<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>
</div>
</blockquote>
Understood.<br>
My primary concern is to make sure GH is good enough *when* we
switch, I dont need to hurry here :)<br>
<br>
regards,<br>
Fedor.<br>
<blockquote type="cite"
cite="mid:CANF-O=bpBNARVquWe2xFypP--bCEGOkgj0zt6g_9JQEczEaMSQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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" moz-do-not-send="true">greened@obbligato.org</a>
<br>
> <mailto:<a
href="mailto:greened@obbligato.org"
target="_blank" moz-do-not-send="true">greened@obbligato.org</a>>>
wrote:<br>
><br>
> Hubert Tong <<a
href="mailto:hubert.reinterpretcast@gmail.com"
target="_blank" moz-do-not-send="true">hubert.reinterpretcast@gmail.com</a><br>
> <mailto:<a
href="mailto:hubert.reinterpretcast@gmail.com"
target="_blank" moz-do-not-send="true">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" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
> <a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">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" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
<a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>