Like many F/OSS developers, I’m a heavy user of GitHub, collaborating on many projects which use the typical “fork & pull” workflow based on pull requests. The GitHub documentation on pull requests covers this workflow fairly comprehensively, but there seems to be one area which is significantly lacking in detail: why and how to amend existing pull requests. The article simply says:
After your pull request is sent, any new commits pushed to your branch will automatically be added to the pull request. This is especially useful if you need to make more changes.
The problem is that this completely ignores the fact that there are often very good reasons for amending existing commits within the pull request, not just for adding new commits it.
Why amend an existing pull request?
A peer review cycle can potentially reveal many issues which make the pull request unready for merging, e.g.
- bugs in the proposed code changes
- missing features in the proposed code changes
- incomplete test coverage
- incomplete documentation changes
- style inconsistencies (including whitespace issues)
- incorrect or incomplete commit messages
- the commits violate the rule of one logical change per commit
- some changes are outside the scope of the pull request
This is of course what makes peer review of pull requests so valuable: the problems can be addressed even before they hit the
master branch, which helps maintain high quality in the development trunk. But then how do we address the issues?
How not to amend your existing pull requests
One approach is for the submitter fix the issues in their original local branch, close the pull request, and submit a new one. However in most cases this is a bad idea, because each time you do it, you create an “orphaned” abandoned pull request, and there is no automatic linking between the new revised one and the abandoned one, which makes it a lot harder to follow the history of the review process.
Another approach (the one apparently implied by the GitHub documentation mentioned above) is to append “fixup” commits to the series which fix issues introduced by commits earlier in the series. But that’s almost always a really bad idea, because again it violates the rule of one logical change per commit. In particular:
It makes life much harder for reviewers, because they have to review a combination of broken commits and subsequent commits which are supposed to fix that brokenness. So for each change, they have follow this process:
- Review it as normal.
- If they spot brokenness, refrain from immediately commenting on it, and instead look ahead for any subsequent commit in the series which may claim to fix that brokenness.
- If such a commit is found, verify that it really does fix the brokenness.
- If it does fix it, selectively ignore that brokenness, whilst taking care not to accidentally ignore other brokenness in the same area.
This problem is significantly compounded each review cycle, i.e. each time fixup commits are appended to the pull request.
The only workaround is to review via the “Files changed” tab, but that brings considerable disadvantages when the pull request contains more than one commit, because then the reviewer cannot review individual logical changes one at a time.
- After merging the pull request, it pollutes the project’s history with useless noise: fixup commits which are not linked to the commits they are fixing.
- It breaks
git revert, and
The right way to amend your existing pull requests
The good news is that GitHub actually supports a much better alternative which works really nicely. As a maintainer and reviewer of many GitHub repositories, I’ve been continually surprised how many people are unaware of this simple trick; perhaps it’s due to the hole in GitHub’s pull request documentation which I mentioned above? The trick is simply this:
- Rewrite the history of your local branch corresponding to the pull request, so that the issues revealed during the review of the pull request are addressed.
- Do a force push (i.e. a
git pushcommand which includes the
I won’t go into details here about how to rewrite history.
git history rewriting via
git rebase (with
git commit --amend etc. is something of an art form, and there are already many great resources out there which there would be no value in me replicating.
So really this whole blog post is a long way of saying:
When you force-push a new head for a pull request branch, GitHub handles it gracefully and automatically updates the pull request in the way you want.
It’s almost like magic. For example, if a reviewer commented on two hunks in the original pull request, and your force-push effectively rewrites one of those hunks but not the other, then the GitHub pull request UI will get updated to say something like “@reviewer commented on an outdated diff” for the rewritten hunk, but any outstanding review comments on the changed hunk will be presented as before the force-push.
Now don’t get me wrong – GitHub is not perfect. In some (but not all) ways, it is inferior to Gerrit’s review system which is better at exposing the complete history of the review cycle. I previously wrote another blog post examining GitHub’s review system in more depth. But in general, it’s usually plenty good enough.
A footnote about git’s force-push
If you’ve previously been fed the mantra “git force-push is evil, don’t ever use it”, or the more specific version “don’t ever force-push to public branches” then hopefully you now realise neither are correct. There are many perfectly valid use cases for force push, otherwise the extremely clever developers who implemented it wouldn’t have bothered (and let’s face it, they know far more about
git than you or I ever will). Ultimately the validity / safety of doing a force push to a public branch depends on the audience’s expectations for the stability of that branch. If people expect it to be stable and never get rewritten (only fast-forwarded), then you shouldn’t force push, or at least not without a decent effort to correct those people’s expectations. However there is generally no reason why people should expect GitHub pull request branches to be stable, because people generally don’t base other branches on top of them.
The article entitled “The Dark Side of the Force Push” may help further your understanding.
Taking over amendment of someone else’s pull requests
Note that the title of the previous sections referred to your existing pull requests. GitHub doesn’t let you amend anyone else’s pull requests, and that’s a good thing for obvious reasons. But what if one developer submits a pull request, vanishes off on holiday, and whilst they are away, it gets reviewed and issues are revealed?
In this case it’s easy to effectively take over the process. Simply create a local branch which points to the same commit as the head of the pull request. If you have the totally awesome
hub CLI tool installed, then this is as simple as:
git checkout https://github.com/github/hub/pull/134
Then amend your branch as necessary according to the existing reviews, and submit a new pull request which references the old pull request it’s superceding, either by URL or by number. You will probably also want to arrange for the old pull request to be closed with a comment linking to its successor.