From f5a349558e0854078917b731ad1cba316ae71650 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 30 Jul 2024 20:19:03 -0400 Subject: [PATCH] docs: Document the process for merging pull requests (#5010) --- CONTRIBUTING.md | 220 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 214 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5355878fa79..ceca1eaa6fc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -73,27 +73,235 @@ The source must be formatted according to the style guide below. Header includes must be [levelized](./Builds/levelization). +Changes should be usually squashed down into a single commit. +Some larger or more complicated change sets make more sense, +and are easier to review if organized into multiple logical commits. +Either way, all commits should fit the following criteria: +* Changes should be presented in a single commit or a logical + sequence of commits. + Specifically, chronological commits that simply + reflect the history of how the author implemented + the change, "warts and all", are not useful to + reviewers. +* Every commit should have a [good message](#good-commit-messages). + to explain a specific aspects of the change. +* Every commit should be signed. +* Every commit should be well-formed (builds successfully, + unit tests passing), as this helps to resolve merge + conflicts, and makes it easier to use `git bisect` + to find bugs. + +### Good commit messages + +Refer to +["How to Write a Git Commit Message"](https://cbea.ms/git-commit/) +for general rules on writing a good commit message. + +In addition to those guidelines, please add one of the following +prefixes to the subject line if appropriate. +* `fix:` - The primary purpose is to fix an existing bug. +* `perf:` - The primary purpose is performance improvements. +* `refactor:` - The changes refactor code without affecting + functionality. +* `test:` - The changes _only_ affect unit tests. +* `docs:` - The changes _only_ affect documentation. This can + include code comments in addition to `.md` files like this one. +* `build:` - The changes _only_ affect the build process, + including CMake and/or Conan settings. +* `chore:` - Other tasks that don't affect the binary, but don't fit + any of the other cases. e.g. formatting, git settings, updating + Github Actions jobs. + +Whenever possible, when updating commits after the PR is open, please +add the PR number to the end of the subject line. e.g. `test: Add +unit tests for Feature X (#1234)`. ## Pull requests In general, pull requests use `develop` as the base branch. - (Hotfixes are an exception.) -Changes to pull requests must be added as new commits. -Once code reviewers have started looking at your code, please avoid -force-pushing a branch in a pull request. +If your changes are not quite ready, but you want to make it easily available +for preliminary examination or review, you can create a "Draft" pull request. +While a pull request is marked as a "Draft", you can rebase or reorganize the +commits in the pull request as desired. + +Github pull requests are created as "Ready" by default, or you can mark +a "Draft" pull request as "Ready". +Once a pull request is marked as "Ready", +any changes must be added as new commits. Do not +force-push to a branch in a pull request under review. +(This includes rebasing your branch onto the updated base branch. +Use a merge operation, instead or hit the "Update branch" button +at the bottom of the Github PR page.) This preserves the ability for reviewers to filter changes since their last review. -A pull request must obtain **approvals from at least two reviewers** before it -can be considered for merge by a Maintainer. +A pull request must obtain **approvals from at least two reviewers** +before it can be considered for merge by a Maintainer. Maintainers retain discretion to require more approvals if they feel the credibility of the existing approvals is insufficient. Pull requests must be merged by [squash-and-merge][2] to preserve a linear history for the `develop` branch. +### When and how to merge pull requests + +#### "Passed" + +A pull request should only have the "Passed" label added when it +meets a few criteria: + +1. It must have two approving reviews [as described + above](#pull-requests). (Exception: PRs that are deemed "trivial" + only need one approval.) +2. All CI checks must be complete and passed. (One-off failures may + be acceptable if they are related to a known issue.) +3. The PR must have a [good commit message](#good-commit-messages). + * If the PR started with a good commit message, and it doesn't + need to be updated, the author can indicate that in a comment. + * Any contributor, preferably the author, can leave a comment + suggesting a commit message. + * If the author squashes and rebases the code in preparation for + merge, they should also ensure the commit message(s) are updated + as well. +4. The PR branch must be up to date with the base branch (usually + `develop`). This is usually accomplised by merging the base branch + into the feature branch, but if the other criteria are met, the + changes can be squashed and rebased on top of the base branch. +5. Finally, and most importantly, the author of the PR must + positively indicate that the PR is ready to merge. That can be + accomplished by adding the "Passed" label if their role allows, + or by leaving a comment to the effect that the PR is ready to + merge. + +Once the "Passed" label is added, a maintainer may merge the PR at +any time, so don't use it lightly. + +#### Instructions for maintainers + +The maintainer should double-check that the PR has met all the +necessary criteria, and can request additional information from the +owner, or additional reviews, and can always feel free to remove the +"Passed" label if appropriate. The maintainer has final say on +whether a PR gets merged, and are encouraged to communicate and +issues or concerns to other maintainers. + +##### Most pull requests: "Squash and merge" + +Most pull requests don't need special handling, and can simply be +merged using the "Squash and merge" button on the Github UI. Update +the suggested commit message if necessary. + +##### Slightly more complicated pull requests + +Some pull requests need to be pushed to `develop` as more than one +commit. There are multiple ways to accomplish this. If the author +describes a process, and it is reasonable, follow it. Otherwise, do +a fast forward only merge (`--ff-only`) on the command line and push. + +Either way, check that: +* The commits are based on the current tip of `develop`. +* The commits are clean: No merge commits (except when reverse + merging), no "[FOLD]" or "fixup!" messages. +* All commits are signed. If the commits are not signed by the author, use + `git commit --amend -S` to sign them yourself. +* At least one (but preferably all) of the commits has the PR number + in the commit message. + +**Never use the "Create a merge commit" or "Rebase and merge" + functions!** + +##### Releases, release candidates, and betas + +All releases, including release candidates and betas, are handled +differently from typical PRs. Most importantly, never use +the Github UI to merge a release. + +1. There are two possible conditions that the `develop` branch will + be in when preparing a release. + 1. Ready or almost ready to go: There may be one or two PRs that + need to be merged, but otherwise, the only change needed is to + update the version number in `BuildInfo.cpp`. In this case, + merge those PRs as appropriate, updating the second one, and + waiting for CI to finish in between. Then update + `BuildInfo.cpp`. + 2. Several pending PRs: In this case, do not use the Github UI, + because the delays waiting for CI in between each merge will be + unnecessarily onerous. Instead, create a working branch (e.g. + `develop-next`) based off of `develop`. Squash the changes + from each PR onto the branch, one commit each (unless + more are needed), being sure to sign each commit and update + the commit message to include the PR number. You may be able + to use a fast-forward merge for the first PR. The workflow may + look something like: +``` +git fetch upstream +git checkout upstream/develop +git checkout -b develop-next +# Use -S on the ff-only merge if prbranch1 isn't signed. +# Or do another branch first. +git merge --ff-only user1/prbranch1 +git merge --squash user2/prbranch2 +git commit -S +git merge --squash user3/prbranch3 +git commit -S +[...] +git push --set-upstream origin develop-next + +``` +2. Create the Pull Request with `release` as the base branch. If any + of the included PRs are still open, + [use closing keywords](https://docs.github.com/articles/closing-issues-using-keywords) + in the description to ensure they are closed when the code is + released. e.g. "Closes #1234" +3. Instead of the default template, reuse and update the message from + the previous release. Include the following verbiage somewhere in + the description: +``` +The base branch is release. All releases (including betas) go in +release. This PR will be merged with --ff-only (not squashed or +rebased, and not using the GitHub UI) to both release and develop. +``` +4. Sign-offs for the three platforms usually occur offline, but at + least one approval will be needed on the PR. +5. Once everything is ready to go, open a terminal, and do the + fast-forward merges manually. Do not push any branches until you + verify that all of them update correctly. +``` +git fetch upstream +git checkout -b upstream--develop -t upstream/develop || git checkout upstream--develop +git reset --hard upstream/develop +# develop-next must be signed already! +git merge --ff-only origin/develop-next +git checkout -b upstream--release -t upstream/release || git checkout upstream--release +git reset --hard upstream/release +git merge --ff-only origin/develop-next +# Only do these 3 steps if pushing a release. No betas or RCs +git checkout -b upstream--master -t upstream/master || git checkout upstream--master +git reset --hard upstream/master +git merge --ff-only origin/develop-next +# Check that all of the branches are updated +git log -1 --oneline +# The output should look like: +# 02ec8b7962 (HEAD -> upstream--master, origin/develop-next, upstream--release, upstream--develop, develop-next) Set version to 2.2.0-rc1 +# Note that all of the upstream--develop/release/master are on this commit. +# (Master will be missing for betas, etc.) +# Just to be safe, do a dry run first: +git push --dry-run upstream-push HEAD:develop +git push --dry-run upstream-push HEAD:release +# git push --dry-run upstream-push HEAD:master +# Now push +git push upstream-push HEAD:develop +git push upstream-push HEAD:release +# git push upstream-push HEAD:master +# Don't forget to tag the release, too. +git tag +git push upstream-push +``` +6. Finally +[create a new release on Github](https://github.com/XRPLF/rippled/releases). + # Style guide