Skip to content

Commit 460a06a

Browse files
committed
Add some GitHub commit message guidance
A common theme in PR reviews has been discussing the level of detail that should be included in commit messages, as well as how to craft a series of commits in a PR that helps guide a reviewer through the changes made to accomplish the overall goal of the PR. This adds some text we can point to on this topic that provides some helpful explanations and examples of commit message and commit series best practices. Signed-off-by: Russell Bryant <rbryant@redhat.com>
1 parent c6a9cab commit 460a06a

File tree

1 file changed

+41
-1
lines changed

1 file changed

+41
-1
lines changed

docs/github-merge-strategy.md

+41-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,46 @@ This document describes the merge strategy used for Pull Requests within all rep
66

77
Every Pull Request that is made to an InstructLab repository should meet the below requirements - certain repositories such as [taxonomy](https://github.com/instruct-lab/taxonomy) may have additional requirements.
88

9+
### Commit Messages and PR Description
10+
11+
PRs should be formatted as a series of logical commits that implement the
12+
overall change proposed in the PR. Each commit should represent a logical step
13+
in the progression of the change. The commit message for each should clearly
14+
explain that step of the progression.
15+
16+
Here are a few examples of PRs that contained a series of commits:
17+
18+
- <https://github.com/instructlab/instructlab-bot/pull/125/commits>
19+
- <https://github.com/instructlab/instructlab/pull/994/commits>
20+
- <https://github.com/instructlab/instructlab/pull/951/commits>
21+
22+
Here are some external resources that provide guidance on writing good commit messages:
23+
24+
- <https://www.berrange.com/tags/commit-message/>
25+
- <https://docs.kernel.org/process/submitting-patches.html#separate-your-changes>
26+
- <https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#smaller-is-better-small-commits-small-pull-requests>
27+
28+
While not a requirement right now, we may want to consider following a spec like
29+
[Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) in the
30+
future. It's worth a look for inspiration, at least.
31+
32+
When your PR contains multiple commits, it is helpful to reuse the work you put
33+
into crafting high-quality commit messages. Here is a script you can use to generate
34+
a summary of the commits in your PR. It starts with a list of the commits one
35+
line at a time and then follows that with the full commit messages. The format works
36+
for pasting directly into GitHub.
37+
38+
```sh
39+
#!/bin/bash
40+
41+
git log --reverse --oneline origin/main..HEAD
42+
echo
43+
git log --reverse origin/main..HEAD
44+
```
45+
46+
Here is an example PR that uses a PR description that was generated using this
47+
script: <https://github.com/instructlab/sdg/pull/67>.
48+
949
### CI checks
1050

1151
We should require that all CI checks pass on a Pull Request before it can be considered for merge. Every repository should have at mimimum the following checks:
@@ -28,7 +68,7 @@ There are [three different merge methods offered by GitHub](https://docs.github.
2868

2969
We use the default merge method of creating merge commits for PRs. This is to ensure we retain the full commit history as intentionally structured by the PR author while also retaining metadata about the PR itself in the merge commit.
3070

31-
This requires project maintainers to include commit messages and the overall structure of the commit series as part of their review. When multiple commits are present, they should represent a logical series of changes that implement the overall change proposed in the PR. The commit message for each should clearly explain that step of the progression.
71+
This requires project maintainers to include commit messages and the overall structure of the commit series as part of their review.
3272

3373
It is common that a PR author may need to do a final rebase to clean up their proposed commit series before a PR can be merged. It is also fine for a project maintainer to perform this step when the changes necessary are straight forward enough to do so. This includes doing a final rebase on `main` if necessary. The PR itself should NOT include any merge commits of `main` back into the developer's branch. We expect the proposed commit series to be a clean set of commits against `main` without conflicts or merge commit history. We only use a merge commit to record the PR's inclusion into `main`.
3474

0 commit comments

Comments
 (0)