Let’s be sure to distinguish “diff uploads” (which I interpret as, uploading a new diff to an existing review) versus “creating new Differentials” i.e. creating new reviews in the Differential tool. We want to turn off the latter but not the former.
1 Like
Yeah, if we’re able to find a way to allow uploading a new diff for an existing review, but not allow creating a new review, that would be fantastic.
If we can’t do that… I think we should plan to keep Phab up and running in its regular mode until we’ve drained enough of the existing queue, and hope the banner + peer pressure get folks to start using GitHub (though, to be honest, if we find a significant portion of the community stays on Phab at that point, I think it gives us some feedback we should take very seriously; hopefully we avoid that situation though).
Yup, and this is different in that we’re not migrating phab reviews (open or closed) to be GitHub PRs. This is one reason why the static instance of Phab is going to be crucial once we get to that point; losing old reviews would be extremely painful (we could still find most of the information in the emails generated from phab which are still archived, but things like suggested code changes don’t always make it into email form so we’d be losing data).
So long as we still allow uploading new diffs for existing reviews, that seems reasonable. I think Feb 2024 might be viable for Clang, but it’s hard to say (the automatic type inference patch has been ongoing for over a year already).
Perhaps that will be fine? It’s a bit hard to say, the reviews I see people revive are anywhere from this year to 5+ years ago; I’ve also seen people ask plenty of questions on old reviews, like “did you consider or is this a bug?” where it’s not trying to revive the patch but get more info from the people involved with it.
I think if someone wants to ask “did you consider” kind of questions, we can point folks to file an issue on GitHub and give it the question label, linking to the static review page.
Reviving an old review is a bit harder to solve that way; perhaps there’s something we could do on the static page though? e.g., maybe there’s a button that says “email everyone on this review?” and it does a mailto link to whatever email address is associated with the accounts that have subscribed to the thread? Something along those lines might help newer folks because it removes the “but HOW do I contact these people?” problem. Or it could be total overkill. 
It was pr-subscribers-llvm-ir.
This system without automatic approval will have a lot of requests falling through the cracks I guess. GitHub could allow at least members of the top-level LLVM team to join automatically.
Merge-from-main can solve the problem of the context going away.
We used the same system with issues and it worked out OK. After the initial rush, there were only a few requests per month. However, the difference with issues was that we did not have team maintainers so the requests went to llvm-project admins instead. Maybe that would be better?
1 Like
This is really what we’re looking for, see this thread (and patch): Reviews.llvm.org read-only mode - #3 by mehdi_amini
I think so! The probably of emails getting lost would be lower. Plus it avoids us having to deal with people that are on vacation or that abandon the project.
Is it just me or is there no option to cleanup the Git branch after a PR has been merged? It’s something I notice available by default with PRs with other projects on Github It would be nice if that was provided.
Those branches live on the forks of contributors, who are free to delete them at their leisure. The important thing is that the main repo is not polluted by these branches (I just double-checked, it’s not).
I am very well aware of that. Doesn’t change the usefulness of the feature or the fact it’s there by default and for some odd reason it was removed.
In which way is it important for the project what contributors do with branches on their forks?
What reason is there to remove it in the first place?
How is this option removed? I checked on a repo I owned and I can find it. Isn’t this just offered if the person who merges it also owns the fork the branch originates from?
Something I’ve noticed was that it’s not possible to add reviewers for a PR if you don’t have commit bits, e.g. if you’re submitting a first-time contribution.
According to the GitHub docs:
Note: Pull request authors can’t request reviews unless they are either a repository owner or collaborator with write access to the repository.
I can’t find any mention of a setting that would control this, but I thought it might be worth flagging.
Now that the fire hose has been turned on, has anyone identified good strategies for filtering email notifications? I see that the Pull Request Migration project acknowledges the need for a guide for filtering GitHub notifications, but that item is still marked as Todo and the linked issue sadly has nothing to say about it other than “GitHub sends out a lot of notifications” which, I suspect everyone agrees with.
I’ve been looking at email headers and there are some GitHub specific ones present; at least for emails I directly receive (these don’t seem to be present in emails that are sent to cfe-commits).
X-GitHub-Sender: <github-username>X-GitHub-Recipient: <github-username>X-GitHub-Reason: review_requested
Unfortunately, these don’t suffice for my purposes. What I need is a mechanism to prioritize notifications for:
- Issues where I’m specifically listed as a reviewer (by name, not due to being a member of, e.g.,
pr-subscribers-clang). - Comments where I’m specifically mentioned (by @<github-username>).
I use a threaded mail reader on multiple devices and rely on server-side filtering to direct incoming messages to dedicated folders (or /dev/null). My mail server is able to filter on message contents, but I don’t know if that is true in general (e.g., I think older versions of Exchange might not support that).
I’m going to experiment with message body filtering; it looks like it might be possible to catch initial review request notifications and explicit mentions by looking for the following text, but this still doesn’t suffice to filter all notifications for a PR that one is specifically assigned to.
- “requested review from @<github-username>”
- “@<github-username>”
What are others trying?
2 Likes
If we ever do get to a point where new comments on Phabricator will be blocked, we need some recommended practice for adding post-commit review comments on commits tied to Phabricator reviews.
Do people have any experience with commenting on commits (not PRs) on GItHub?
1 Like
You know, I forgot that was still possible. I was figuring, since I have my own fork anyway, the way to do a post-review commit was to create the PR as usual and then immediately merge it (ticking the box to bypass review). Then there’d always be a PR.
I wonder if email filtering tactics ought to be in its own thread, instead of here. This is already a very busy thread.
1 Like
I filter based on the CC field. For mentions there will be Mention <mention@noreply.github.com> or Team mention <team_mention@noreply.github.com> and for explicit review requests, there will be Review requested <review_requested@noreply.github.com>
2 Likes
Except that catches actions on items where you were mentioned in the past. It does not bug you only for new mentions.
1 Like