As has been noted before, the commits lists have become much noisier with the move to GitHub PRs. If you don’t subscribe to those lists, you won’t see absolutely everything that happens on the project, but you will still get GitHub emails for the PRs you are subscribed to.
My initial email filters (for stuff to throw away) look as follows. Most (but not all) of it seems to be coming from our github actions mailer, which is the one that sends mail to cfe-commits, llvm-commits etc. Caveat, I haven’t had these filters in place for a huge amount of time but so far they seem to be catching the dross and keeping the comments.
FTR I’m on Outlook/Exchange. The syntax doesn’t match anything in particular, hope it’s clear.
I added the “https:” to keep the filter from catching review comments with one of those words in it. The actual body syntax is <URL> <keyword> <URL>.
The “review_requested” might look important, but AFAICT this email doesn’t report whose review was requested; it could be anyone. I think the event is just that a reviewer was added to the PR, which makes this email not really useful.
Rule 2. GitHub sends an email to subscribers whenever the author does a push, and for every person/team added to the reviewer list; I decided I don’t care about those, except the ones I specifically subscribe to.
From: notifications@github.com
AND CC: push@noreply.github.com or review_requested@noreply.github.com
UNLESS body contains "requested your review" or (a team name I want to know about)
Worth noting that if you are subscribed to a PR, you will get TWO emails per comment, one from notifications@github.com and one on the -commits list. It’s easier to identify the GitHub version, which is “CC: comment@noreply.github.com” (instead of “push” as in Rule 2).
Edit: Updated rules after a couple of days experience with them.
+1 For libc++ we also try to clean up our review queue but we have some patches that have been in active development for a long time. We already ask people to move newer not-yet reviewed patches to GitHub.
At the moment we’re commandeering and finishing old patches. Unfortunately this takes a bit of time and the current Phab issues make this process less smooth.
Indeed, I was actually planning on asking for a similar extension to the Phabricator read-only deadline for libc++. We’re working as fast as we can to drain our review queue, but there’s years of history in there. IMO discouraging new code reviews on Phabricator (or enforcing that) would be better, since we can still work on finishing open patches but all new patches would basically end up on Github, which is really what we want.
After consulting with @MaskRay on what is possible, I propose this timeline:
Oct 1: Prevent the creation of new differentials (code reviews)
Nov 15: Prevent the upload of new patches to existing differentials
Dec 15: Stop importing new commits from github
Remain in this state for at least 6 months. Comments are allowed.
The goal here is to greatly ramp down the maintenance burden of Phabricator by 2024, and also avoid doing anything significant over the holidays. As shown in this recent thread, a large chunk of the load on Phabricator comes from importing commits from git. This includes closing active code reviews. The hope is that we can maintain Phabricator in a comment-only mode for a long time at very low cost.
This will not address the use case of maintaining long term code review continuity. Specifically, for those long-running, year+ language feature implementation reviews, they will have to be reuploaded as pull requests. However, it will extend the time available for folks to comment on old issues and link forward to pull requests that continue the patch or reopen an old review.
Does that seem workable for reviewers who are draining their review queues?
Shall we look into this when we get close to Nov 15. and reevaluate? Ideally the number of revision that gets actively worked on would get naturally very small before we get to turn off the ability to finish the reviews.
We’re reviewing PR as a single unit, and we recommend pushing individual commit for addressing comments, this is at odd with a “rebase and merge” workflow where many individual commits would land. This also does not allow to review the “final commit message” (since there isn’t really one).
For Clang, time will tell. I think Nov 15 may be unrealistically short to finalize reviews on Phab, but folks are actively trying our best to clear Phab reviews first. It’s too soon to tell if we’re making sufficient headway to make that date or not. I’ll speak up as early as I can if I think I have a more firm answer one way or the other.
IMO, that’s not reasonable to ask of code reviewers. Please consider reviews like this: ⚙ D140828 [C++] Implement "Deducing this" (P0847R7) where there are hundreds of comments in the review. Expecting that kind of review to move to GitHub and losing that amount of context is simply not workable. It’s not hard for the patch author to make the PR, but it’s basically impossible to tell “have you addressed all my concerns” from a code review perspective.
@rnk, your proposed timeline and rationale does not appear to acknowledge the needs expressed in the immediately preceding comments by @ldionne and @mordante or by @AaronBallman further up thread.
Yes, this seems workable for libc++. Thanks a lot for being flexible about the original plan.
I think Clang might be in a different boat, but for libc++ I think this timeline is fine – at least from my perspective. Our goal is definitely to be completely off of Phabricator within the next few weeks, and while we might not meet October 1st, November 15th should be OK.
I hear this concern, and this review was something I considered when proposing the timeline. I understand that the transition is disruptive and is creating a lot of work for reviewers. So, first let me say I’m really grateful for all the work from you and everyone else that has gone into this transition so far.
I have two main goals:
Drive the migration to GitHub pull requests and avoid fragmenting the code review ecosystem. Update the documentation and make sure that new contributors don’t have to think about our legacy Phabricator code review tool.
Reduce the maintenance cost of running Phabricator before the 2023 holidays. This has recently created a surprising amount of unplanned work, and I want to reduce that.
For now, I want to wait and see how the migration is going. We can re-evaluate and look at open differentials in early November.
One option on the table is to disable git imports and continue to allow updating existing differentials. The theory is that it should be “cheap” to leave Phabricator running as a diff upload service with commenting functionality. I think it will make it difficult to rebase patches and update them, which I imagine you will want to do for long reviews.
I also wanted to point at the incremental development section of the developer policy, and to suggest that it seems completely appropriate to checkpoint this work by landing it under a cc1 flag. I personally would not want to iterate on a single code review with hundreds of comments. I would rather find a way to break things up into smaller patches. I acknowledge there are absolutely costs to doing things this way. Flags are not free, they create complexity and maintenance overhead. I just want to say that there are options.
On this topic of long-running reviews, @alina mentioned that the pointer provenance patch series by @dobbelaj is another example of something where we might lose review continuity.
As the author, of this patch both me and Aaron are committed to progress it as fast as we can, but I’m more than happy to continue that work on phab.
Neither losing the conversation, nor commiting something half baked look like desirable outcome. The feature is already gated on C++26, which few people are going to use on production, but we still need to make sure nothing breaks. Yet another flag we would have to keep forever is not going to help.
I do expect the amount of impacted diffs to be small, and we are working as fast as we can to drain the queue of small reviews, but reviewers are in short supply. (deducing this is taking time because we struggle to find the time to focus on it, same for any large/complex enough PR)
I am actually trying to encourage you to consider committing something half-baked behind a flag. That’s actually OK! Nothing says we have to keep cc1 flags forever. Developing flagged-off functionality in-tree is a classic incremental development pattern, and we do it all the time. Many of the recent debug info improvements come to mind.
I wonder if I can help on this front. I’ll follow up on Clang Frontend on Discord.
Can we do something to make this timeline (or approach, at least, if the dates aren’t fixed) better publicised? I keep seeing people migrating revisions with months worth of context to GitHub PRs, thinking they’re being helpful and doing the thing we the community want of them, and have to tell them they misunderstood and unfortunately wasted everyone’s time doing so. Perhaps by updating the big red banner on reviews.llvm.org to clarify that existing revisions should stay put?
The two biggest reviews I was concerned about have both landed (deducing this and automatic type inference) and follow-up work will continue on GitHub.
We’re making good progress and I’m hopeful we can make the Nov 15th deadline, but I’m still requesting flexibility given how much effort remains.
Aside-but-related: how are the efforts going with making the “static” version of Phabricator? I’m hoping the community can see what this looks like so we can provide feedback before the cutoff date so that this part of the transition can go smoothly.