Skip to content

Conversation

@davidmrdavid
Copy link
Contributor

Related to: https://discourse.llvm.org/t/forbidding-username-in-commits/86997

Context:

When we merge a commit including tag to another username (e.g. @<someUser>), that account will receive an email / notification every time that PR is cherry-picked and pushed to a fork, generating a lot of spam in the user's inbox. As of today, there's no way of disabling this on GitHub settings.

This PR documents this error in our developer policy, and reminds contributors to avoid it.

Next steps: I'm a big believer that any policy that is not enforced via automation is not enforced at all, so I'd love to see some kind of check to prevent this error. However, there does not seem to be an agreement to do that yet. I'll see if I can gather support for doing that.

…rname in commit messages and PR descriptions
@davidmrdavid
Copy link
Contributor Author

Although I could merge this, I want to wait until I get an approval from a 'docs owner'. Not sure who that'd be though.

@boomanaiden154
Copy link
Contributor

Although I could merge this

Having commit access does not let you land changes without review for the most part, although as a social policy, not a programmatically enforced one. From the developer policy:

You are granted commit-after-approval to all parts of LLVM. For information on how to get approval for a patch, please see LLVM Code-Review Policy and Practices. When approved, you may commit it yourself.

You are allowed to commit patches without approval which you think are obvious. This is clearly a subjective decision — we simply expect you to use good judgement. Examples include: fixing build breakage, reverting obviously broken patches, documentation/comment changes, any other minor changes. Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to. Also, try to separate formatting or whitespace changes from functional changes, either by correcting the format first (ideally) or afterward. Such changes should be highly localized and the commit message should clearly state that the commit is not intended to change functionality, usually by stating it is NFC.

Given you have commit access I'm assuming you've already read that, but just wanted to make sure since the wording you used here confused me a bit.

@davidmrdavid
Copy link
Contributor Author

Thanks @boomanaiden154. Yes, agreed. I would not merge without a review; I was just trying to state that I was looking for a reviewer, and that I had commit access to merge once approved.

My apologies for the confusion!

@davidmrdavid
Copy link
Contributor Author

@AaronBallman: I added you as a reviewer, as I noticed you in the commit history you made some relatively recent changes here, and GitHub suggested so as well. If there's a better owner (or a different etiquette to finding a reviewer in this part of the codebase), please let me know and feel free to unassign, thank you!

@kuhar kuhar requested review from dwblaikie and nikic October 21, 2025 00:20
included in the submitted code.

* Avoid 'tagging' someone's username in your commits and PR descriptions
(e.g. `@<someUser>`), doing so results in that account receiving an email
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have almost all email notification disabled but still receive github notifications. Maybe s/an email/a notification/`?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, replaced here: 9a42646

thanks!

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these sort of PRs, where there's clear consensus on the approach in the corresponding Discourse thread, I'd add those involved in the discussion. I don't believe these sort of docs have a particular owner.


* Avoid 'tagging' someone's username in your commits and PR descriptions
(e.g. `@<someUser>`), doing so results in that account receiving an email
every time the commit is cherry-picked and to a fork.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "and" here seems misplaced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, yes. That was a typo. It should read: "[...] the commit is cherry-picked and/or pushed to a fork".

Fixed here: f8cdebc

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the preceding wording nits.

@davidmrdavid
Copy link
Contributor Author

I'd add those involved in the discussion

I will do so next time, thank you for the guidance.

@nikic - can I'd appreciate a final pass now that the feedback is incorporated, so I can merge. Thank you!

@davidmrdavid davidmrdavid requested review from kuhar and nikic October 21, 2025 16:13
@davidmrdavid
Copy link
Contributor Author

davidmrdavid commented Oct 21, 2025

Thanks all Unfortunately buildkite is being flakey, and I don't have the permissions to retry those checks (as far as I can tell), so I'll have to push another commit ( a git pull ) to force the check to re-run.

@davidmrdavid davidmrdavid requested a review from kuhar October 21, 2025 17:44
@davidmrdavid
Copy link
Contributor Author

davidmrdavid commented Oct 21, 2025

SInce I made another push (although the diff has not changed). @kuhar can you help me with a final re-approval? I'll merge shortly after. Thank you.

@davidmrdavid davidmrdavid merged commit 3728ac7 into llvm:main Oct 21, 2025
14 of 15 checks passed
@davidmrdavid davidmrdavid deleted the dev/dajusto/document-that-usernames-should-not-be-in-commit-messages branch October 21, 2025 22:08
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Oct 29, 2025
…rname in commit messages and PR descriptions (llvm#164328)

Related to:
https://discourse.llvm.org/t/forbidding-username-in-commits/86997

**Context:**

When we merge a commit including tag to another username (e.g.
`@<someUser>`), that account will receive an email / notification every
time that PR is cherry-picked and pushed to a fork, generating _a lot_
of spam in the user's inbox. As of today, there's no way of disabling
this on GitHub settings.

**This PR** documents this error in our developer policy, and reminds
contributors to avoid it.

**Next steps:** I'm a big believer that any policy that is not enforced
via automation is not enforced at all, so I'd love to see some kind of
check to prevent this error. However, there does not seem to be an
agreement to do that _yet_. I'll see if I can gather support for doing
that.

---------

Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
aokblast pushed a commit to aokblast/llvm-project that referenced this pull request Oct 30, 2025
…rname in commit messages and PR descriptions (llvm#164328)

Related to:
https://discourse.llvm.org/t/forbidding-username-in-commits/86997

**Context:**

When we merge a commit including tag to another username (e.g.
`@<someUser>`), that account will receive an email / notification every
time that PR is cherry-picked and pushed to a fork, generating _a lot_
of spam in the user's inbox. As of today, there's no way of disabling
this on GitHub settings.

**This PR** documents this error in our developer policy, and reminds
contributors to avoid it.

**Next steps:** I'm a big believer that any policy that is not enforced
via automation is not enforced at all, so I'd love to see some kind of
check to prevent this error. However, there does not seem to be an
agreement to do that _yet_. I'll see if I can gather support for doing
that.

---------

Co-authored-by: Jakub Kuderski <kubakuderski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants