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

Hi David,

On Wed, Jan 15, 2020 at 12:51 PM David Greene via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> I would like to understand better how people use Phab's advanced
> features and why.  For example, what drives the need for patch series
> and dependence relationships?  Some walk-through examples would be very
> helpful.
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

It's a series of changes across TableGen and the AMDGPU backend to
refactor how we represent a class of instructions. Some patterns that
occur:
- an NFC (no functional change) refactoring/rename change in an area
that is then later touched by a functional change.
- a change to common infrastructure (TableGen) that is motivated by
and used by a subsequent functional change in the AMDGPU backend, but
which can stand on its own and which may want to be reviewed by
different people
- a cleanup change to remove deprecated functionality from the backend
once the refactored functionality is available, separated out in order
to smoothen the upgrade path for frontends that are maintained outside
of the llvm-project repository

Reviewing all of this in a single amorphous diff is something that I
wouldn't wish on anybody. Conversely, having the linkage between
different commits provides context to reviewers.

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.

Cheers,
Nicolai
-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

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

On Thu, Jan 16, 2020 at 10:45 AM 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.
>
There are notes within the comments about when child/parent revisions are
added. So if you're actively reviewing, and open the review to see the
latest comments - you'd probably see a comment like "Split this off
into
its own review" & an auto-comment/status update describing that new
relationship. If you were opening a new patch series that had just been
sent out - you'd see that relationship status update as the first comment
sort of thing.

> 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?
>
Roughly speaking. At this point - that one remaining is just like a normal
review, since it's based on committed code/not another outstanding review.

> 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?
>
Correct - it's the dependence graph. Humans generated the dependencies -
there's an "Edit Related Revisions" button on the top right, where
you can
specify patches that are children or parents to this one.

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

Sometimes easy, sometimes harder - and depends on the kind of review. for
me this really helps keep the substantive parts of the review focussed -
you can move depedent refactoring into separate reviews that are easy to
eyeball knowing you aren't expecting to find anything
"interesting". Then
separately review the "interesting" stuff without all the noise of the
refactoring. That's super important to me.

>   Do people
> really review such commits early in the process?  Even without the early
> review I agree the context can be useful.
>
Yep - I might do superficial review early - basic stylistic stuff to get
that out of the way even if the substance isn't ready to be reivewed yet
(most often by suggesting ways to pull out refactorings to focus the main
review on the substantive parts).

>
> > 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.
>
I believe he's saying that having patch series grouped together makes it
easier to think of it as a logical unit. Rather than "I have 10 patches out
for review" thinking of it as "I have 5 features out for review" 
-maybe
one complicated on ein the form of a sries of 5 commits, and a small pair,
and 4 singles, for instance.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200116/e99f21b4/attachment.html>

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>