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.

Sign up for GitHub

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

conventions: write good commit messages #339

Merged
merged 1 commit into from
Apr 3, 2019
Merged

conventions: write good commit messages #339

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions engineering/repositories.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ Each repository should belong to a team, and the full team has admin rights over
* A maintainer can merge his own PRs without code review, but it is encouraged to ask for a review Merging without review, if done, should be reserved to fixing typos or minor maintenance tasks.
* If a maintainer is missing, the Lead of the team that the project is owned by, or a backup maintainer designated by him will act as maintainer.

## Commit messages

Commit and Pull Request descriptions should be clear and detailed. Keep in mind that commit messages will be read by (eventually) a lot of people:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any preference about both (PR desc vs commit message)?

I use to put most of the effort on explaining the change in the PR description because it is easy to link other issues, commits... create examples, headings...
...and less in commit messages (in my case, they're almost a phrase using imperative tense to describe the change to apply)

I know that PR descriptions can be lost if the repository is migrated, or forked, or moved to bitbucket... and commit messages are forever... so, should we focus much more on commit message? or can we rely on GitHub PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

(eventually) a lot of people:

and processed by 🤖

Copy link
Contributor

Choose a reason for hiding this comment

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

In my view, both are very important. Each commit in the PR should document the change made by that particular commit, and the PR overall should document why these commits are together.

Because the scope of individual commits and PRs is often quite different, the level of detail might vary. However, I would generally argue that they should have a similar structure:

  1. A one line synopsis of what the commit/PR accomplishes

  2. A more detailed explanation of why and what is being changed.

For tiny commits it may be OK to omit (2), particularly if it's updates from a code review. But even then there can be value—for example:

Updates from code review.

Fix spelling and punctuation errors, and reformat C files with clang-format.

I find the hardest thing about writing good, clear commit and PR descriptions is getting out of the present moment, where I know as much as I will ever know about this particular issue—and putting myself in the future, when someone else will have to reconstruct it. There is no simple formula for that, obviously—what I try to do is anticipate the questions someone is likely to ask me and answer them up front. YMMV.

Most (all?) of our projects seem to prefer not squashing PRs when they are merged to master. I'd argue that it's therefore important that even our little code-review commits speak for themselves. In a repo where commits are expected to be squashed (even if not ahead-of-time), the PR description and the merge commit can basically be the same description.

Either way, though, the general advice proposed in this PR seems good.


- Code reviewers will read them to have context on what they are reviewing, to understand why the change was needed and why it was done in a certain way.
- Any developer might eventually get there when investigating (e.g. git bisect) a regression. They might get there long after the change is introduced and the context forgotten.
- Maintainers might go back to it when writing release notes.
- Any user might read it later to understand a change that affects them.

Take some time to explain what and why you did something. It will save a lot of time to everyone, including yourself.

## Issues

* Maintainers should review GitHub issues and answer them in less than a working day. An answer might be just a "Thank you, we'll look into it" as minimum.
Expand Down