Read the Diffs, addendum
02 06 2009Just as I was about to write a post extolling the virtues of watching your coworkers’ checkins, I saw that Eric Sink beat me to it.
One additional thing that Eric didn’t mention: Read your own diffs. To some people this may sound stupid — why would you look at the diffs after your own checkins? To answer this criticism, I would like a quick show of hands — how many of you have several unrelated changes open in your source tree on a regular basis? OK, how many of you have unintentionally checked in changes because of that?
Looking over your own diffs immediately after checkin is a quick and easy sanity check that lets you catch problems before they affect your fellow engineers or QA/QE:
- Did you check in extra files?
- Were there unrelated changes in any of the files that shouldn’t have been checked in yet?
- Did you miss any files?
- Did you forget to update a section of code? Looking at diffs makes it easy to spot patterns where, for example, 2 of 3 related functions were modified, and remind you that the 3rd function may also need updating.
A minute or two right after each checkin can save you from a mob of your fellow engineers out for blood because you broke the build with a checkin mistake. In my book, that makes it time well spent.
Or even better, read the diffs *before* checking in. I always do a quick cc_diff and scan it (at least the filename) before committing.
As for reading other peoples’ diffs, I have to admit I don’t do that. I’ve got coworkers who do (or at least did), and I don’t see how they ever manage to get any of their own work done.
I look at my diffs before checking in too, but since I frequently have multiple changes in progress at the same time in the same workspace, looking at the actual checkin helps me insure I didn’t check in more than I intended to.
The pre-commit scripts that we use display the actual diffs that it’s going to commit, followed by a prompt (y/n) to confirm the commit.
Great advice. Prior to getting an OTS I generally…
o Put the files in a changelist (go perforce!) and write the submission comments.
o Consider my strategy for building and testing the change. Was it sufficient? Note why it was in the checkin msg.
o Consider the affects of my checkin on my peers’ workflows in dev and QE. Did I change anything they need to know? Did I add/remove/change any features?
Then with all that context in mind, I quickly race through the diffs in that changeset (<5 mins generally) to see that my actual changes are accurately reflected in the submission comments. Were the largest risks covered? Did I reasonably describe what I actually changed? Etc.
I find that forcing myself to review the diffs “quickly” allows me to get a good overall view of the change and ensure that I’ve described the change well at a high level.
Two pet (s/w development) peeves…
o Ppl that check in five unrelated changes in one submission. Go granular ppl!
o Ppl that don’t describe the impact of their change at a high level. Examples: ‘fixes 27859032′ and ‘next step’. Those aren’t submission descriptions. Just paste in some James Joyce or something! At least I’ll learn something when I read your change description!
Rant off.
-t