Git is magic, but sometimes it drives my crazy with all that power causing strange situations and clean-up work. The magic of git is that it is (nearly?) always possible to clean up the mess and get back to a good state. The problem is that there are A LOT of commands to master. This post is a story about cleaning up a pull request to prepare it for merging into master.
The pull request contains functionality that I do want to merge, but also workarounds for cultural sensitive unit tests that are obsolete and should not be merged. There’s also a rebase that has been done in the wrong way and a merge from master of the forked repo – which is no longer in sync with the main repo’s master branch.
Keeping a backup
Before doing anything at all I want to make sure that I don’t screw up more. The git way to do that is to make a separate branch. Remember that git commits are immutable. Although git permits rewriting of history, the actual commits are never changed. Changes to a branch are done by creating a new series of commits and moving the branch pointer. That means that if I do my fixups on a separate branch, only that branch will be affected and the original pull request branch will still be intact.
PS C:\git\authservices> git checkout 39SignVal Switched to branch '39SignVal' PS C:\git\authservices> git checkout -b 39SignValFixed Switched to a new branch '39SignValFixed' PS C:\git\authservices> |
Changing Merge to Rebase
Looking at the revision graph, there have been two branches that are now merged with a merge commit. One of the branches contains a commit that I want to remove entirely. It would also look better if the branches were rebased instead of merged. The first step is to get branches for each of the parents. Every commit in git can have several parents. They are reached as HEAD^[ParentNo], so what I want is one branch each for HEAD^1 and HEAD^2. That is done by using git reset --hard to move the current branch pointer to another commit.
PS C:\git\authservices> git reset --hard HEAD^2 HEAD is now at a968ce6 Fixed tests to be more culture robust PS C:\git\authservices> git checkout 39SignVal Switched to branch '39SignVal' PS C:\git\authservices> git checkout -b 39SignValBase Switched to a new branch '39SignValBase' PS C:\git\authservices> git reset --hard HEAD^1 HEAD is now at 63168ca Rebased from original master PS C:\git\authservices> |
The result is that I’ve moved the 39SignValFixed branch pointer back and created a new 39SignValBase branch pointer. This enable individual clean-up of each of the two branches, before rebasing them.
To be honest, things didn’t go as smooth as I show here when I did all this work. Several times I went down the wrong path, but I only present the way I ended up doing here (don’t want to bore you with all my errors). One thing I found out when working with all this is the importance to check that every step compiles and the tests run before going on with the next operation. At first I tried a one-step rebase, but the resulting conflicts were too hard to sort out.
Removing a Commit from the Base Branch
The 39SignValBase branch contains two commits. The first one is an incorrect fix of the cultural sensitiveness of the unit tests. I want to drop that commit entirely. That is done with an interactive rebase.
PS C:\git\authservices> git rebase -i HEAD~2
Successfully rebased and updated refs/heads/39SignValBase.
PS C:\git\authservices> |
HEAD~2 means that I want to go back to the grand-parent of the current HEAD. the -i is short for interactive, which makes git bring up my default editor with a text file where I can mess around with the commits affected by the rebase.
#pick 9b30d26 Fixed a unit test pick 63168ca Rebased from original master |
I’ve commented out the commit I want to remove. When I hit save and exit git catches the change and perform the rebase. The resulting branch (39SignValBase) contains some issues: An XML comment is incorrect and one test is not running (and it doesn’t test some new functionality). That is fixed by an ordinary commit to that branch.
Before moving on I also review the code of 39SignValBase and fix some issues, so that 39SignValBase is no in proper shape.
Rebasing 39SignValFixed on 39SignValBase
In the original branch sent as a pull request the final commit was a merge. Now when the base branch is cleaned up, it is now time to replicate that, but not using merge. Instead I’ll rebase the latter branch on top of the base branch. That will give me one nice straight line in the commit history.
Before rebasing there’s a small issue to fix though: The 39SignValFixed branch also contains the “Fixed a unit test” commit that I previously removed from the 39SignValBase branch. That commit is dropped with rebase -i in exactly the same way as for the 39SignValBase branch.
Then it’s time for the rebase, to stitch those branches together. I imagine a rebase much like cutting a branch off a tree and then stitching it back somewhere else. When rebasing, it is the branch you’re sitting on that is sawed off (and that is perfectly fine).
PS C:\git\authservices> git checkout 39SignValFixed Switched to branch '39SignValFixed' PS C:\git\authservices>git rebase 39SignValBase |
git now finds the latest common ancestor of the current branch (39SignValFixed) and the given branch (39SignValBase). The commits of the current branch are then replayed on top of the base branch. In this case I got a lot of conflicts at each step. This is one of those cases where git really shines over other version control systems I’ve used; git drops back to the console when there is a conflict. I can use git mergetool to resolve the conflicts, but also fire up my IDE and build the solution at each step as well as running the tests. Fixing the compilation errors at each conflict resolution step is so much easier than getting a giant merge conflict from all commits to sort out. When one conflict is resolved, git rebase --continue is used to continue the rebase.
Rebase was Wasted Work
When the rebase is done the result can be inspected: Whitespace changes only. There were no new changes in the 39SignValFixed branch, they were just a result of a previous failed rebase/merge attempt. So let’s get rid of that wasted work and drop the the 39SignValBase name.
PS C:\git\authservices> git reset --hard HEAD~2 HEAD is now at aa7e538 Cleanups to Saml2Response PS C:\git\authservices> git branch -d 39SignValBase Deleted branch 39SignValBase (was aa7e538). PS C:\git\authservices> |
Now everything is fine – there’s a 39SignValFixed branch that is in a clean up state. It’s just one more thing to do before sending this off as a pull request.
Rebasing on master
Any pull request should be rebased on the current master before being sent. That way the maintainer won’t have any conflicts to sort out when merging the pull request. I’m still on the 39SignValFixed branch – which is the one I want to cut off and stitch back on top of master (remember, in git the correct way is to saw of the branch you’re sitting on).
PS C:\git\authservices> git rebase master First, rewinding head to replay your work on top of it... Applying: Rebased from original master Applying: Updated test XmlDocumentExtensions_Sign Applying: Minor fixes to Saml2ResponseTests. Applying: Cleanups to Saml2Response PS C:\git\authservices> |
The result is finally something that is easy to merge to master. In fact: a merge now will just move the master branch pointer to the node pointed at by 39SignValFixed.
Lessons Learned
I certainly learnt a lot of git work by cleaning up this pull request. I hope you did too by reading about my adventures.
But there’s a more important lesson to be learned from this. The reason that we had this mess in the first place: The original work had been done on the master branch of the fork. That made a rebase hard to do.
Never, ever work on master. Always create a feature branch.