RFC: Requiring Pull Requests for Contributions

[tstellar]

Proposal

I would like to propose that we require pull requests and disable direct pushes for all contributions to repositories in the llvm organization with the exception of the user/* branches that are used for stacked PRs. The main incentive for this proposal is to enable accurate tracking of authorship of each commit, to enable better auditing during supply chain security incidents. There are a few examples below where we needed this recently. In addition to this, requiring pull requests will make it easier to do post-commit review for all commits.

Authors

• @tstellar
• @kbeyls

Benefits

• This change will make it possible to accurately track authorship of a commit. Currently, the only way to track authorship of commits is by looking at the author field of the git commit. However, this information is supplied by the committer and it is trivial to put another author’s name on your commit. We’ve recently had two[1][2] instances where we needed to search for commits made by a certain github user, and it is impossible to do this without requiring pull requests. Being able to accurately track authorship is important for security auditing.

• It enables better post-commit reviews. We don’t really have a good space to do post-commit reviews on commits that are directly pushed to the repository. Requiring pull requests will fix this by creating a natural space (pull request) to review each commit. This will enable better visibility to the community of which of the “post-commit review” commits actually had any review or not.

• Reverts can be made through the UI. This is a much more convenient way to revert a patch than having to manually do it on a local git checkout. It also makes it easier to track which commits have been reverted, which is useful when backporting fixes or trying to track down regressions.

Impact

To get an idea of how this change would impact the project, on April 9, I took a sample of the latest 100 commits and categorized each commit based on type and if there was a PR or not:

PR	Type	Count
Yes	N/A	60
No	Other	16
No	NFC	13
Yes	Manual Push	4
No	Revert	2
No	Bazel	2
No	GN	1
No	Re-Commit	1
No	Pre-Commit Test	1

From what I can tell, the ‘Other’ category is mainly commits from code owners for changes that qualify for post-commit review. So, the most common use cases that will be affected by this change are this and also NFC changes. In order to minimize the inconvenience of this change on these workflows, we will add a helper script that will submit a PR and automatically merge it, so that these kinds of changes can still be done with a single command on the command line.

Timeline

If this proposal is accepted, then 60 days after its acceptance, pull requests will become required. The first 30 days would be used for developing the script to enable the “create PR and merge” workflow and to work with a few people who make use of this workflow frequently to make sure that it remains easy. On day 31, we will configure GitHub to emit a warning when someone pushes directly, then pull requests will become required on day 60.

[jhuber6]

@tstellar

In order to minimize the inconvenience of this change on these workflows, we will add a helper script that will submit a PR and automatically merge it, so that these kinds of changes can still be done with a single command on the command line.

So, the new standard going forward will be self-approved PRs? Weren’t there some contentions against that in PRs without approvals muddy the waters?

I think the ability to land patches without review is important for development, for example NFC formatting, quick fixes to tests or builds, or just contributing to code where the only reasonable reviewer is the author themselves.

[preames]

I oppose this proposal. I do not feel this is a good tradeoff for the project as whole.

[rupprecht]

To be clear: this will require creating a PR, and require that commits happen by merging that PR, but not add a requirement that the PR has a pre-commit review before merging?

Assuming that, +1 for this change. The benefits outlined are important, and the burden this imposes does not seem to be much.

This also makes it more consistent that post-commit discussion happens on the PR, because now there will always be one.

I frequently commit trivial build fixes w/o pre-commit review, e.g. unused vars in release builds or bazel deps. I usually do those with a direct push, but I would be more than happy to do it via creating a PR and immediately merging that.

@jhuber6

Weren’t there some contentions against that in PRs without approvals muddy the waters?

I believe the only opposition there is the fact that PRs send notifications even though pre-commit review is not intended. There is now a skip-precommit-approval label, although it looks like it still sends notifications, and there is some more work needed per Document use of `skip-precommit-approval` label for non-review pull requests by dwblaikie · Pull Request #81053 · llvm/llvm-project · GitHub.

[asb]

I’d be strongly opposed if we were dropping commits without prior review altogether (especially when there are workflow challenges for this, e.g. it’s common practice to have a commit with precommitted tests and the change that alters the test in one PR, but then you have to directly commit the first patch after review as squash+merge is the only choice).

I don’t love the proposal, but understand where it’s coming from and given limitations with GitHub workflow the solution space is a bit limited. Allowing direct commit via PR with a helper script seems OK, if a little clunky.

@tstellar

It enables better post-commit reviews. We don’t really have a good space to do post-commit reviews on commits that are directly pushed to the repository. Requiring pull requests will fix this by creating a natural space (pull request) to review each commit. This will enable better visibility to the community of which of the “post-commit review” commits actually had any review or not.

For this, I’d point out that GitHub does allow comments on commits and although we’ve lost the flexibility of Herald for subscribing to commits we may want to do post-commit review on, one idea might be to have llvm-bot do an @mention of the relevant foo-subscribers team if a comment is left on a PR to give it a signal boost (or even for every commit that didn’t originate on a PR, but without flexibility to opt-out that’s a bit noisy).

EDIT: I’ll also point out that having PRs doesn’t indicate whether a commit has post-commit review or not. It only provides a different place for people to give comments. So you can tell if people had comments they want to share, but not if anyone took a look and thought it was fine. Its probably going to be the common case that those of us watching commits see a commit fly past without review, see it seems fine, and move on with our lives without going to give an explicit LGTM on a commit page or PR.

[tstellar]

@rupprecht

I believe the only opposition there is the fact that PRs send notifications even though pre-commit review is not intended. There is now a skip-precommit-approval label, although it looks like it still sends notifications, and there is some more work needed per Document use of skip-precommit-approval label for non-review pull requests by dwblaikie · Pull Request #81053 · llvm/llvm-project · GitHub.

My feeling is that subscribers should still be notified even if the commit is not intended to get a pre-commit review. This allows subscribers to monitor all changes that are going into a certain area of the code and also gives them an opportunity to do post-commit review.

[tstellar]

@preames

I oppose this proposal. I do not feel this is a good tradeoff for the project as whole.

Can you elaborate more on why you don’t feel this is a good tradeoff?

[dblaikie]

To the general proposal: I’m in favor of PRs-for-every-commit. My understanding is that that’s the norm on github, paves the way for other options (possibly, but separate discussion for each) improvements.

Such as mandatory precommit automated checks (with some way around it, when necessary, and probably holding the precommit checks to a higher standard in terms of speed and accuracy) and possibly mandatory precommit review (LLVM’s history of changes without precommit review is pretty questionable/don’t think it scales).

@tstellar

My feeling is that subscribers should still be notified even if the commit is not intended to get a pre-commit review. This allows subscribers to monitor all changes that are going into a certain area of the code and also gives them an opportunity to do post-commit review.

Once we move to all-PRs, I’m OK with them all sending email so long as there’s a good way to filter them. When only some no-precommit-review commits were PRs (today) it didn’t seem useful for some no-precommit-review commits to send email and some not. When they all do, then great - I might actually stop looking through the whole commits list every week & just rely on the PR emails - filter the not-for-precommit-review ones to look at less frequently, since they aren’t blocking anyone.

[ftynse]

As someone who regularly does all sorts of direct pushes for bazel, reverts and other documentation fixes, this sounds like a reasonable tradeoff as long as pre-commit review is not required (I believe this is the case, but it isn’t entirely obvious from the top post).

The only use case that looks affected is pushing a stack of commits (typically reverts) in one action, but this can be circumvented by manually creating a PR for those first.

@tstellar

My feeling is that subscribers should still be notified even if the commit is not intended to get a pre-commit review. This allows subscribers to monitor all changes that are going into a certain area of the code and also gives them an opportunity to do post-commit review.

+1 to this. We’ve lost Herald rules for files and github codeowners file provides some equivalence here, but for PRs only.

[asb]

@ftynse

The only use case that looks affected is pushing a stack of commits (typically reverts) in one action, but this can be circumvented by manually creating a PR for those first.

With our current rules, I think you’d need to make one PR per reverted commit as we force squash and merge rather than allowing merging a PR with multiple commits. Overall that’s a good trade-off, but it would cause a bit of friction in the example you mention.

[michaelrj-google]

This seems like a good idea to me, I rely on PRs to get notified when a change has happened.

[efriedma-quic]

Are we sure it isn’t possible to figure out who pushed a commit independent of the authorship information?

I mean, sure, you can’t figure it out by examining the repo itself, but there are other ways to get information about activity in a GitHub repo; for example, REST API endpoints for events - GitHub Docs .

[jankorous]

I support the proposal.

[tobiashieta]

I am in favour of this suggestion.

[tstellar]

@efriedma-quic

Are we sure it isn’t possible to figure out who pushed a commit independent of the authorship information?

I mean, sure, you can’t figure it out by examining the repo itself, but there are other ways to get information about activity in a GitHub repo; for example, REST API endpoints for events - GitHub Docs .

Here is an example API query for a commit I pushed to my fork with a forged author and committer. There was nothing in there that identified me as being involved with this commit:

curl -L -H 'Accept: application/vnd.github+json' -H 'X-GitHub-Api-Version: 2022-11-28' https://api.github.com/repos/tstellar/llvm-project/commits/b1bea3a3689ab17975c3a9a9c0fa1edf18a96187

If someone knows another way to query for the actual user who pushed a commit, I would be interested in knowing about it.

[asb]

@tstellar

If someone knows another way to query for the actual user who pushed a commit, I would be interested in knowing about it.

It looks like you can get this info from a webhook Webhook events and payloads - GitHub Docs

Sadly there doesn’t seem to be a way to get it from GitHub’s logs directly - the audit log may have it on GitHub enterprise but it doesn’t seem to be there for standard orgs on public GitHub.

[boomanaiden154-1]

It seems possible to get the Github username of the person who pushes commits using the event API. Using the following curl invocation:

curl -L \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer <snip>" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/networks/llvm/llvm-project/events

Provides a list of events, with push events looking like the following:

{
  "id": "37863936328",
  "type": "PushEvent",
  "actor": {
    "id": 5361294,
    "login": "alexey-bataev",
    "display_login": "alexey-bataev",
    "gravatar_id": "",
    "url": "https://api.github.com/users/alexey-bataev",
    "avatar_url": "https://avatars.githubusercontent.com/u/5361294?"
  },
  "repo": {
    "id": 75821432,
    "name": "llvm/llvm-project",
    "url": "https://api.github.com/repos/llvm/llvm-project"
  },
  "payload": {
    "repository_id": 75821432,
    "push_id": 18177187301,
    "size": 1,
    "distinct_size": 1,
    "ref": "refs/heads/main",
    "head": "79314c64d0f88fa4ccc1709dee373fdfb9a6ca49",
    "before": "01d7dcfe5e3b261f587c05e33ffeecae7a11da80",
    "commits": [
      {
        "sha": "79314c64d0f88fa4ccc1709dee373fdfb9a6ca49",
        "author": {
          "email": "a.bataev@outlook.com",
          "name": "Alexey Bataev"
        },
        "message": "[SLP]Fix PR90224: check that users of gep are all vectorized.\n\nBefore deleting extractelement instruction for vectorized GEP with\nexternal users, need to check that all users vectorized before deleting\nthis extractelement.",
        "distinct": true,
        "url": "https://api.github.com/repos/llvm/llvm-project/commits/79314c64d0f88fa4ccc1709dee373fdfb9a6ca49"
      }
    ]
  },
  "public": true,
  "created_at": "2024-04-26T18:51:26Z",
  "org": {
    "id": 17149993,
    "login": "llvm",
    "gravatar_id": "",
    "url": "https://api.github.com/orgs/llvm",
    "avatar_url": "https://avatars.githubusercontent.com/u/17149993?"
  }
}

The actor field is what we actually care about here, and includes all the interesting information, particularly an association with the Github user who performed the push. This endpoint by default includes all commits for the network surrounding llvm/llvm-project (so includes forks), but this seems workable to get the information that is desired (username of the Github user actually performing the push).

[asb]

@boomanaiden154-1

The actor field is what we actually care about here, and includes all the interesting information, particularly an association with the Github user who performed the push. This endpoint by default includes all commits for the network surrounding llvm/llvm-project (so includes forks), but this seems workable to get the information that is desired (username of the Github user actually performing the push).

I was looking in the same direction. You can actually query events of a particular repo e.g. curl -L -H "Accept: application/vnd.github+json" -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/tstellar/llvm-project/events though at the time of writing it’s not showing me the push with the b1bea3a3689ab17975c3a9a9c0fa1edf18a96187 commit. The docs do say " Note: This API is not built to serve real-time use cases. Depending on the time of day, event latency can be anywhere from 30s to 6h." so perhaps it will show up later.

[tstellar]

@asb

I was looking in the same direction. You can actually query events of a particular repo e.g. curl -L -H "Accept: application/vnd.github+json" -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/tstellar/llvm-project/events though at the time of writing it’s not showing me the push with the b1bea3a3689ab17975c3a9a9c0fa1edf18a96187 commit. The docs do say " Note: This API is not built to serve real-time use cases. Depending on the time of day, event latency can be anywhere from 30s to 6h." so perhaps it will show up later.

From what I can tell, this is limited to only the 300 most recent events.

[boomanaiden154-1]

@asb

I was looking in the same direction. You can actually query events of a particular repo e.g. curl -L -H "Accept: application/vnd.github+json" -H "X-GitHub-Api-Version: 2022-11-28" https://api.github.com/repos/tstellar/llvm-project/events though at the time of writing it’s not showing me the push with the b1bea3a3689ab17975c3a9a9c0fa1edf18a96187 commit. The docs do say " Note: This API is not built to serve real-time use cases. Depending on the time of day, event latency can be anywhere from 30s to 6h." so perhaps it will show up later.

A little bit unfortunate, but I think for auditing purposes (which is what we would want this for), that’s probably fine.

@tstellar

From what I can tell, this is limited to only the 300 most recent events.

Where are you seeing that restriction? The API Docs seem to indicate that it is paginated and don’t enumerate specific limits on the how many events can be queried. I’m also guessing we could plug into the Github GraphQL API to be more specific about the events that we care about (for example, only requesting push events). Then, even if there is a somewhat low limit, querying once every day, or once every couple days will pick up all the push events.