diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5355878fa79..2f0826dbd37 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -73,6 +73,35 @@ 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 one commit with a +[good commit message](#good-commit-messages). +In some cases, it is appropriate to organize the changes into +multiple commits. In that situation, all of the commits should be +distinct changes and have +[good commit messages](#good-commit-messages). + +### 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 @@ -94,6 +123,157 @@ 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 fee 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. +* 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 +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 +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