Update on GitHub pull requests

[MaskRay]

@mehdi_amini

Just checked, for the web based UI, we could have Apache2 redirect Login

But arc is using the same API for creating and editing a revision, so this would require some changes to Phab code itself: ⇵ differential.revision.edit

(Unfamiliar with conduit methods)
Edit src/applications/differential/conduit/DifferentialConduitAPIMethod.php ? Is anyone interested able to write a patch?

edit: created Reviews.llvm.org read-only mode for the discussion.

[MaskRay]

@NekoCdr

Apparently flag is just implemented and used in a couple of places (18 usages). Very likely this feature is far from being complete.
first patch:

Thank you for investigation! I created a new post Reviews.llvm.org read-only mode for the discussion.

[jh7370]

I don’t think it’s necessarily a good idea to disable new patches on Phabricator just yet, even though most of them should be on GitHub now. The reason is that there are occasionally patch series where new patches are added/spun off etc into new patches, which are based on other patches that are still under review in Phabricator. As these patches won’t have equivalent git branches in somebody’s fork, a new PR cannot be based on top of an existing Phab review, so either they have to push their still in-review-on-Phab patches to a GitHub repo and go from there or they have to create a PR that also includes these commits too. Neither of these seem ideal.

I’ve seen at least one case where somebody hit this, and chose to create a new Phabricator patch, precisely to avoid these issues.

I think we can just do a soft reviewer-led rejection of new patches on Phabricator for now at least, i.e. if somebody asks for review on a new Phabricator patch, unless there is a good reason (such as the above) to review it there, the reviewer should ask the patch creator to create it in GitHub instead. I suspect there won’t be many people in this situation anyway, what with the bright red banner at the top of the screen.

[nlopes]

I tried to subscribe to one of these new ‘pr-subscribers-xx’ teams, but the request is pending.
Aren’t we free to subscribe to whatever PR groups we are interested? If there’s an intention to use these teams as mandatory reviewers, then maybe we need a separated set of teams that can be freely subscribed by anyone.

Another question: what’s the process to decide on the maintainers of these teams? I didn’t see any discussion about this (but may have missed, ofc).

Thanks!

[tstellar]

@nlopes Which teams have you tried to subscribe to? So far, the person who requested the team is being made the maintainer. I’m not sure there is a good way to make self-subscribe work for the teams, unfortunately.

[pogo59]

@nlopes

Another question: what’s the process to decide on the maintainers of these teams? I didn’t see any discussion about this (but may have missed, ofc).

For the debug-info team, Tom made me the maintainer because I was first to suggest it. I added the code owners as maintainers, which I think is the right thing to do for most teams. Code ownership might not be obvious in some other cases though.

[MaskRay]

@nlopes

I tried to subscribe to one of these new ‘pr-subscribers-xx’ teams, but the request is pending.
Aren’t we free to subscribe to whatever PR groups we are interested? If there’s an intention to use these teams as mandatory reviewers, then maybe we need a separated set of teams that can be freely subscribed by anyone.

For a pr-subscribers-xx maintainer (approver for the purpose of patch subscription, not related to authority), it seems that GitHub only presents the “pending request” information on the https://github.com/orgs/llvm/teams/pr-subscribers-* page, not on any other page (confirmed by @boomanaiden154-1)

I don’t think we can expect a maintainer to visit https://github.com/orgs/llvm/teams/pr-subscribers-* pages they manage to check new pending join requests routinely. So if they miss the email notification would like to join "LLVM"'s "pr-subscribers-llvm-binary-utilities" team, the request may stay pending forever.

The current best way is perhaps to have more maintainers for the pr-subscribers-xxx teams. I guess every maintainer will receive a would like to join "LLVM"'s "pr-subscribers-llvm-binary-utilities" team email notification (with the high volume of llvm-project emails, I think the error rate is not very low), but it’s probably better than neglecting an email notification. Unfortunately it seems that a maintainer cannot give maintainer state to another member. Perhaps only administrators can do this.

[tstellar]

OK, that’s interesting, because I get email notifications for the issue-subscribers-* teams when someone wants to join. I’m’ not sure why it doesn’t do the same for the pr-subscribers-* teams.

[MaskRay]

As I requested many teams when the system was set up, I have gotten a lot of email notifications. To prevent from my missing “would like to join XXX” email notifications and to decrease my workload, consider changing the maintainer for the following teams

https://github.com/orgs/llvm/teams/pr-subscribers-llvm-lto/ @teresajohnson

