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

On Thu, Jan 16, 2020 at 1:45 PM David Greene via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Nicolai Hähnle <nhaehnle at gmail.com> writes:
>
> > Here's a somewhat more complex example of changes made by myself a
> > year and a half ago, starting with https://reviews.llvm.org/D48013
>
> Aha!  I found it!  The "Stack" tab under "Revision
Contents" after all
> of the review comments.  That's *really* not obvious if there are lots
> of review comments to scroll through.
>
> So if I'm understanding this correctly, there are 17 commits in this
> series, one of which is still open for review.  Is that correct?
>
> What is the graph on the left trying to show me?  The commit graph?
> This is a non-linear branch?  Or is this a dependence graph between
> patches in the series?  If the latter, what generated those
> dependencies?
>
> > Reviewing all of this in a single amorphous diff is something that I
> > wouldn't wish on anybody.
>
> Certainly!  I don't think anyone is advocating for that.  I agree
GitHub
> by default presenting things as a giant patch is not helpful, but I'm
> told that can be changed.  I don't how GitHub reviews work for PRs with
> multiple commits since I've never tried it.  Hopefully someone with
such
> experience will chime in.
>
It was already mentioned earlier in the thread that:
- The individual commits cannot be approved in the sense of approving a PR.
- Comments on individual commits are easily lost.

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.
- 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.

>
> > Conversely, having the linkage between different commits provides
> > context to reviewers.
>
> I can see that too.  For commits that are dependent on a long series of
> other commits (D48017 for example), how hard is it to review without
> reviewers also looking at the commits upon which it depends?  Do people
> really review such commits early in the process?  Even without the early
> review I agree the context can be useful.
>
> > It is also helpful to me: I can keep track of reviews to the series
> > while simultaneously working on other changes that happen to be
> > unrelated, and having the commits in separate stacks helps by
> > implicitly grouping them. Admittedly this advantage is comparatively
> > minor because the UI isn't perfect.
>
> I'm not sure how this differs from having multiple patches up for
review
> simultaneously.  I do that all the time and is just as easy (for me)
> with GitHub PRs.  Are you simply saying that you can have multiple patch
> series up for review at the same time?  I feel like I am not grasping
> some (to me) subtle point you're making.
>
>                      -David
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/486a429c/attachment.html>

Hubert Tong <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.
> - 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.
> 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.
> - 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.  They may be collapsed.  What does
"hide" mean?  Are they completely inaccessible?

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.  After all, Phab auto-hides comments all the time if the
conversation gets "too long."  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.

                         -David

On Wed, Jan 22, 2020 at 5:19 PM David Greene <greened at obbligato.org>
wrote:
> Hubert Tong <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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/4da84038/attachment.html>