Skip to content
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.

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

FIX: genaudit can identify squash-merged pull requests #145

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Oct 10, 2023

So far, we relied on the fact that the Botan repository always merges pull requests with an enforced merge commit. The genaudit script collected those merge commits and used them as ground truth to find all pull requests landed in the audit time frame.

When merging pull requests with the "squash-rebase" strategy, GitHub creates a new commit that combines all commits in a pull request. This is (technically speaking) a commit straight to the main branch.

This pull request allows genaudit to distinguish such "squash-merge" commits from ordinary commits that @randombit created straight on the master branch. We assume that "squash-merge" commits are:

  1. Signed by the GitHub web-flow public key, and
  2. Contain the pull request identifier in their commit message in parenthesis, like: Example: my fancy pull request (#1337)

Limitation: This won't work when merging pull requests with the "rebase" strategy, as this may result in a fast-forward merge and won't adapt the commit messages.

@reneme reneme force-pushed the fix/detect_squash_merged_prs branch from db88f39 to 10564ea Compare October 10, 2023 10:29
@reneme reneme requested review from FAlbertDev and lieser and removed request for FAlbertDev October 10, 2023 10:37
@reneme reneme self-assigned this Oct 10, 2023
@reneme reneme added the bug Something isn't working label Oct 10, 2023
@reneme reneme added this to the Botan 3.2.0 milestone Oct 10, 2023
@reneme reneme marked this pull request as ready for review October 10, 2023 10:38
@reneme reneme requested a review from FAlbertDev October 10, 2023 11:00
@randombit
Copy link

Oh sorry I didn't realize anyone was relying on this. I enabled squash merging momentarily on master to merge randombit/botan#3712 with a squash so it could make it in time for the 3.2 release. In general I prefer merge commits to master so it's clearly traced how a commit made it to master.

@reneme
Copy link
Collaborator Author

reneme commented Oct 10, 2023

I enabled squash merging momentarily on master to merge randombit/botan#3712 with a squash so it could make it in time for the 3.2 release.

That's what I assumed. Anyway, good to know, that we may continue to rely on the PR merge commits. 😄

tools/genaudit/genaudit/repo.py Show resolved Hide resolved
@reneme reneme merged commit bf8cc18 into main Oct 10, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants