I think that “warning” comment could be sufficient, specially in case when commits to PR are done with valid email, but PR would be merged with hidden email.
Status checks can be made blocking, if you’re able to have an action that comments with a warning about it you can in fact block merging the PR automatically.
Can you have blocking status checks for PRs while still also allowing manual pushes to the repo? I didn’t think this was possible.
It looks like the old branch protection mechanism can’t allow bypassing status checks, but that the new rulesets feature might?
We could add a bypass for everyone with commit access, but it’s not clear if that would apply only to manual pushes or also to pull requests. If the bypass did apply to pull requests, then that would defeat the purpose of it.
To revive this old thread. Definitely something should be done here. Use of private addresses also impacts the post-merge workflow as buildbot certainly is unable to notify such users should breakage happened.
We are seeing quite decent traffic of unroutable emails from buildbot to private GitHub addresses.
I think we might have at least a github action that would check the email and post comment if it is a private one.
This should be address by this work in progress though I believe: Build bots to report build statuses to github - #14 by mehdi_amini
Reporting status in PR will mean that everyone who was subscribed to the PR will be informed.
Only if the change comes via PR. So far there might be direct pushes with no PRs behind them. Requiring “proper” emails is not only buildbot-related thing (also mentioned as 3. in RFC: New criteria for commit access - #72 by ldrumm etc.).
OK but the “hidden emails” feature isn’t affecting direct pushed though, and no amount of GitHub action can help 
Yes, you are right. But at least we might encourage people to unhide the emails via PR comment. So, notifications are related but a bit different here.
I forgot about this thread until @MaskRay pointed me at it again after I merged a new contributor’s PR (they didn’t have commit access). I remember now when I clicked the squash and merge button that it gave me a preview of the email details that would be included. I should have paid more attention to it and flagged it up, but I didn’t, so the commit landed with bad email addresses.
I think there’s nearly full consensus that we should be mandating that proper email addresses are included in the author information: @j2kun seems to be the sole exception from a quick skim, with, I acknowledge a legitimate concern, but I feel like the health of the organisation outweighs this concern. As a workaround (and I admit it’s not a great one), you could always disable the private address option temporarily whilst you land your changes and then re-enable it afterwards.
Adding a comment to PRs when a commit is added without a correct email address seems like a good idea to me (it seems to be having the desired effect for clang-format, in my experience, for example), but I’d also suggest a few other things:
- Make sure the Getting Started docs for LLVM explain that a real email address is required for all contributions (and point out how to make sure this happens in GitHub).
- Make a broader announcement to the LLVM community to advise people of this policy, and how reviewers can make sure it is enforced.
- Add a comment after the squash and merge has happened if the issue is detected again (to resolve the “email address was overwritten” issue).
- I think you can add something that GitHub will report back to a user who does a direct push with the “wrong” email address style. It won’t stop them but at least it might encourage them to fix things (especially if it happens when they push to their original branch for the PR). Obviously, this won’t detect fake emails though.
1 Like
FWIW, I recently got merge permissions so now I can select which email I’d like to display when I’m merging.
1 Like
Even though Mehdi indeed brought this once up with me (and then I forgot), I did not realize the implications of enabling/disabling this setting (and I don’t even remember actively changing it). Now that I know, I have fixed it and it seems to have picked this up in my latest PR push…
$ git log | grep Aart
Author: Aart Bik <ajcbik@google.com>
Author: Aart Bik <39774503+aartbik@users.noreply.github.com>
Author: Aart Bik <39774503+aartbik@users.noreply.github.com>
Author: Aart Bik <39774503+aartbik@users.noreply.github.com>
...
2 Likes
We still don’t warn users, here are examples of PR that landed with a hidden email just now:
Yeah, for some reason action did not run
The second PR was submitted back in September when there was no such check. The first one should work, but the workflow did not run, for some unknown reason it did not trigger…
For reference, I also exclusively use the private e-mail address on GitHub. I cannot afford putting up my personal e-mail address publicly on GitHub as from experience it yields a ton of spam e-mail from bots that scrap git e-mail addresses. Besides, what is the difference between this e-mail address and a junk e-mail address the committer does not use?
1 Like
I think it’s definitely good policy to have the bot leave a comment suggesting/requesting a public email be used, but I’m not sure it’d be useful to require one, for the reason you give: it would just make people put a junk address instead.
1 Like
Are you monitoring the “junk” address?
Right now the only way to get notified by the build bots of a build breakage is through emails (I wish the infra would comment on pull-requests…).
So if you don’t have an email you’re monitoring, how do you know if you’re breaking the build?
(I’m using the same email address for LLVM contribution ~forever and I haven’t noticed much spam, I consistently get ~30 spams per month, all correctly filtered by gmail).