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

build: add a script to merge a PR according to our current workflow #868

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

coriolinus
Copy link
Contributor

We have decided to require both signed commits and a linear history in github, which breaks the github "merge" button completely.

This script does the right thing to accomplish signed commits with a linear history, because it's kind of a pain to do manually each time.

What's new in this PR


PR Submission Checklist for internal contributors
  • The PR Title
    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@coriolinus coriolinus requested a review from a team as a code owner January 20, 2025 15:06
Copy link

github-actions bot commented Jan 20, 2025

🐰 Bencher Report

Branchprgn/build/merge-script
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
18.70
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.83
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
9.40
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
11.88
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
15.14
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
16.92
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
989.30
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
7.03
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
84.54
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
220.08
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
426.49
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
676.43
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
117.76
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
28.41
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
46.43
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
61.50
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
79.89
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
95.87
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold
19.92
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold
115.47
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold
35.73
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold
56.88
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold
76.25
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold
100.90
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
27.57
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.65
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
8.86
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
11.87
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
17.06
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
21.78
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
29.85
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
137.14
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
115.11
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
93.71
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
71.71
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
50.42
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
136.83
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.95
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
33.48
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
59.56
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
86.45
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
111.31
🐰 View full continuous benchmarking report in Bencher

scripts/merge-pr.sh Outdated Show resolved Hide resolved
scripts/merge-pr.sh Outdated Show resolved Hide resolved
# ensure that the branch is at the tip of `origin/main` for a linear history
git fetch
git checkout "$branch"
if ! git rebase origin/main; then
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a prime use case for git merge-base ---is-ancestor. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I was looking for something like that, but google wasn't helpful. I think this behavior, which does the rebase as a side-effect, is actually better though.

There are 3 cases we care about:

  1. The branch already originates from origin/main
  2. The branch originates from some older commit than origin/main but applies cleanly.
  3. The branch originates from some older commit than origin/main but does not apply cleanly.

git merge-base --is-ancestor will do a great job distinguishing case 1, but conflates cases 2 and 3. We do want to distinguish between those.

git rebase as written here conflates 1 and 2, but distinguishes case 3; I think that's the behavior we actually want.

Copy link
Member

@istankovic istankovic left a comment

Choose a reason for hiding this comment

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

Looks good, with a few smaller things.

@coriolinus coriolinus force-pushed the prgn/build/merge-script branch from 753e491 to 993a743 Compare January 20, 2025 16:27
We have decided to require both signed commits and a linear history
in github, which breaks the github "merge" button completely.

This script does the right thing to accomplish signed commits with
a linear history, because it's kind of a pain to do manually
each time.
@coriolinus coriolinus force-pushed the prgn/build/merge-script branch from 993a743 to 815cc85 Compare January 20, 2025 17:17
@coriolinus coriolinus merged commit 815cc85 into main Jan 20, 2025
33 checks passed
@coriolinus coriolinus deleted the prgn/build/merge-script branch January 20, 2025 17:17
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.

2 participants