In this post I’ll explain what we consider a good pull request and how we deal with merging them. If you’ve worked with any project that leverages pull requests, you will notice we aren’t doing or saying anything all that unique. However how we merge PRs into our source tree may not look familiar.
Before we get too far, let’s review what the typical contribution process looks like. First, contributors fork the repo to create their own copy. Then they create a branch off the dev branch (the name of the branch does not matter). Contributors then do their work on that branch and when work is complete, they push it to their fork.
Before creating and submitting the PR, you need to make sure your branch has all the commits from the current dev branch in your branch. This is referred to as making sure you are in sync. Why does this matter? Because if you are not in sync when your PR is submitted, there’s a fairly good chance that your contributions could conflict with the code in dev. In our case, if there are conflicts, your PR will be kicked back and you will be asked to fix the conflicts.
How do you sync you branch? Easy… first you need to jump over to the dev branch & make sure it is current with the upstream (the original repo, not your fork… your fork is origin):
git checkout dev # update your local dev to be a mirror of what's in the main repo git pull --rebase upstream dev
Now, jump back over to your branch where you are doing you work (we’ll call that feature-branch) & rebase it. What does that mean? Think of it like this. Let’s say you originally created the branch form dev on Monday. It is now Thursday and changes have been going on since Monday. What you are doing when you rebase is effectively saying “act like the start date for feature-branch was really Thursday.”:
git checkout feature-branch git rebase dev
You may get conflicts or merge conflicts. If you don’t, hey all good. But maybe you get conflicts. These are good to find before submitting the PR because if you didn’t do this, you’d get the shame of the rest of the contributors for having your PR kicked back because of conflicts found when merging. OK, just kidding… but wouldn’t you want to deal with them first?
I said a similar thing in the last post about commits where each commit should have single responsibility. This is one rule we don’t budge on - a single commit should only close a single issue, not multiple issues. The only exception is if one issue stems from other issues like there’s some cascading regression. But that’s rare in my experience.
If a contributor submits a PR that includes a commit that closes multiple issues, we insist that the commit is broken up into separate commits that address each item. Take a look at this PR: https://github.com/ngOfficeUIFabric/ng-officeuifabric/pull/201 .
As you can see from the screenshot it closed three issues as it did three things (add a new directive and refactor two other directives) but there’s only a single commit. This is not what we want… instead what I did was break up the commit, create three separate commits for each action taken and reference the appropriate issues:
Breaking up a commit is actually pretty easy. This is effectively you saying “undo the last commit action, but don’t change the code, just act like the commit never happened”… think of it as a commit is a glass jar containing a bunch of commits. You are breaking the glass jar and left with it’s contents you need to repackage.
If it’s the most recent commit you want to address (which if you follow the process I outline blow in the section “Merge PR’s Manually” it should be because you isolate the PR from other code in it’s own branch) you simply break the most recent thing on your HEAD:
git reset HEAD~1
Now you are left with all the files that were in the commit with changes applied. You can see this by running
git status. At this point you selectively.
If you need to go further back in the tree, then you can do something like
git rebase -i HEAD~3 where
3 is how many commits back it is. In the interactive session you find the commit you want to break up and change it from
edit, save & exit. Now the rebase will stop just after the commit you want to edit so now you run
git reset HEAD~. Repeat the process above in committing each file(s) and adding the correct message. When finished, continue with the rebase (
git rebase --continue) which will take you back to your branch (if it doesn’t you have additional cleanup work to do, but following these steps you should not have any).
This last step is a matter of preference… but since you break the commit and create new commits, you will be the author, not the original contributor. I don’t like this so I rewrite history which is something I explained in my last post , making sure the original contributor is listed as the author on the new commits. You can see how this gets manifested in the log from this commit: 98b142a2 :
Now, it’s ideal to have a single PR address a single issue so that means that it would have a single commit in it. This isn’t a hard and fast rule we enforce… it’s more of a preference.
A PR should at least address related issues… it should not include a bunch of unrelated issues. But if it closes two or three related issues, as long as each issue is tied to a specific commit in the PR, it’s acceptable.
Do I really need to explain this? Make it clear what’s going on so the person merging can get in your head and understand the context. A good example of this can be seen in PR210 . You can see it closes issue 17, what it’s actually doing and some extra stuff.
I recommend using GitHub’s new support for PR templates too: https://help.github.com/articles/creating-a-pull-request-template-for-your-repository
And for the grand finale, how do we merge PR’s? It’s not as simple as pressing that green button in GitHub!!!
Why? Because that generates merge commits… you’ve seen those in my last post… they don’t do a damn bit of good except make a horribly dirty & unusable commit log!
So what do we do to avoid this? Admittedly I lifted this process from the Angular Material team… you’ll see our docs on the process are very similar but I’ll outline it here with some more explanation.
Let me first explain it from a high level, then dig deeper. From a high level, the process works like this:
- create local branch & pull in the PR changes
- run all automated checks (tests, vetting, compilation, etc)
- check demo
- address any commit cleanup work
- merge into the main
Let’s pick through these things one at a time.
The 2nd biggest complaint I have with GitHub’s big enticing green merge button is that all the work happens on your hosted repo. To me, that thing should be treated like a bank ledger as much as possible - you don’t change it. If you click this button, it makes changes to your main repo in GitHub, not locally where you can verify what you’re doing before you make the change. Merging is a big deal and I want to have a chance to say “whoa… what the hell did I just do” and push the eject button (in other words: delete the local clone, re-clone and start over with what’s in the source of truth: GitHub).
See the link in the image above from GitHub that says command line instructions? That gives you the commands you want which is basically the following (let’s assume I’ve submitted a PR #555 from my forked repo in the branch newfeature-issue12. I’d do the following:
# make sure your dev branch is current git checkout dev git pull --rebase origin dev # create local branch for the PR off the dev branch git checkout -b pr555 dev # pull the code in from the contributor's repo & branch git pull https://github.com/andrewconnell/ng-officeuifabric newfeature-issue12
At this point I now have a branch with the changes in the PR.
Now here I want to ensure that before I even look at the actual code, it passes all checks. I want to be as clean as possible when I do this and not rely on any legacy stuff.
Because our build process uses Node.js, we use node modules so I blow those away & re-download everything, as well as our built library & clean out previously compiled TypeScript.
Next up, we build the project. Since we use TypeScript, the transpilation process will find any errors. Our TypeScript setup uses a tsconfig.json for all settings so we run it using the following:
tsc -p ./ # we actually have this mapped to an NPM script so we actually use this: npm run build:ts
If there were any errors, it’s a coding error so we kick it back as a “build failed” to the contributor and abort the merge. The contributor should not be sending this if build fails so as far as I’m concerned, this one is up to them to fix.
Next, run the tests:
gulp test --verbose
Again, any errors here get kicked back. Bad contributor for submitting a failing test!
Last but not least, I want to see the demo, so we build the library & open the demo in question:
# build library in prod/minified mode gulp build-lib # build library in debug mode gulp build-lib --debug # ok... I don't run those two commands above... # I run this script that calls both of them npm run build:lib # open the demo, say for the icon directive open src/components/icon/demo/index.html
If everything looks good, I consider this one finished!
I then check the commits to make sure they follow the patters above and in the other post I already published , including breaking them up, squashing them together & making sure they reference closed & referenced issues.
This last piece is important because when the PR is closed, the issues will be automatically closed if referenced correctly in the issues and PR.
Last step is to now push the code into the dev branch. Because we created this branch from dev, this part is easy. What we do is simply jump to dev & rebase it on the PR branch. This will add the commits from the PR to the tip of dev without the merge commits!
# jump over to dev git checkout dev # rebase dev on the PR git rebase pr555 # no need for the PR branch now, so delete it git branch -D pr555
That’s it… push to GitHub and you’re good! I then close the PR and make sure each issue was appropriately closed as well.
There are two downsides to this manual approach. First GitHub uses the merge commits to indicate the PR was merged in their browser interface. That’s what the purple icon indicates. The red icon indicates the PR was closed. If you use this process I outlined in this post, you’ll only get the red PR close icon. No biggie though… that’s not important.
Second, this might sound like a lot of work? Nah… script it like we do! I have this shell script check-pr.sh that prompts me for the PR number, the contributor’s repo URL and the name of the branch as well as the name of the directive to test. It does all this automatically for me:
Hopefully this gives you some insight and guidance, or maybe you learned something that you can apply to your project. At any rate I wanted a post I could share with contributors to the project so they could see how we did it… hopefully it helps others.
Now onto working on the next post in this series… this one is going to be fun… stay tuned next week!