Best Way To Merge A (GitHub) Pull Request

Hellogerard

Here at Differential, we're big believers in open-source, both as consumers and producers. As producers, eventually, others will want to contribute to your project. A few days ago, I asked the following of the Twitterverse.{:.commentable-section data-section-id="0"}

Anyone have thoughts on the best way to merge a pull request? Use the button on Github? Locally? Fast-forward? Non-fast-forward? etc.{:.commentable-section data-section-id="1"}

— Gerard Sychay (@hellogerard) May 3, 2013

After briefly scouring the whole of the Internet, I fleshed out three potential methods.{:.commentable-section data-section-id="2"}

Use the Merge Pull Request Button

{:#use-the-merge-pull-request-button}

The built-in GitHub Merge Pull Request button is certainly the fastest and most convenient solution.{:.commentable-section data-section-id="3"}

{:.commentable-section data-section-id="4"}

However, depending on when your contributor forked the project, you may get overlapping merge commits, which nobody likes. Here's an example of that happening (sorry to pick on Slim-Extras):{:.commentable-section data-section-id="5"}

Plus, I don't like the thought of code merging behind the scenes without my controlling every step. There must be a better way.{:.commentable-section data-section-id="7"}

Catch Feature Up with Master by Rebasing, then fast-forward Merge

{:#catch-feature-up-with-master-by-rebasing-then-fast-forward-merge}

IMO, open-source Pull Requests should first be checked out locally for review. The actual commands and sequence varies widely, but I typically run the following:{:.commentable-section data-section-id="8"}

$ git remote add <username><URL>
$ git fetch <username>
$ git co -b <pull-request-branch><username>/<pull-request-branch>

At this point you have the pull request code local and side-by-side with your master branch. You can run tests, diff sets of files, and generally review the changes. If you are satisfied, run git rebase master to rebase your master branch onto the pull request branch. This ensures that any changes you've made to your project since the contributor forked a copy is inserted in a clean fashion to the pull request changes.{:.commentable-section data-section-id="10"}

Now, how to get them into master and close the pull request? You can of course run:{:.commentable-section data-section-id="11"}

$ git co master  
$ git merge <pull-request-branch>

Since you've rebased any recent commits on master onto the pull request branch, merging will fast-foward master. Your history will look like this:{:.commentable-section data-section-id="13"}

That's not bad, but now that everything is a straight line, it's kind of hard to see where the pull request begins and ends. I guess you could look at the author. But what's so bad about merge commits anyway? You can see who and what was committed in a branch. And why not let branches show up in the branch history?{:.commentable-section data-section-id="15"}

Catch Feature Up with Master by Rebasing, then merge --no-ff

{:#catch-feature-up-with-master-by-rebasing-then-merge-no-ff-}

The third option: prepare your pull request branch as above by rebasing, but after checking out master, the last command becomes:{:.commentable-section data-section-id="16"}

$ git merge --no-ff <pull-request-branch>

You'll get something like the following history:{:.commentable-section data-section-id="18"}

The two pull requests are easy to see, as are when they were merged. In both cases of merging locally, after you merge a pull request branch and push it back to Github, Github will close the pull request.{:.commentable-section data-section-id="20"}

In Conclusion

{:#in-conclusion}

In case u care, answer to my pull request question, I've decided to: checkout fork locally, rebase master to fork, merge --no-ff to master.{:.commentable-section data-section-id="21"}

— Gerard Sychay (@hellogerard) May 4, 2013

Note: some of this merging philosophy was explained recently in Git merge vs. rebase.{:.commentable-section data-section-id="22"}