Review your commit changes with 'git add --patch'

I found that git is particularly useful for reviewing changes I'm going to commit.

When I'm ready to commit and my change is more than couple of lines of code in one file, I do

  $ git add --patch .

Then git nicely shows me every hunk of change I made in the code.

I see two benefits of working that way:

1. Obviously it is opportunity to skim through my own code, and it leads to more focused reviews than if I just read through whole source, or even skim through full patch to be committed.
2. It leads to atomic commits which I'm big fan of:

• I don't forget to add things to index. This is not much important when using git, 'cause I always can amend my commits, or even rebase interactively, but it's always nice to form nice commit earlier rather than later. Of course I can do just 'git add .' or even 'git commit -a' for that which leads to next point:
• if for some reason I made several unrelated changes, I have an opportunity to split my change into several logical changes if needed. That is where real power comes.

The workflow is as follows:

  $ <hack hack hack>

  $ git add --patch . # interactive session where I select what comes to commit

  $ git stash save --keep-index # I stash my unstaged index; needed for the next step:

  $ <compile and run tests to make sure you haven't screw with partial adding patches>

  $ git commit

  $ git stash pop # get the remainder of my changes back in working copy

  $ <repeat with git add>

If I have screw somewhere:

  $ git reset # moves staged changes from index (they are still in working copy)

If I want to review the changes one more time after adding but before commit:

  $ git diff --staged

It sound like a lot of work should be done for every commit. I don't know. First of all I do it fast enough (shell history can help do it even faster). And I use this technique only if I've touched code in several places.

Advanced techniques and details of git add you can add in the documentation, and in Markus Prinz'es article.

SCM: you should make atomic commits

As with functions/modules/classes, tools, and almost everything else in software development, a change that you are going to commit should have a single purpose, and should accomplish this purpose. I will refer to such changeset as an atomic commit.
The most important side effect of an atomic commit is that it produces diff that is easy to read and understand. In turn, having diff that is easy to read and understand makes you happy because:
- it simplifies your debugging by localizing changes;
- it simplifies code review by localizing changes.

Atomic commit doesn't mix refactoring, bug fixing, development of new feature, and style changing. Neither mixes it several refactorings, or bug fixings or whatever.

Atomic commit is self-contained, and accumulates all changes that serve its purpose.

Atomic commit has a short and up-to-the point log message, which usually doesn't contain 'and'-word and lists.

"But I don't have time to fix that minor issues, like capitalizing and spaces, separately: I want to do that along working on my primary task at hand!"

If those are minor issues, why bother spending time on them? It's not "fixing" then, it's polishing. You should polish your product, not your code (unless it happens that your code is the product). Otherwise, just get a piece of paper or text file and add notes about what should be done after you have finished your task.

"But often while working on a task I notice some TODOs or small things that I will forget if I haven't fix them right now!"

TODOs are easy to grep, aren't they: why don't you just get good habit to elaborate them periodically. Small things could be transformed in TODOs so you don't forget. Otherwise, just get a notebook or text file and add notes about what should be done after you have finished your task.

"But dumping small things to the piece of paper kills my flow!"

And fixing those things while working on bigger one doesn't? Then forget about small things.

Even better, switch to the modern SCM. This days modern means distributed, and most often that means git or Mercurial. For those who use one of such tool, there are no excuses at all to not produce atomic commits. Because in your local repository you can commit absolutely freestyle, and then slice the meat you've just produced into nice atomic cuts before upstreaming. (For git users, interactive rebases and partial commits are primary tools for that.)

"But merging changes from one codeline to another means that corresponding commit doesn't have single purpose!"

Wrong. The single purpose of merge should be delivering change to another codeline. That's why you should carefully choose the changes you want to merge in one commit. Once again, that is much easier to do with distributed tool, but it is also simple to do right with svn or perforce. (I will post more on branching/merging in some of subsequent posts.)

Example: this is just awful:

$ git diff

diff --git a/my.cpp b/my.cpp
index 6223d3c..4246210 100644
--- a/my.cpp
+++ b/my.cpp
@@ -1,14 +1,17 @@
-int fancy_stuff(int arg)
+int fancyStuff(int n)
 {
-    // do a lot of stuff
-    return arg * 2;
+    // Do a lot of stuff.
+    return n * 2;
 }

-int contrived(int arg)
+int contrivedFunction(int n)
 {
-    if (arg > 0) {
-        return fancy_stuff(arg*2);
-    } else {
-        throw std::runtime_error("arg is negative!");
+    if (n >= 0)
+    {
+        return fancyStuff(n * 2);
+    }
+    else
+    {
+        throw std::runtime_error("n is negative!");
     }
 }

... if all you wanted to say was:

 int contrived(int arg)
 {
-    if (arg > 0) {
+    if (arg >= 0) {
         return fancy_stuff(arg*2);
     } else {
         throw std::runtime_error("arg is negative!");