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

Wed Sep 16 06:33:08 PDT 2020

See https://docs.reviewable.io/registration.html#github-authorizations where
they document what they do with the permissions they request.

On Wed, Sep 16, 2020 at 4:36 AM Anton Korobeynikov <anton at korobeynikov.info>
wrote:

> 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/764184b1/attachment-0001.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/764184b1/attachment-0001.png>