Optimize Your Pull-request

Exciting to see a flurry of activity on the Simple.Web repository including some awesome pull-requests. Managing these has however reminded me how hard it is the keep a clean commit history, and more so how important it is to clearly communicate your expectations to contributors.

Good

So what IMHO makes a good pull-request?

  • Changes have not been made against the master branch
  • Commits are made against a new branch used for pull-request
  • Branches are named appropriately (e.g. feature-X, fix-Y)
  • Only contains commits directly relating to the change
  • Concise description of changes and it’s motivation
  • Branch has been rebased often and not merged
  • Contains tests that confidently verifies functionality
  • Commits are squashed into as few commits as appropriate
  • Commit messages written in the imperative (e.g. “Implement” vs “Implemented”)
  • When PR merged new changes are made against a new branch

Example

#master> git fetch upstream/master              (alternatively you could ..
#master> git rebase upstream/master                just 'git pull --rebase')
#master> git checkout -b feature-owinsupport
#feature-owinsupport>

   ... 8 commits ...

#feature-owinsupport> git checkout master
#master> git fetch upstream/master
#master> git rebase upstream/master
#master> git checkout -b fix-issue290
#fix-issue290>

   ... 1 commit ...

#fix-issue290> git fetch upstream/master         (make sure nothing has ..
#fix-issue290> git rebase upstream/master           come in since)
#fix-issue290>

   ... Make pull-request on GitHub to master ...

#fix-issue290> git checkout feature-owinsupport
#feature-owinsupport>

   ... 2 more commits ....

#feature-owinsupport> git fetch upstream/master         (make sure nothing has ..
#feature-owinsupport> git rebase upstream/master           come in since)
#feature-owinsupport> git rebase -i HEAD~10             (squash my commits)

   ... pick the 1st commit, squash the rest ....
   ... edit the description when prompted to ONLY "Implement support for OWIN 1.0."

#feature-owinsupport> git push origin feature-owinsupport

   ... Make pull-request on GitHub to master ...

#feature-owinsupport> git checkout master

   ... Pull-request has since been merged ...

#master> git fetch upstream/master                      (yours changes will ..
#master> git rebase upstream/master                       now be in master)

    ... House-keeping (optional) ...

#master> git push origin :feature-owinsupport           (remove your remote branch)
#master> git branch -D feature-owinsupport              (if you are sure they're merged)

I have used fetch/rebase (or git pull --rebase) in the #master branch examples above. You could of course do a ‘merge’ with git pull on master given it should only ever have to fast-forward.

Of course there are instances where the apparently easy process above isn’t so easy, including the example of making further changes to a branch already pull-requested, or working with repository owners on an upstream branch. Please do ask “what should I do if … ?” questions in the comments below.

Support

We obviously want your pull-requests to be merged smoothly as possible, and want to help you know if once merged it’s not going to break anything, and so the Simple CI server (which anyone can log into as guest) automatically;

  • Builds your pull-request in isolation, on Mono and .NET, running all tests
  • Builds a merged version of your pull-request in isolation, on Mono and .NET, running all tests

This allows all of us to know if your PR behaves as expected, and will continue to do so once merged.

That’s all for now folks! :–)

Comments