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

[David Blaikie via llvm-dev]

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>