Workflow

This chapter contains guidance on how we use Git in general and GitHub in particular in our day-to-day engineering work.

Branching

With very few rare exceptions all work should be done in branches, not master. Branches should be named with a prefix indicating the type of work done, and the GitHub issue ID the work pertains to. For example:

  • feat/100-implement-frobber-support
  • bug/200-gonkolator-tainted-on-refresh
  • scrap/experiement-with-webasm-port

If there is a GitHub ID associated with the work you're doing, it should be in the branch name. Some developers are lazy about this, but @anelson is strict about it.

The prefix on the branch name should indicate the type of work being done:

  • feat - A new feature
  • bug - A bug fix. Sometimes the line between the two is blurry. Use your best judgment.
  • scrap - When working on spikes or prototypes sometimes there is no intention to merge the code back into master.

If you think your task doesn't fit into these categories and a different prefix would better clarify your intentions, then go ahead and follow your best judgment. But ignoring this convention out of laziness or failure to read this section of the handbook will likely get your PR rejected.

Branching off of branches

In rare cases, you need to do some work relative not to the code in master, but someone else's branch. This should be avoided because it has the potential for some truly disastrous Git calamities. If you must do this, make sure you coordinate carefully with whoever is working in your upstream branch, and make sure they know that if they force-push their branch you will have your vengeance, in this life or the next.

Organizing your commits in a branch

When you are not yet ready to submit your branch to a Pull Request, you can do whatever you want in that branch. Force push, merge commits, cherry picking, whatever awful contortions Git will let you perform, you're free to do. We have no rules around this.

@anelson tries to maintain fastidious notes on his work by frequently committing code with meaningful comments on each commit. His reasoning is that this helps to prepare the pull request by having all of the details about the progress that leads up to the final result, close at hand in the commit messages.

Some other members of the team are believers in the "fix", "fix 2", "fix fix fix", "fix 100" school of meaningless commit messages. What you do in the privacy of your own branch is your business (subject to the caveat above if you're sharing the branch with someone else, in which case as a courtesy to your colleages please at least make an effort to be decent).

All of this changes, however, when you're ready to submit your branch to a PR for review by your colleagues. See the next section.

Pull Requests

PRs should be created when a branch is ready to merge, or created as a Draft PR if the code is not ready for merging but you'd like to solicit some feedback (or, alternatively, when you want to trigger CI builds on your branch).

When a PR is ready for someone to review, you are expected to provide a meaningful description. At the very least your description text must explain:

  • What is the purpose of the PR
  • What work was performed. The level of detail needed here depends on the part of the code you're working in, but in general this should be detailed enough to serve as a guide as the reviewer is reading through the diff.
  • Links to the GitHub issue(s) that merging this PR will resolve. These must be in the form "Closes #XXX" where XXX is the issue number. Each issue must be alone on a separate line, with its own "Closes " prefix. This is required so that GitHub will automatically close those issues when the PR is merged.

@anelson's approach is to use the squashed commit message as the PR description. If you've squashed commits down to one commit before creating the PR, GitHub will automatically use that commit as the description.

Preparing a branch for a PR

Before submitting a PR for review, you must squash all commits into a single commit with a meaningful commit message. If you follow @anelson's approach described above, you'll do this before you open the PR so that the PR description automatically is the same as the commit message. If not you'll have to compose the description text separately which seems wasteful.

Squashing commits serves two purposes:

  • It makes it easier to know which commits after the PR is created need to be re-reviewed.
  • It is a reminder to the developer to write a single, coherent, comprehensive commit message describing the work done.

If you do not squash your commits before you request a review, the reviewer is within their rights to refuse to perform the review until you fix this mistake. If your reviewer is @anelson you're practically guaranteed to have your review rejected.

The best way to perform this squashing, and clean up your commit message(s) at the same time, is to use interactive rebase (git rebase --interactive). If you're not familiar with this, read up on it before your first attempt. It can be intimidating the first time or two, particularly if your git-fu is not strong.

Note that as with most rules, there's an exception. In some cases, organizing your changes into multiple commits serves to better present your changes to the reviewer. For example, a big refactor that happens in multiple steps might make sense to present with one commit for each of the steps, letting the reviewer review a step at a time or collapse those steps into the final result. Use your best judgment as to whether your particular changes should be presented this way. Good faith deviation from the squash rule will not be rejected.

Rebasing

Up until the time when you have requested a review, you can rebase whenever you want. After you have requested a review, you should never rebase, unless you must do so to resolve a conflict with master. The reason why rebasing after a review starts is considered bad (to say nothing of rude) is because after a rebase, GitHub's PR UI will no longer allow a reviewer to look only at changes made since the last review. This is hugely disruptive, particularly for large PRs with a lot of changes.

That said, if your branch has a conflict with master, GitHub cannot build your PR (because PR builds are done by doing a local trial merge with master first), and you will not be able to merge even if your PR is approved. In this case you have no choice but to rebase and force push, but realize that your reviewing colleagues may secretly (or not-so-secretly) hate you as a result.

Making changes based on PR comments

Normally you'll submit an initial PR with one squashed commit for review, and then as you get feedback in the review you'll need to make changes. These changes should NOT be squashed or force-pushed. Each round of changes should be in a separate commit, with a meaningful summary of what changed.

This is really important for the reviewers. See the above section "Rebasing" about why this is so important.

Merging to master

When merging to master after a successful approved PR, always use a fast-forward merge and always squash the commit. Most of our repositories are configured to require this, such that GitHub won't allow you to perform the merge incorrectly.

If you have followed the guidance above about squashing and rebasing for your PR this will be automatic, but even if for some reason you did not, this merge guidance applies. Specifically, do not put extraneous merge commits into master. The purpose of this principle is to ensure the change log on master is a log of features implemented, bugs fixed, etc. Not arcane minutiae reflecting which team members merged which branches into which other branches during development.

Note that by default GitHub will generate a commit message that is a concatenation of all of your commits since the initial PR. This will include commits containing changes based on PR feedback. Most of the time, there's no informational value in these commits, so you should edit them out of the final commit message before merging to master. Obviously if any of these commits make material changes to the resulting code, that guidance doesn't apply, and you should include all relevant details about those changes in the final commit.

Commit messages

Adhere to the standard git commit message convention when writing commit messages. This is absolutely required for the commits merging PRs into master, but it is a good practice for all commit messages. Even if you are pretty sure a commit message will end up getting squashed into another message, it's quite useful when doing your interactive rebase to be able to scan the list of commits and understand immediately which one does what.

Never break the build on merge

You may do whatever you want in your branches during development, including breaking the build in spectacular ways. However, a branch cannot be merged into master until the CI server has both successfully built the branch, /and/ successfully built the branch with an experimental merge into master. You don't have to do anything to verify this; the CI system pushes the results of both builds into the PR. All you have to do is make sure you don't merge until these are both green.

This is important not only to avoid disrupting the work of your colleagues if they merge your changes from master into their feature branches, but it also allows workflows using git bisect to search the history to identify when a particular bug was introduced.