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

[Hubert Tong via llvm-dev]

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>