Contrary to some folks in this thread, personally my experience so far with the transition has been great. While I liked the Phabricator review UI and workflow a lot better, everything else has been much better for me so far. In particular, I find it a lot easier to filter which notifications I’m getting with Github and I can even unsubscribe from specific PRs if I know someone else is handling it. This has solved the biggest problem I had with Phabricator, which is that I couldn’t easily silence some reviews but not others. This led to an overwhelming number of emails, and I find that it’s not nearly as much of an issue now.
So yeah, overall I’m really happy even though the review UI and workflow itself is (IMO) quite inferior to the Phabricator one.
Big picture, moving to pull requests is costing a bunch of productivity at the moment. I hopefully this is going to continue improving, but I suspect I’m going to miss phabricator long term.
My current pain points are:
Lack of a auto-close bot. On phabricator, if we submit a change with the appropriate Differential Revision line, the review is closed. We don’t have the analogy for manually pushed PRs yet.
Stacked reviews. Enough ink has been spilled on this already, I won’t repeat. It is a significant loss.
Email workflow. The recent reply-to header change helped a lot, and I’ve now got a vaguely workable set of email filters, but there’s still lots of room to improve the email usability. I’ve been finding that I have much less idea what’s going on with the project overall (precommit) than I used to. This has already improved some, and I’m hoping it continues to improve.
I do want to make a point of thanking the folks actively working on things. The situation has gotten noticeably better since we started the transition. In retrospect, I wish we’d been a bit slower on the transition (i.e. had a longer overlap period with both being allowed, and messaged as a gradual transition), but well, that’s hindsight.
Phabricator had Active Revisions, Waiting on Review, and Waiting on Authors. On GitHub, it seems like this information is all jumbled together in the Notifications tab. More importantly, I have no idea how to see the Active Revisions equivalent. This has led to people explicitly requesting my review and I did not see it.
I tried to go to GitHub notifications and filter repo:llvm/llvm-project reason:review-requested. However since I am part of pr-subscriber groups, I also get results where I was not explicitly requested for review. In addition, I noticed that I see changes that have been merged or closed. I tried to add filter is:open and that returned no results, even though without that filter, I see open changed that requested review. The is filter works when I click on PRs for llvm/project but not from the notifications tab. I tried using state:open as per Filtering and searching issues and pull requests - GitHub Docs, but it looks like that may not be a valid notifications filter either. Looks like user-review-requested:@me does not work to filter explicit requests for my review. I’m not sure if I am missing something.
As a very, very occasional contributor, I am pretty happy with the transition. There was at least one small patch that I was sitting on for while but submitting to Phabricator was so cumbersome I never bothered doing it; submitting a PR to Github is so much easier.
One point that is rather confusing is that after my patch landed, I got some emails about failed buildbot results. It was a docs-only change so there wasn’t really any way my PR could have been responsible for this. I’ve not seen this in other projects I have contributed to; usually CI notifications go out via github (where they will be subject to my filter rules) as opposed to direct emails (bypassing all filter rules, and also not working with noreply.github.com addresses as has been mentioned above). They also usually go out before the PR is landed; I’m not sure where the post-landing notifications go for other projects (most projects I contribute to use tooling to ensure there never are any post-landing CI failures) but I think it is whoever clicked “merge”, not the PR author.
I realize this is due to the buildbot infrastructure pre-dating the entire move to github, but for people used to github workflows this is foreign. The buildbot emails are written assuming that the recipient knows why they are getting the email and what to do with them, but that assumption simply doesn’t hold true. It would be helpful if these emails started with a paragraph explaining what I am supposed to do now, or maybe a bot should post a message when the PR landed explaining what will happen next.
Already said but: I feel like stacked PRs (despite being a very regular thing, at least for me they are a core part of my workflow) were a complete afterthought. A lot of effort was put into this migration - I know the task wasn’t easy and it’s a job well done - just not on stacked PRs. I already really, really miss Phabricator’s UI with the neat graph showing how different commits fit together.
The worst thing is that I see no good solution. GitHub just doesn’t seem to care about stacked reviews being a thing and you either have to use some external tool to make them work, or push branches to the LLVM monorepo directly which are both horrible solutions IMO. GitHub is also a huge website that inevitably moves slow so I have zero confidence they’ll fix this issue in the short-medium term (if ever at all)
Here is an idea on how we could make things a tiny bit better though:
Make a bot that can understand a command like /stackedpr <N>, it then looks for other PRs associated with the first N commits in the PR, and posts a pretty-printed table with a link to the PR for those commit, along with a comment reminding reviewers that this is a stacked PR and only the last Y commits need to be reviewed. It would at least save the pain of writing that comment ourselves on every review.
Also - this is nitpicky I know but is there another, better GitHub frontend/tool to review PRs? The PRs UI is very casual IMO, I wish there was a more compact view, options such as viewing changes aggregated instead of as commits, filtering out irrelevant activity such as “tag added”, “review requested”, “bot comments”, etc.
I was expecting a single email with both llvm-commits and cfe-commits in the To: - instead the email appears to have been spent twice (despite having the same message body the subject line has different [llvm][clang-tools-extra] tags)
In terms of notifications/subscriptions, I’ve got something that works now - Herald was better, but the bot-based subscriber works well enough (thank you for standing it up so quickly as it became clear the CODEOWNERS based approach wasn’t going to work).
Many have commented on how the review interface in general is weaker than phabricator (which I agree with), but for me my biggest disappointment is that by strongly discouraging force push + rebase we’ve disallowed what was previously best+usual practice:
Rebasing a patch on current HEAD helps ensure that reviewers are reviewing something as close to what will be committed as possible, which helps avoid errors introduced at the last minute when rebasing just prior to commit
Having a whole bunch of fixup commits in a branch is an ugly workaround for GitHub limitations and means I may have to separately maintain the commit I actually want and the version being updated incrementally in the PR (I know local tooling and autosquash can help with this, but it’s still an issue we didn’t have with phabricator)
It also makes it much more awkward to approximate the stacked reviews we had in phabricator. Local scripting could pretty easily handle 1 commit == 1 PR and force pushing updates to appropriate branches linked to separate PRs. I guess this is still possible, but fixup commits make this more complex and make the commit list noisier. Not a great solution for a 10-deep series, but I think it would be OK for 3-4 PRs and it felt like few patch series were deeper than that on phabricator anyway.
I know that GitHub’s isn’t always fantastic in maintaining comment thread context after force pushes, but I do wonder if we’ve really explored this trade-off. For another project I worked on, force pushes when updating a PR were the norm and it seemed OK in practice (even if phabricator’s approach for capturing full pach evolution history was superior).
This gripe about no-rebase and fixup commits, is exactly what I’m disliking (and dreading) as well. To me, the ideal way of dealing with that would be to rebase and squash in changes each time, to keep the branch up to date exactly as it’s desired to end up like.
Also, for some multi-commit series (stacked reviews), keeping it all in one PR but asking reviewers to look at it from a per-commit view, not a whole-PR view, would be most natural to me - although I get that it doesn’t scale too well for really big patchsets.
For committing it, we’d would have to unlock the rebase+merge behaviour though. And that makes it harder to approve individual commits (but verbal comments might be enough even if one doesn’t set the formal approved-flag for the whole PR?). For merging individual commits, it’d require pushing those changes manually from the commandline though, not via github’s UI.
Maybe we’re imagining different MacGyvered/homebrew stacked review workflows, but I don’t think it would be necessary to allow rebase+merge in GitHub’s UI assuming we were ok with rebase+force push to update PRs. Imagine I have a 3-deep series:
PR 1 contains commit A
PR 2 contains commit A+B
PR 3 contains commit A+B+C
If PR 1 and PR2 are reviewed, then I can commit PR1 from the GitHub UI, and then if I wish, rebase PR2 and then commit that from the web UI too (as it now contains just commit ‘B’).
Maybe I’m misunderstanding you - if so, apologies!
Yeah, that would work as well, and probably would be a reasonable compromise.
Ideally, one would just do one PR with commits A+B+C, and tell reviewers to review (and commit) it as three individual commits; GitLab supports such review workflows quite well (although they don’t have an option to just approve/commit a subset of it). But as a compromise with GitHub (without needing to disable our setting of Squash+merge), I guess your suggestion would be ok as well, and vastly better than keeping fixup commits separately!
Also, on the topic of discouraging the use of rebase or not, and whether context of inline commits is lost or not; doesn’t such context disappear after the review anyway, if the PR branch is deleted? (And deleting the branches is kinda recommended anyway, to not litter up the branch namespace in one’s fork - and GitHub provides a button in the PR to allow deleting it after it has been merged.)