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

Wed Sep 16 01:36:32 PDT 2020

Apparently reviewable.io requests full permissions to control the
repository and its settings.

We need to figure out why it needs so much...

On Wed, Sep 16, 2020 at 9:43 AM Stella Laurenzo via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Uh-oh: Failed to publish: GitHub error 404 on POST
> https://api.github.com/repos/llvm/mlir-npcomp/pulls/42/reviews: Not Found
> (The llvm organization may need to authorize Reviewable as an accepted
> third party application.)
>
> Can an admin take the suggested action on the mlir-npcomp project in the
> LLVM org? I've followed the instructions in this help doc
> <https://docs.github.com/en/github/setting-up-and-managing-your-github-user-account/requesting-organization-approval-for-oauth-apps>
> and requested that reviewables be granted access to the LLVM organization.
> I'm not completely aware of how this will show up on the admin side. +LLVM
> Administration List <llvm-admin at lists.llvm.org>
>
> [image: image.png]
>
> I'm far from an expert user of reviewable after one review, but I found it
> wholly superior to the built-in GitHub UI and will likely keep using it
> (assuming no major problems). I've got comments pending on the above access
> request and will check in the morning. If not resolved, I'll port them to
> the normal GitHub UI to unblock the review.
>
> On Tue, Sep 15, 2020 at 2:48 PM Stella Laurenzo <stellaraccident at gmail.com>
> wrote:
>
>> I just tried it for a fairly trivial review on another project. I'll need
>> to do some more complex reviews with it, but my initial impressions are
>> quite positive. The UI is intuitive and seems "review oriented", whereas
>> I've always felt like GitHub's UI is very "commit oriented" -- it feels
>> like a review tool. I should also give it a try on mobile: the UI presents
>> as a long-scrolling-page form factor that feels slightly odd on desktop,
>> but I suspect is optimized for tablet/phone use.
>>
>>
>> On Mon, Sep 14, 2020 at 12:44 PM James Y Knight via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Has anyone tried out reviewable.io yet? It integrates with GitHub pull
>>> requests, but provides a separate UI for doing the review which promises to
>>> fix a lot of the issues encountered with Github's review interface.
>>>
>>> Some of the things it claims to support which seem like important
>>> additions:
>>> - Tracking the resolved status of each discussion point
>>> - Rebasing a PR without losing review history.
>>> - Optionally reviewing each commit in a branch separately, and tracking
>>> across rebase/force-pushes of the PR branch, via matching the
>>> commit descriptions of the new push against the old push.
>>>
>>> However, I haven't tried it. It'd be great if some of the folks using
>>> GitHub PRs could try switching to reviewable.io, to evaluate whether
>>> that can more successfully replace phabricator than just GitHub PRs by
>>> themselves.
>>>
>>> On Mon, Sep 14, 2020 at 3:14 PM Nicolai Hähnle via cfe-dev <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>>> Hi Stephen,
>>>>
>>>> Here's a real-life example of reviews that GitHub doesn't properly
>>>> support to my knowledge: https://reviews.llvm.org/D83088, look for
>>>> "Stack" under "Revision Contents".
>>>>
>>>> The key here is that there's a sequence of commits that's partially
>>>> dependent on each other, so that reviewing each one individually is
>>>> suboptimal, but at the same time you really don't want to be forced to
>>>> review the whole thing as a unit. In fact, I've been adding to it over
>>>> time.
>>>>
>>>> If the review process of LLVM was significantly faster, the pain of
>>>> GitHub's inferior tooling here wouldn't be quite as much of a problem,
>>>> but it just isn't -- and I don't think hurrying up the process at the
>>>> expense of quality is quite the right answer, either.
>>>>
>>>> Cheers,
>>>> Nicolai
>>>>
>>>> On Fri, Sep 11, 2020 at 7:39 PM Stephen Neuendorffer
>>>> <stephen.neuendorffer at gmail.com> wrote:
>>>> > The LLVM incubator projects have been using github PRs for reviews
>>>> and so far haven't really seen any significant issues.  The biggest
>>>> confusion so far has not been with reviews but with the difference between
>>>> "rebase and merge" and "squash and mere".  We have used basically 3
>>>> different processes:
>>>> >
>>>> > Method 1: start a review with one commit on a new branch (typically
>>>> in a personal repository 'fork'). Respond to review comments by adding
>>>> additional commits on the same branch. Then you want to "Squash and
>>>> Merge".   Real life example: https://github.com/llvm/circt/pull/62
>>>> Note that Github keeps seems to keep a reference to commits where a review
>>>> occurred, but does not necessarily preserve other commits in the chain.
>>>> Also note that you can go back and look at either the final disposition of
>>>> the code on the main page, or view the diff "as reviewed" to understand
>>>> comments in their original context.
>>>> >
>>>> > Method 2: start a review as a sequence of commits. Respond to review
>>>> comments by updating your sequence of commits rebasing locally and git push
>>>> -f to update your branch with a new set of commits. The sequence of commits
>>>> is preserved using 'rebase and merge'.  Real life example:
>>>> >
>>>> > Method 3: start a review with one commit, respond to review comments
>>>> with git commit --amend on the commit. git push -f to update your branch.
>>>> In this case, only one commit is being merged, so 'rebase and merge' and
>>>> 'squash and merge'.  Real life example:
>>>> https://github.com/llvm/circt/pull/63
>>>> >
>>>> > Some projects attempt to simplify the options and only allow "squash
>>>> and merge" as an option.  This eliminates Method 2, but also prevents
>>>> mistakes where people intend to use Method 1, but accidently select "Rebase
>>>> and merge".  This results in undesired commit history littered with a bunch
>>>> of meaningless commits like "Fix typo.", "Address comments." etc  Method 2
>>>> is generally harder to review and discouraged, but seems sometimes
>>>> necessary when you have dependent commits that are logically separate but
>>>> need to be reviewed together to make sense.
>>>> >
>>>> > There seems to be a split between those who prefer to curate commits
>>>> locally and present them in the PR (i.e. Method 3) as they are to be
>>>> committed (i.e. squash/amend/etc from my workstation and push the result),
>>>> and those who seem to feel that it is better to avoid "git rebase" and "git
>>>> commit --amend" and let github handle rewriting the history with the commit.
>>>> >
>>>> > Steve
>>>> >
>>>> >
>>>> >
>>>> > On Wed, Jan 29, 2020 at 12:56 AM Nicolai Hähnle via llvm-dev <
>>>> llvm-dev at lists.llvm.org> wrote:
>>>> >>
>>>> >> On Tue, Jan 28, 2020 at 2:23 PM Renato Golin <rengolin at gmail.com>
>>>> wrote:
>>>> >> > On Tue, 28 Jan 2020 at 12:28, Nicolai Hähnle <nhaehnle at gmail.com>
>>>> wrote:
>>>> >> > > I don't quite follow. Yes, gratuitous useless changes tend to
>>>> create
>>>> >> > > merge and rebase problems and should generally be avoided. Why
>>>> does it
>>>> >> > > make a difference whether they're in multiple commits though?
>>>> >> >
>>>> >> > I think you misunderstood.
>>>> >> >
>>>> >> > Our policy [1] is that independent NFC fixups on existing code
>>>> should
>>>> >> > be separate from new changes.
>>>> >> >
>>>> >> > The point here is that fixups *to the patch* should be squashed
>>>> into
>>>> >> > their original commits *before* merging.
>>>> >>
>>>> >> Okay, this intention wasn't clear to me from what was written. It
>>>> >> looks like we agree, then.
>>>> >>
>>>> >>
>>>> >>
>>>> >> > Once the patches are in master, and problems have been found, then
>>>> >> > adding a fixup to master is totally fine.
>>>> >> >
>>>> >> > We don't want to rewrite history on master, that'd be a nightmare.
>>>> If
>>>> >> > something is botched, we revert, then reapply.
>>>> >> >
>>>> >> > We also don't want to have fixups to things that haven't landed on
>>>> >> > master yet, so cleaning up the patches and series before merging is
>>>> >> > highly encouraged.
>>>> >> >
>>>> >> > But during the review, rewriting history of the series itself
>>>> makes it
>>>> >> > hard for any review tool to have meaningful representations.
>>>> >>
>>>> >> This isn't quite true, as others already pointed out, and *not*
>>>> >> rewriting history can make reviews harder as well! In fact, I've
>>>> *just
>>>> >> now* had that happen to me on another GitHub project, where this
>>>> >> sequence of events happened:
>>>> >>
>>>> >> 1. PR was opened with a series of commits, one of which (call it
>>>> >> commit B for base) is non-trivial and under review separately as a
>>>> >> different PR.
>>>> >> 2. Other reviewer makes comments, asks for some refactoring changes.
>>>> >> 3. Author makes those changes, adds them as a fixup commit.
>>>> >> 4. I can now no longer usefully review the PR, because I only have
>>>> two
>>>> >> options, both of which are similarly useless:
>>>> >> 4a. I look at all changes in the PR, in which case I get a messy
>>>> >> mixture of commit B (which ought to be reviewed separately) and the
>>>> >> rest.
>>>> >> 4b. I look at individual commits in the PR, but then I only see a
>>>> >> stale version of the author's work.
>>>> >>
>>>> >> The fixup approach *might* work if there is only a single reviewer,
>>>> >> but even then I suspect things can quickly become messy. And in any
>>>> >> case, the default assumption in LLVM should be that anybody can join
>>>> a
>>>> >> review at any time.
>>>> >>
>>>> >> The one tool that actually gets this right is Gerrit, which
>>>> >> understands commit series *and* allows you to diff between versions
>>>> of
>>>> >> a commit. It's unfortunate that Gerrit is so ugly that most people
>>>> >> won't even look at it (and it does have other weaknesses as well,
>>>> >> admittedly).
>>>> >>
>>>> >> Cheers,
>>>> >> Nicolai
>>>> >>
>>>> >>
>>>> >>
>>>> >> > So, it's better to have separate fixups during review, but we
>>>> really
>>>> >> > should squash them into their related commits in the series before
>>>> >> > pushing.
>>>> >> >
>>>> >> > --renato
>>>> >> >
>>>> >> > [1] http://llvm.org/docs/DeveloperPolicy.html
>>>> >>
>>>> >>
>>>> >>
>>>> >> --
>>>> >> Lerne, wie die Welt wirklich ist,
>>>> >> aber vergiss niemals, wie sie sein sollte.
>>>> >> _______________________________________________
>>>> >> LLVM Developers mailing list
>>>> >> llvm-dev at lists.llvm.org
>>>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>>
>>>>
>>>> --
>>>> Lerne, wie die Welt wirklich ist,
>>>> aber vergiss niemals, wie sie sein sollte.
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
With best regards, Anton Korobeynikov
Department of Statistical Modelling, Saint Petersburg State University
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200916/2043f417/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image.png
Type: image/png
Size: 25040 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200916/2043f417/attachment.png>