https://github.com/orgs/llvm/teams/pr-subscribers-lld-coff/ @mstorsjo

https://github.com/orgs/llvm/teams/pr-subscribers-lld-macho/ @int3

https://github.com/orgs/llvm/teams/pr-subscribers-lld-wasm/ @sbc100

https://github.com/orgs/llvm/teams/pr-subscribers-sanitizer/ @fmayer

I have also invited some members which I think active to these teams, in case they do not know this notification system. Sorry if I failed to invite some folks.

[pogo59]

@tstellar

OK, that’s interesting, because I get email notifications for the issue-subscribers-* teams when someone wants to join. I’m’ not sure why it doesn’t do the same for the pr-subscribers-* teams.

Note that @MaskRay said “if they miss the email notification” not that the notification doesn’t happen. It does.

[rnk]

@AaronBallman

There is basically no chance this deadline will work in practice for Clang. For example, I have several long-running patches under review in Phabricator, …
…
One possible solution could be to put Phab into a mode where new reviews cannot be initiated in Phab, but comments and uploading new revisions of existing reviews are still allowed,

I’m in favor of adjusting the plan from “make Phab readonly” to “disable the creation of new diffs”, I think that accomplishes the goal of directing folks to use the new system going forward. When the schedule was being discussed, it wasn’t clear if this was an option, and we planned assuming it wasn’t.

Would that plan work for you and Clang?

@AaronBallman

Tangentially, there’s a problem that’s become more obvious to me in the past week: resurrecting old patches is a thing people still regularly do:

Thanks, that does seem like a real issue. To try to relate this to the bugzilla migration, in that case, we actually did a full migration of all the ongoing issues, and there was an implicit forward link from bugzilla to github via llvm.org/prNNN .

In the short term, if the plan to disable new diff uploads succeeds, comments will be open, and this won’t be an issue. We’ll need to set a new timeline for going readonly, and just to throw something out, I’d suggest February 2024 (4 months after October), but I haven’t gathered any consensus on that.

In the long term after comments are disabled, I do expect folks will want to do as you suggest: reopen long closed patches, and without comments, there won’t be a way to link forward from the Phab review to the PR, we can only link backwards from the PR to the Phab review. That’s not the best, but is that something you can live with? I think six months out, we’ll be living in the new world, and we’ll be out of the habit of commenting on Phabricator.

[pogo59]

@rnk

if the plan to disable new diff uploads succeeds

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.

[AaronBallman]

@rnk

I’m in favor of adjusting the plan from “make Phab readonly” to “disable the creation of new diffs”, I think that accomplishes the goal of directing folks to use the new system going forward. When the schedule was being discussed, it wasn’t clear if this was an option, and we planned assuming it wasn’t.

Would that plan work for you and Clang?

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).

@rnk

Thanks, that does seem like a real issue. To try to relate this to the bugzilla migration, in that case, we actually did a full migration of all the ongoing issues, and there was an implicit forward link from bugzilla to github via llvm.org/prNNN .

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).

@rnk

In the short term, if the plan to disable new diff uploads succeeds, comments will be open, and this won’t be an issue. We’ll need to set a new timeline for going readonly, and just to throw something out, I’d suggest February 2024 (4 months after October), but I haven’t gathered any consensus on that.

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).

@rnk

In the long term after comments are disabled, I do expect folks will want to do as you suggest: reopen long closed patches, and without comments, there won’t be a way to link forward from the Phab review to the PR, we can only link backwards from the PR to the Phab review. That’s not the best, but is that something you can live with? I think six months out, we’ll be living in the new world, and we’ll be out of the habit of commenting on Phabricator.

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.

[nlopes]

@tstellar

@nlopes Which teams have you tried to subscribe to? So far, the person who requested the team is being made the maintainer. I’m not sure there is a good way to make self-subscribe work for the teams, unfortunately.

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.

[reinterpretcast]

@tobiashieta

I would say that maybe rebasing should be used sparingly until the change is fully reviewed unless a rebase is required to pass the review (passing tests etc.).

Merge-from-main can solve the problem of the context going away.

[tstellar]

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?

[mehdi_amini]

@pogo59

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.

@AaronBallman

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.

This is really what we’re looking for, see this thread (and patch): Reviews.llvm.org read-only mode - #3 by mehdi_amini

[nlopes]

@tstellar

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?

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.

[BradSmith]

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.

[h-vetinari]

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).