Problems with drive-by fixes

To start with, I think drive-by fixes are great. If you see that something is wrong when fixing something else, it can be a good idea to fix it right away, since otherwise you probably won’t do it.

However, even when doing drive-by fixes, I still think that each landed branch should focus on one thing only. As soon as you start to group unrelated things together, you make more work for others. It might be easier for you, but think about all the people that are going to look at your changes. Please, don’t be lazy! It doesn’t take much work to extract the drive-by fix into a separate branch, and most importantly to land it separately. If you do find that it’s too time-consuming to do this, let’s talk, and see what is taking time. There should be something we can do to make it easier.

There’s no such thing as a risk-free drive-by fix. There’s always the potential of something going wrong (even if the application is well-tested). When something goes wrong, someone needs to go back and look at what was done. Now, if you land the drive-by fix together with unrelated (or even related) changes, you basically hide it. By reducing your workload slightly, you create much more work for someone else.

For example, on Friday I saw that we had some problems with scripts in Launchpad. They were trying to write to a mail directory, to which they didn’t have access. That was odd, since scripts have always talked to the SMTP servers directly, and didn’t use the queued mailer that needed write access to that directory. Looking through the recent commit logs didn’t reveal anything. Luckily enough, William Grant pointed out that r9205 of db-devel contained a change to sendmail.py, which probably was the cause of the problems. This turned out to be correct, but it was still pretty much impossible to see why that change was made. I decided that the best thing to do was to revert the change, but I wasn’t sure exactly what to revert. That diff of that revision is more than 4000 lines, and more than 70 files were changed. So how can I know which other files were change to accommodate the change in sendmail.py. I tried looking at the commit logs, but that didn’t reveal much. The only thing I could do was to revert the change in sendmail.py and send it off to ec2, waiting three hours to see if anything broke.

So, I plead again, if you do drive-by fixes (and you should), please spend a few minutes extra, to extract the fix into a separate branch, and land it separately!

Is there maybe anything we can do to make this easier to do?

Add post to:   Delicious Reddit Slashdot Digg Technorati Google
Make comment

Pingbacks

27.06.2013 8:56 propecia @www.treintadetreinta.org
21.06.2013 10:03 rimonabantexcellence site title @www.rimonabantexcellence.com
Hello http://tillenius.me/blog/2010/05/03/problems-with-drive-by-fixes/

Comments

Jonathan Lange 3.05.2010 13:58

Yes there is.

People merge drive-by fixes into a regular branch for two reasons: the cost of interruption is too high or the cost of landing a second branch is too high.

I don’t think anything can be done about the first. Surely there is much we can do to make the cycle of creating, reviewing and landing a branch faster.

It’s common for me to make note of a change that needs to be made, finish my branch, submit for review, and while I’m waiting for review, create another branch as a pipe in the pipeline for the “drive-by” fix. This means I get two different reviews with their two contextual diffs. It eliminates the need to explain why X unrelated change was made in this branch.

Using bzr-pipelines often means that there’s very little “cost of interruption” and the “cost of landing a second branch” isn’t much higher than the cost of landing any branch.

I favour separating drive-by fixes into a branch that can be landed before or after the primary branch. My biggest grief is lint checkers which report dozens of issues in an old module. The policy to have a lint-free branch can lead to unwanted drive-by fixes.

I think it’s inaccurate to characterize this as a drive by fix though: the specific change was made to enable the broader changes to be tested (I think the problem came from it actually doing far more than that, but that’s a mistake of understanding, not intention).

Basically I agree with your main point though :-)

Required. 30 chars of fewer.

Required.