Skip to content

DeveloperPolicy: Update commit access requirements #127006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 21, 2025
Merged

DeveloperPolicy: Update commit access requirements #127006

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5492cf
DeveloperPolicy: Update commit access requirements
tstellar Feb 13, 2025
b610f21
Update DeveloperPolicy.rst
tstellar Feb 15, 2025
53ebac0
Fix Typo.
tstellar Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions llvm/docs/DeveloperPolicy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,11 +539,13 @@ What are the expectations around a revert?
Obtaining Commit Access
-----------------------

We grant commit access to contributors that can provide a valid justification.
If you would like commit access, please use this `link <https://github.com/llvm/llvm-project/issues/new?title=Request%20Commit%20Access%20For%20%3Cuser%3E&body=%23%23%23%20Why%20Are%20you%20requesting%20commit%20access%20?>`_ to file
Comment on lines -542 to -543
Copy link
Member

@ldionne ldionne Feb 18, 2025

Choose a reason for hiding this comment

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

I'm a bit late to the party, but I would suggest something like the following introductory sentence:

LLVM operates using Github Pull Requests. In most cases, contributing to the project does not require commit access, as Pull Request reviewers can perform the merge once ready. However, contributors are eligible to obtain direct commit access to the repository once they have 3 or more merged pull requests. To do so, you may use this link <...>

This clarifies that one can actually go a long way without requiring commit access.

Copy link
Collaborator Author

@tstellar tstellar Feb 21, 2025

Choose a reason for hiding this comment

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

@ldionne I've submitted this as a follow up change here: #128244

Once you have 3 or more merged pull requests, you may use this
link <https://github.com/llvm/llvm-project/issues/new?title=Request%20Commit%20Access%20For%20%3Cuser%3E&body=%23%23%23%20Why%20Are%20you%20requesting%20commit%20access%20?>`_ to file
an issue and request commit access. Replace the <user> string in the title
with your github username, and explain why you are requesting commit access in
the issue description. If approved, a GitHub invitation will be sent to your
the issue description. Once the issue is created, you will need to get two
current contributors to ack your requests before commit access will be granted.
Copy link
Member

@JDevlieghere JDevlieghere Feb 13, 2025

Choose a reason for hiding this comment

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

Maybe "to support" instead of "to ack"?

Copy link
Member

@rengolin rengolin Feb 13, 2025

Choose a reason for hiding this comment

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

Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."

Copy link
Contributor

@asb asb Feb 13, 2025

Choose a reason for hiding this comment

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

Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."

I think that risks giving the impression that more is required than having had 3 PRs merged, and there's not consensus on that.

I do think it would be helpful to give advice like "Once the issue is created, you will need to get two
current contributors to ack your requests before commit access will be granted. It is recommended that you CC reviewers of your merged patches in order to provide this approval."

Copy link
Collaborator Author

@tstellar tstellar Feb 13, 2025

Choose a reason for hiding this comment

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

Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."

We have automation which posts a contribution summary when someone applies for commit access and notifies people who have reviewed or committed their patches. See #127077 (comment)

Copy link
Contributor

@asb asb Feb 13, 2025

Choose a reason for hiding this comment

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

I've been reflecting on this some more, and please consider this 100% non-blocking as I know it's been a long process...

I think it remains unclear what criteria the reviewer uses to give the 'ack'. If the sole requirement is that 3 patches were committed then it would be almost automated and surely one person double checking there was no mistake (e.g. checking for reverts) would be fine. But if more consideration is required, what is it? If one group of committers applies very different criteria to another, what do we do?

This is a step in the right direction, but it feels like we've not really succeeded in resolving the ambiguity of commit access requests unless that's clarified.

Copy link
Collaborator Author

@tstellar tstellar Feb 13, 2025

Choose a reason for hiding this comment

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

@asb It's a good question. For me, the ack means 1) I know this person and 2) I believe they will adhere to our Developer Policy/Code of Conduct.

Copy link
Contributor

@asb asb Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it would be uncontroversial to say something like:

"Reviewers of your committed patches will automatically be CCed upon creating the issue. Most commonly these reviewers will provide the necessary approval, but approvals from other LLVM committers are also acceptable. Those reviewing the application are confirming that you have indeed had three patches committed, and that based on interactions on those reviews and elsewhere in the LLVM community they have no concern about you adhering to our Developer Policy and Code of Conduct."

Copy link
Collaborator Author

@tstellar tstellar Feb 15, 2025

Choose a reason for hiding this comment

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

"Reviewers of your committed patches will automatically be CCed upon creating the issue. Most commonly these reviewers will provide the necessary approval, but approvals from other LLVM committers are also acceptable. Those reviewing the application are confirming that you have indeed had three patches committed, and that based on interactions on those reviews and elsewhere in the LLVM community they have no concern about you adhering to our Developer Policy and Code of Conduct."

@asb Sounds good to me, I've added this paragraph to the document.

If approved, a GitHub invitation will be sent to your
GitHub account. In case you don't get notification from GitHub, go to
`Invitation Link <https://github.com/orgs/llvm/invitation>`_ directly. Once
you accept the invitation, you'll get commit access.
Copy link
Member

@ldionne ldionne Feb 18, 2025

Choose a reason for hiding this comment

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

I can't comment on the lines below directly, but the following paragraph should probably be updated too:

Prior to obtaining commit access, it is common practice to request that
someone with commit access commits on your behalf. When doing so, please
provide the name and email address you would like to use in the Author
property of the commit.

In fact, I think it can be removed entirely, but we may want to keep the bit of information about a valid author name and email address if we don't discuss it anywhere else.

Expand Down