[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
David Greene via llvm-dev
llvm-dev at lists.llvm.org
Wed Jan 22 14:12:46 PST 2020
David Blaikie <dblaikie at gmail.com> writes:
>> 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.
Yes, I can definitely appreciate that.
>> 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).
Ok. If it is true that GitHub can be configured to show a branch as a
series of diffs rather than a giant diff, hopefully we'd have similar
functionality. Of course someone would need to verify that. :)
>From what I have read GitHub does *not* present comments attached to
individual commit diffs, which is certainly a downgrade from Phab.
>> 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.
Makes sense, that's what I assumed. I am not sure Phab provides any
benefit over GitHub PRs in this regard.
More information about the llvm-dev