Refactoring in a GIT world


Mike McQuaid recently said in a mail regarding good style in GIT

Treat committing as being slightly more granular than saving a file. Do it very, very often.

This is a very succinct way of expressing something that you feel happening to yourself when you migrate from a CVCS to a DVCS: Commits become smaller. And so they should!

Take Refactoring as a specific case-in-point. We all say we’re refactoring all the time, but if we’re honest, we’re not really doing all the compile-and-test cycles that the book mandates after every minor step. Nor should we: they’re a waste of time, and a band-aid for the lack of proper tool support.

Automated tests, where available, are not usually designed for speed of execution. So it’s not uncommon for tests to run an unacceptably long time. And for a unit test, everything that exceeds one second is something that I consider too slow. Too long, to run each time you move a single line of code around.

Many will argue for a refactoring browser as a proper tool for the job. It can certainly help avoiding dropping-the-brick mistakes. But a refactoring browser cannot deal with more complicated refactorings, or semantic failures that a refactoring introduces. We still need the tests. We still need to limit the amount of changes in between tests lest we waste even more time searching for the specific change that broke a test (assuming we have it).

In fact, since a refactoring browser does the whole multi-step process in one big stride, it’s actually harder to find where it introduces a regression. There are probably refactoring browsers that optionally record each step of a refactoring in order to catch regressions; use them.

The problem also extends to the time after publishing the refactoring. In real-world projects, well-meaning refactorings still introduce regressions because there’s no 100% test coverage in that area of the code, or some users of the code use it outside of its design parameters. So, regressions will often show up further down the line.

Now, git-bisect is a very good tool for finding the commit that introduced a regression, but it doesn’t help much if the commits are too large. But what is a good commit size?

The Refactoring book gives a very clear answer to that: just replace compile-and-test by compile-and-commit, or just ‘commit’. Once the whole refactoring is done, you let your extensive test suite run on the result. If you broke something, you use git-bisect to find the commit that broke it. Since each commit is a trivial change, you’ll never search for the bugger, git-bisect will find it for you.

Even better: when you don’t squash the series of commits that make up your refactoring when pushing upstream, any regressions that you may have introduced and that were not caught by your test suite will still be pinpointed by git-bisect, in a trivial manner.

Here is a summary for doing a refactoring in GIT:

  1. Indicate that you’re doing a refactoring, and which one, by using a common prefix for all summary lines of the commits involved. Example: “FooBar: Extract Method doCalculate(), pt.N: use new doCalulate() in doStuff()”
  2. Commit at least as often as the book says you should compile-and-test.
  3. Run the tests only once, at the end
  4. If something breaks, use git-bisect to find the bad commit, and rewrite history.
  5. Don’t squash the commits when pushing. If colleagues complain about your tiny commits, complain about their large commits, and point them to this post🙂

Is it worth it? Is the extra time you spend writing commit messages offset by the time you save when something goes wrong? Immediately, or later on? I don’t know yet. But if the experience written down in Fowler’s book is to be trusted, there’s a high probability it should.

About marcmutz
Marc Mutz is a Senior Software Engineer, Trainer, and Consultant with KDAB.

