<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 16, 2020 at 10:45 AM David Greene via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> writes:<br>
<br>
> Here's a somewhat more complex example of changes made by myself a<br>
> year and a half ago, starting with <a href="https://reviews.llvm.org/D48013" rel="noreferrer" target="_blank">https://reviews.llvm.org/D48013</a><br>
<br>
Aha!  I found it!  The "Stack" tab under "Revision Contents" after all<br>
of the review comments.  That's *really* not obvious if there are lots<br>
of review comments to scroll through.<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">So if I'm understanding this correctly, there are 17 commits in this<br>
series, one of which is still open for review.  Is that correct?<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">What is the graph on the left trying to show me?  The commit graph?<br>
This is a non-linear branch?  Or is this a dependence graph between<br>
patches in the series?  If the latter, what generated those<br>
dependencies?<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Reviewing all of this in a single amorphous diff is something that I<br>
> wouldn't wish on anybody.<br>
<br>
Certainly!  I don't think anyone is advocating for that.  I agree GitHub<br>
by default presenting things as a giant patch is not helpful, but I'm<br>
told that can be changed.  I don't how GitHub reviews work for PRs with<br>
multiple commits since I've never tried it.  Hopefully someone with such<br>
experience will chime in.<br>
<br>
> Conversely, having the linkage between different commits provides<br>
> context to reviewers.<br>
<br>
I can see that too.  For commits that are dependent on a long series of<br>
other commits (D48017 for example), how hard is it to review without<br>
reviewers also looking at the commits upon which it depends?</blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">  Do people<br>
really review such commits early in the process?  Even without the early<br>
review I agree the context can be useful.<br></blockquote><div><br>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).<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> It is also helpful to me: I can keep track of reviews to the series<br>
> while simultaneously working on other changes that happen to be<br>
> unrelated, and having the commits in separate stacks helps by<br>
> implicitly grouping them. Admittedly this advantage is comparatively<br>
> minor because the UI isn't perfect.<br>
<br>
I'm not sure how this differs from having multiple patches up for review<br>
simultaneously.  I do that all the time and is just as easy (for me)<br>
with GitHub PRs.  Are you simply saying that you can have multiple patch<br>
series up for review at the same time?  I feel like I am not grasping<br>
some (to me) subtle point you're making.<br></blockquote><div><br>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.<br><br><br></div></div></div>