Effective Pull Requests

A pull request is a way of tracking a request to make changes to a code repository. It can be used for merging changes from one repository to another, as in an open source project where many people make contributions, or from one branch to another within the same repository, which is more commonly used for getting peer code review and approval within a single team.

The concept of a pull request is general and GitHub and Bitbucket both have similar implementations. The Bitbucket pull request is described in the tutorial. Most of this post is about generally applicable principles, but specific details and examples relate to Bitbucket because that is what we use at Adzuna.

As the pull request creator

Make the purpose of the pull request clear

If you create a pull request, it will most likely be assumed that you have a submission you want merged, and this should be the default assumption. However, it’s also convenient to make pull requests early in development to get feedback on whether you are heading in the right direction at all. In this case, or if your pull request is for any purpose other then to work towards merging the contents into the target, it’s important to make that clear.

At Adzuna we have a convention of prefixing the pull request title with “PREVIEW” or “WIP” to show that we are asking for early feedback and not actually planning to merge the code for some time. This is to distinguish from a code ‘review’, a common step required in team workflows before releasing code.

Don’t send out the pull request too early — create privately first and add reviewers later

When a pull request is created, the reviewers get an email. And they get emails for every comment, approval, decline or task added to the pull request. And every commit, merge or rebase on the branch the pull request refers to.

As a result of this, you don’t want to send out the pull request too early. Firstly you want to check that your branch contains what you think it does and review it yourself to see if you spot any mistakes. For this purpose, we recommend creating the review request without any reviewers first. If it’s a small one, you can simplify this by checking the diff in the “diff” tab of the Create Pull Request page.

Check the branch passes in CI (Jenkins) first

If practicable, we suggest not sending out a review request until the branch is passing in your continuous integration system. We use Jenkins and it notifies BitBucket of which commits have passed and failed.

This verifies that you have not forgotten to commit a file, which is making it work on your machine but nowhere else. However, in some cases the continuous integration server can be quite busy. This may introduce a delay in your development cycle so we do sometimes skip this step to get faster peer feedback.

Tick the ‘close source branch when the pull request is merged’ box

This means your branch will be removed from the central repository when merged. The commits will remain until garbage collected, and it will be easy to recreate your branch from the merge commit. In addition, you and others will have a copy of the branch until everyone has run a git remote prune.

The benefits of this are:

• it makes it easier to see the current work in progress by listing branches
• avoids someone accidentally committing to a branch after its been merged

Rebase the branch before sending out the request to reviewers

Rebasing or merging can be use to pick up changes in the codebase since you branched off. We prefer rebase as it gives a clearer view of history. You can also use rebase interactive to rewrite the history on your branch, for example:

• Make better commit messages, or fix mistakes in hasty commit messages from development
• Squash together lots of commits. For example, if you make a mistake and fix it during development, you don’t really need to record this fact for ever in master

Rebasing should be done with care and you will need to do a forced push, which can destroy work done by someone else on the same branch. The git option--force-with-lease is a slightly safer option designed for pushing rebases without removing new commits — but it is still experimental and it’s important you only rebase branches you “own” or have co-ordinated with others. We recommend the following in your git config to help with this:

[alias]

fp = push --force-with-lease

rbi = rebase -i remotes/origin/HEAD

The use of remotes/origin/HEAD means we rebase on what we last knew was on the remote, without needing to remember to pull into our local copy of master.

Approve your own pull request

Really? Yes, approve it when you believe that you’ve met all the requirements and are about to merge it. Then do any final rebase, check if there’s a change freeze, etc and merge it.

Merge using the merge button

The pull request has a merge button. It’s really convenient to use, avoiding the need to checkout master, pull, merge and push.

As long as you rebased reasonably recently, the merge should not have conflicts. If there are conflicts, the merge button will be disabled and a warning box will tell you about the problem.

Conflict resolution can be tricky and introduce bugs. Rather than try to resolve conflicts on merge, it’s better to rebase the branch to remove the conflicts there, and do a clean merge (after re-testing the branch to make sure your conflict resolution was correct).

Apart from convenience, the merge button also enables:

• requirement for approvals before merging
• requirement for successful builds before merging

These can be rigorously enforced if you disallow direct pushes to the mainline branch. This can be useful if lots of people are contributing to a repository. A few ‘super users’ can be allow direct pushes to do emergency releases or resolve broken merges.

Close stale pull requests

When merged, pull requests go into a ‘closed’ state and no longer appear by default on the ‘pull requests’ page. If you have a pull request that for some reason is not going to be merged, we recommend closing it with the ‘decline’ button so that everyone knows it is not planned to be merged. This saves time for anyone who comes across it and might think about reviewing it, but more importantly it cuts down the number of open pull requests so there is a clear view of what is waiting to be reviewed.

When you see that there is something waiting to be reviewed, try to make it a priority to review it and provide feedback or approve it, so that the the amount of time each piece of work takes from beginning development to release is reduced.

As the reviewer

Don’t approve until you’re happy for it to be merged

Don’t press the approve button until you are happy for the pull request to be merged. Obvious but important.

Make comments with any queries or suggestions

If code is messy or unclear, or there looks like a bug, make a comment. Either attach the comment to the line of code where it appears, or add a comment to the pull request as a whole if you want to query something broader.

Create a ‘task’ for any blockers, but be sparing

Some comments on pull requests are optional suggestions, or requests for clarification. However, some look like bugs or are important enough to require clarification before the pull request is merged. For these, we create a ‘task’ (using Create Task after commenting) to make it clear that this is a blocker that you expect to be fixed or clarified before approval.

This also signals to other reviewers that there may be an issue, which they may miss and press approved without noticing it.

This feature should be used only when the issue may be a major bug or misunderstanding of the task in hand. It shouldn’t be used to encourage overengineering or scope creep.

Don’t increase the scope or encourage overengineering

The pull request might be a first step towards a goal. It is good to break things into small steps. Don’t expand the story/task into the whole epic at the pull request stage with lots of suggestions. Likewise, think carefully about possible bugs or redesigns. Is it really an issue that stops this pull request being merged, or a possible future enhancement? Judicious use of tasks vs comments will help with this.

Decline if the whole concept is wrong or the pull request is stale

The decline button should be used if the whole pull request is wrong. This could be that it is implementing a feature that is wrong, doing it in completely the wrong place or if it has gone stale and the owner has failed to decline it themselves. In general it is better if the owner declines the pull request but help keep the list of open pull requests small if necessary by declining old ones.