9 Responses to Refactoring in a GIT world

  1. reisi says:

    Firstly, a good post. I find the fifth point in your summary especially interesting.. For the whole time I’ve been thinking that people should use feature-branches and push/create pull request for a single (squashed) patch. That being said, we still use svn and are only planning to move to git … This is one of the things I’d like to know before migrating.

    If you take “patches reviewed in mailinglist” model used my many oss projects, you do not want that the patch is 100 parts, you want that it’s in less than 10 parts. It’d be very confusing if later patches would fix something in the first two files, so all parts must be a subset of the whole

    So for reviewing purposes you’d need to see the squashed, and in some cases (esp. very large refactoring jobs) it might be good to hold on to the history of refactoring. I’d also add “clean history” as a pro of squashed merges — that way git log will only show you the “feature level” workings, not “forgot to invert this condition” — “whoops inverted it twice”, etc.. Is there no way you could have both [with git]?

    • marcmutz says:

      You can always view less information, by viewing the diff over multiple commits, so easier review isn’t a valid reason for squashing. This is again a tooling problem:
      Yes, the mails are static, but gitk isn’t. You can always fetch the actual commits, and review them any way you like (per commit, per file, per author, overall), without affecting your own work-in-progress. A good review tool would allow you to switch between seeing the individual commits vs. the aggregated change.

      The important part is that the fine-grained commits are preserved, and not squashed. It’s one thing to filter and aggregate information, another one to permanently remove it.

      • jok says:

        This is one of those cultural things, I suppose…
        The rule I’ve tried to impose on commits is “When what you have works, commit it and state why you changed whatever it was you changed. If it doesn’t work it better be a branch”. This tends to scale over CVS,SVN and GIT just allright.

  2. Jan Kundrát says:

    I don’t really have that much experience in this matter, but I tend to feel that having each commit “as correct as possible” is worth the effort. This means that I typically won’t commit stuff which either fails to compile, or breaks functionality somehow. I’m inclined to believe that this is a correct method, if only to save the time when doing the ex-post bisecting many weeks later, where you really shouldn’t waste time debugging a particular changeset which is known to break something and which you’ve already fixed in your next commit months ago. Unfortunately, this will lead to creating rather big patches, especially when you do a “big change”. But maybe I’m simply already spoilt and I call a “big change” something which is not big at all, like this changeset I made yesterday. When I was working on it, it looked like a non-trivial change — I’ve certainly saved the files/rebuilt many times, without even calling git.

    So, what’s your opinion on that?

    • marcmutz says:

      There are basically two situations I see myself going through over and over again:

      Either, I’m coding away and forget time, and end up with lots of changes in a lot of files. Once the thing works for me, I like to take a step back, and try to find a route of independent, minimal, but self-contained patches that, in sum, re-create the whole, modulo debug and other unwanted stuff. E.g., I like to commit whitespace-only changes separately. How detailed I go usually depends on how fine-grained I can git-add -p the stuff without having to revert to patch editing (though I use that a lot, too). Once I have a patch series, I back-track with git-checkout HEAD^ && make to see if each of them compiles separately, rewriting them if they don’t. Eventually, I push.

      I might also decide to do some commits in between, when I see that the density of changes becomes unmanageable. I then often end up with commits that, once I’m done, make no sense anymore. I then rewrite the (unpushed!) history to remove/squash/reorder commits so each one becomes independent, minimal, but self-contained and—we have this great German word “zielführend” (lit. leading towards the goal).

      Or, I know exactly where I need to go. Refactoring is such a situation. The book tells me which steps I need to take. In order to save time, I basically do one step, commit, do the next step, commit, etc. In other words: I commit more often than I’d compile, much less test. If something fails at the end (to compile or pass tests), I immediately turn to git-bisect --run.

      As an example, I had some (two dozen or so) int fields, some of which I wanted to convert to uint. I did this one field at a time, which makes each commit literally adding one character. I wrote a small shell pipeline that would automatically create a commit message from the diff (“change field $(git diff –cached | grep -E ^[+] | cut -cN- | …) from ‘int’ to ‘uint'”), so it took only marginally longer than without the commits in between. Once I compiled, I found that the behaviour of the application had changed, and used git-bisect to find the five or so fields where the change to unsigned broke the arithmetic expression using them. I fixed those in separate commits, which I then prepended to the series of int->uint commits. That was the result that I pushed. Another option would have been to amend the fixes onto the actual changes, but I felt that would mix two steps.

      So, it’s really about which hat you’re wearing: If you’re wearing the refactoring hat, plan your refactoring (follow the recipe from the book, write a shell pipeline to rapidly commit each step, and be it only git commit -a -m "Foo: refactor bar, pt.$i" && let i++. If you’re wearing the development hat, try to break out the result into as many smaller pieces as makes sense. This is not a function of locs changed, even though lots of locs changed for each step might indicate shotgun surgery, but of semantic distance. If this step introduced a bug, would you have to search for it? Chances are that if you’d have to, the step (and therefore commit) was too large.

  3. just replace compile-and-test by compile-and-commit, or just ‘commit’.

    It’s important that every commit compiles, otherwise you’ll break bisect. Also, for the same reason it’s important that every commit leaves the application in a consistent state, so that you can use it and see whether a given commit causes the bug you’re bisecting for.

    My practice is to commit small changes that make sense on their own (e.g., “Remove old_val argument from foobar(). Update callers.”

  4. Torgny says:

    I agree totaly with Thiago, never knowingly commit (or rather push a commit) that doesn’t build or doesn’t work in some sence. If you do you screw your fellow developers and break bug hunting in the future.

    • marcmutz says:

      > never knowingly commit (or rather push a commit) that doesn’t build

      This is the crucial difference. There’s nothing wrong with committing stuff that you don’t know compiles. You have to check before pushing, and rewrite the commit if it turns out it doesn’t. See my reply to Jan for more details.

  5. TheBlackCat says:

    Just out of curiosity, does kdevelop have a save-to-git toolbar button? Making committing as easy as saving would probably encourage people to follow this policy more closely.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: