Gated checkins are evil

I was re­cent­ly talk­ing with a de­vel­op­er about a pre-build strat­e­gy he wants to set up in his team which uses trunk-based de­vel­op­ment. His idea is that in or­der to pre­vent prob­lem­at­ic code from hit­ting the trunk in ver­sion con­trol, a client-side pre-com­mit hook (i.e. a hook which runs on the de­vel­op­er's ma­chine) will run the lin­ter, com­pile the ap­pli­ca­tion and run some of the tests. The prob­lem he's try­ing to solve is that the de­vel­op­ers don't look reg­u­lar­ly at the state of the build; do­ing those ear­ly tasks in a pre-com­mit hook should make them more aware of the build prob­lems.

I al­ready ex­plained why do­ing any­thing oth­er than style check­ing in a serv­er-side pre-com­mit hook is not an op­tion. I then claimed that a com­mit which takes 900 ms. is too slow. In an ear­li­er ar­ti­cle on Soft­wa­reEngi­neer­ing.SE, I ad­dressed specif­i­cal­ly the sub­ject of unit tests in a serv­er-side pre-com­mit hook.

Im­por­tant note: in the linked ar­ti­cle and an­swer, I was talk­ing about the pre-com­mit hook which runs on the serv­er, i.e. on the same ma­chine which hosts the repos­i­to­ry. In the cur­rent ar­ti­cle, I'm talk­ing about pre-com­mit hook which runs on de­vel­op­ers' ma­chines. When it comes to serv­er-side pre-com­mit hooks, putting any­thing which in­volves the ex­e­cu­tion of cus­tom source code is crazy any­way, for ob­vi­ous se­cu­ri­ty rea­sons. This in­cludes unit tests, but ex­cludes lin­ters.

In this ar­ti­cle, I'll start by ex­plain­ing more in de­tails why com­mits should be short, and then ex­plain how to de­cide which steps of a build pipeline can be moved in a client-side pre-com­mit hook.

Why com­mits should be fast

Long com­mits break the flow

I don't mean that a com­mit which takes in av­er­age five sec­onds is slow; what I mean is that a 900 ms. com­mit is al­ready too slow. It is slow be­cause the per­son has to wait, i.e. re­main pas­sive.

For soft­ware de­vel­op­ers, break­ing the flow leads to a huge pro­duc­tiv­i­ty loss. When a com­mit takes time, it breaks the flow. If a com­mit takes a few sec­onds, the de­vel­op­er re­mains in front of the mon­i­tor, but the flow breaks any­way. If a com­mit takes a minute, the de­vel­op­er switch­es to a dif­fer­ent task, which nec­es­sar­i­ly breaks the flow. If a com­mit takes sev­er­al min­utes, the de­vel­op­er leaves his desk and goes for a cup of cof­fee or a walk.

Get­ting back in the zone takes time: fif­teen min­utes ac­cord­ing to Joel Spol­sky, and I see no rea­son not to be­lieve this met­ric. It may not be par­tic­u­lar­ly im­por­tant for teams where de­vel­op­ers can­not get in the zone in the first place—such as for teams which work in an open space—but for de­vel­op­ers who may ac­tu­al­ly be pro­duc­tive from time to time, this is a huge is­sue.

Giv­en that it is not un­usu­al to do in av­er­age ten com­mits per day, slow com­mits will waste two and a half hours per day per de­vel­op­er, not count­ing the time to do the com­mits them­selves. I don't know how about you, but it seems like... a lot, giv­en the hourly rate of a pro­fes­sion­al de­vel­op­er.

Long com­mits dis­cour­age com­mit­ting fre­quent­ly

Wait­ing sucks. If de­vel­op­ers have to wait at each com­mit, this will in­evitably push some of them to post­pone their com­mits. This leads to mul­ti­ple is­sues:

• Post­poned com­mits lead to painful merge. As sim­ple as that.

• De­vel­op­ers do­ing mi­nor changes—refac­tor­ing, fix­ing style, cor­rect­ing ty­pos—will be in­clined to put those changes in a big­ger com­mit, such as the fix of a bug. This pol­lutes those big­ger com­mits, com­pli­cates code re­view and makes it eas­i­er for the is­sues to creep in. When such com­mit breaks the build, it takes more time to see what caused a re­gres­sion, es­pe­cial­ly when the re­gres­sion is caused by some­thing small, such as a change in a name of a vari­able dur­ing refac­tor­ing.

• Some de­vel­op­ers will com­mit when they can af­ford wait­ing, such as at noon or in the evening, in­stead of com­mit­ting af­ter they fin­ished per­form­ing an atom­ic change. Need­less it is to say that such com­mits are per­fect­ly mean­ing­less, and heav­i­ly de­crease the use­ful­ness of a ver­sion con­trol sys­tem.

What could be put in pre-com­mit hooks

The build pipeline can usu­al­ly have nu­mer­ous and di­verse steps. Here's an ex­am­ple of a build pipeline:

The pipeline can be bro­ken in two: some steps can be per­formed lo­cal­ly, launched by a pre-com­mit hook; oth­ers will re­main on the build serv­er.

The ques­tion is where to put the line. If each task is now rep­re­sent­ed pro­por­tion­nal­ly to its du­ra­tion, should the lim­it be put here?

Or maybe here?

Or here?

I ex­plained in the pre­vi­ous sec­tion that com­mits which take 900 ms. is al­ready too slow. This alone doesn't give us too much op­por­tu­ni­ties: the choice is prob­a­bly be­tween no steps in pre-com­mit hook, or just a style check, or a style check and a lin­ter. In the ar­ti­cle I al­ready quot­ed above, I ex­plained the dif­fi­cul­ties I en­coun­tered when de­cid­ing where to put the lin­ters. Years lat­er, I still keep lin­ters in the build pipeline, and the pre-com­mit hook only checks the style (the ex­cep­tion be­ing Dou­glas Crock­ford's jslint, giv­en how fast the tool is). Putting lin­ters first in the pipeline, by the way, en­sures that I get the feed­back in terms of sec­onds when I break one of their rules.

The du­ra­tion ar­gu­ment put aside, what else can help us de­cide what to put in the build pipeline, and what should be done in a pre-com­mit hook?

One of the goals of the build pipeline is to catch things which were bro­ken by a com­mit. It could be a bug caught by a unit test, or a per­for­mance is­sue spot­ted by a stress test, or a prob­lem­at­ic pat­tern de­tect­ed by a lin­ter, or a style er­ror found by a style check­er. A ben­e­fit of a pre-com­mit hook over a build serv­er is that when­ev­er some­thing get caught, the prob­lem­at­ic code doesn't stand a chance of reach­ing the ver­sion con­trol (and af­fect oth­er de­vel­op­ers). This is just huge, and if we could run the en­tire pipeline lo­cal­ly, in a mat­ter of a few dozens of mil­lisec­onds, I couldn't be more hap­py. Just do svn ci -m "...", and in­stant­ly, the change is de­ployed on the servers, or you're pre­sent­ed with a mes­sage telling that your com­mit was re­ject­ed be­cause it would break some­thing.

In real life, it doesn't hap­pen like this, and not only be­cause the build takes time, but also be­cause some tasks can­not be per­formed on de­vel­op­er's ma­chine, or don't scale well, or are sim­ply un­re­li­able. Let's dis­cuss those three as­pects in de­tail.

• Some tasks can­not be per­formed on de­vel­op­er's ma­chine. A smoke test or a sys­tem test, for in­stance, re­quires sub­stan­tial ef­fort to set up the in­fra­struc­ture re­quired to run it. For any but the most ba­sic pro­jects, it is im­pos­si­ble to have such in­fra­struc­ture on a PC of a de­vel­op­er.

  Se­cu­ri­ty is an­oth­er im­por­tant fac­tor here; even if one trusts a de­vel­op­er by let­ting him to write code which will be pushed to pro­duc­tion, it may not be a good idea to let de­vel­op­ers' ma­chines have ac­cess al­low­ing to re­build pro­duc­tion servers.

• Large pro­jects may re­quire slight­ly more than an or­di­nary de­vel­op­er's desk­top to be built. It is not un­usu­al to have dozens of beefy servers work­ing in par­al­lel on a sin­gle build. Imag­in­ing that the build could work on a sin­gle ma­chine pre­vents the ap­proach from scal­ing.

  For small­er pro­jects which can tech­ni­cal­ly be built on an av­er­age de­vel­op­ment work­sta­tion, it's still usu­al­ly a good idea to throw more hard­ware in or­der to re­duce the build time from, say, twen­ty min­utes down to two. Faster the de­vel­op­er is in­formed of the is­sue in­tro­duced by his com­mit, cheap­er it is to solve the is­sue, and this short cy­cle alone can jus­ti­fy a thou­sands of dol­lars ex­pense on ded­i­cat­ed servers.

• “It works on my ma­chine.” That's great, but it doesn't mean that it will work any­where else. One of the is­sues I en­counter reg­u­lar­ly, for in­stance, is that I use a Python pack­age in my app, but for­get to de­clare the de­pen­den­cy to the pack­age. It works per­fect­ly fine lo­cal­ly and all the tests pass, but when I com­mit the changes, the build serv­er starts telling of­fend­ing things about my in­tel­lec­tu­al ca­pac­i­ty. On one side, it is ob­vi­ous that I am the prob­lem here; on the oth­er side, if I was nev­er the prob­lem, I wouldn't need re­gres­sion test­ing the first place, would I?

  Build servers are based on scripts. They are de­stroyed and re­built sev­er­al times per day. They run on stan­dard­ized vir­tu­al­ized hard­ware. They are clones. De­vel­op­ers' work­sta­tions are slight­ly more volatile, which means that every such ma­chine has its quirks, its ver­sions of pack­ages, its per­son­al­i­ty. Even if they were reini­tial­ized on reg­u­lar ba­sis, say once per week (which would be un­bear­able for their users, but this is a dif­fer­ent sub­ject), dis­crep­an­cies will still be in­tro­duced, es­pe­cial­ly when work­ing on a hard to re­pro­duce bug or a tricky fea­ture. If I made some­thing work af­ter two hours of head-bang­ing, it doesn't mean it ac­tu­al­ly works every­where; it may also mean that I ad­just­ed my work­sta­tion so that the fea­ture works lo­cal­ly, but wouldn't be able to re­pro­duce what I did on a dif­fer­ent ma­chine.

  Here's a fun­ny sto­ry: a year ago I in­tro­duced a re­gres­sion which was caught at the very end of the chain by the ca­nary tests. I for­got to repli­cate a change in DNS records in the ap­pli­ca­tion I was work­ing on, so the ap­pli­ca­tion was still us­ing the old do­main name of a de­pen­den­cy ser­vice. Nev­er­the­less, it worked per­fect­ly fine lo­cal­ly, where the old DNS records were still in cache, as well as on the build serv­er which also had the DNS records in cache. The prob­lem ap­peared only in pro­duc­tion where the new­ly de­ployed servers with the new ver­sion of the app and an emp­ty cache were try­ing to ac­cess the do­main which didn't ex­ist, lead­ing to lots of HTTP 500. This was the hard way to learn that (1) the build serv­er should nev­er run the tests, and should del­e­gate them to volatile test servers re­built af­ter they per­form their job, (2) serv­ing HTTP 500 just be­cause a de­pen­dent ser­vice is un­reach­able in un­pro­fes­sion­al, (3) chang­ing DNS records sucks in all cas­es.

So here again, it doesn't leave us too much op­tions. Since tests—even unit tests—can­not be trust­ed when ran lo­cal­ly, they have to be part of the build pipeline, run­ning on ded­i­cat­ed and volatile test servers. What about lin­ters? It de­pends. It may work for some lan­guages, and may be more dif­fi­cult for some oth­ers to en­sure that they pro­duce the same re­sult on every de­vel­op­er's ma­chine. Style check­ing re­mains that ba­sic thing which can rel­a­tive­ly easy be per­formed lo­cal­ly.

Or there is an­oth­er way. One can per­form all those tasks on ded­i­cat­ed vir­tu­al ma­chines any­way—just like the build does—and just do the or­ches­tra­tion from de­vel­op­er's ma­chine in­stead of a build serv­er. This would mean that lin­ters, tests and de­ploy­ment will be as re­li­able as if they were launched by the build serv­er, as well as be able to scale per­fect­ly well. In this case, what is re­al­ly the dif­fer­ence be­tween or­ches­trat­ing the process from a build serv­er ver­sus do­ing it from a work­sta­tion of a de­vel­op­er? It all comes to two fac­tors:

• Do­ing it lo­cal­ly pre­vents faulty code from reach­ing ver­sion con­trol. You may ask your­self how could that be bad. Eas­i­ly. Failed builds are im­por­tant in terms of data analy­sis and pub­lic­i­ty. It is very in­ter­est­ing (and rep­re­sen­ta­tive of the team and the com­pa­ny) to watch how the ra­tio evolves over time, and what are the er­rors which lead to fail­ures. More­over, the fact that a build failed doesn't mean the com­mit has no place in this world. If I have to have the last mod­i­fi­ca­tions of my col­league right now to avoid a painful merge lat­er, I don't care if it breaks some unit tests. If a de­vel­op­er needs to com­mit right now be­cause in five min­utes, he's leav­ing for a two-weeks va­ca­tion in Dubai, it is much more im­por­tant for the team to get his mod­i­fi­ca­tions and to fix a few bro­ken tests lat­er than to redo what he did dur­ing the last two hours.

• Do­ing it lo­cal­ly forces the de­vel­op­er to be in­formed if the build breaks. Putting aside my ex­pla­na­tion about the rea­sons com­mits should be fast, block­ing the com­mit is not the best way to no­ti­fy a de­vel­op­er about any­thing. There are much more ef­fec­tive ways to do it: show the build in red on a mon­i­tor­ing screen, or send a SMS to the de­vel­op­er, or as­sault him phys­i­cal­ly (don't do the last one; had that thing in my eye once, didn't like it). A com­mit which stalls for two min­utes won't be watched by the de­vel­op­er any­way: the de­vel­op­er will launch it, and then go work on some­thing else, and will see a pos­si­ble er­ror mes­sage much lat­er than he could with prop­er no­ti­fi­ca­tion mech­a­nism.

To con­clude, if you're hes­i­tant at mov­ing some of the steps from the build pipeline to the pre-com­mit hook, just don't. While style check­ers can (and usu­al­ly should) be in a pre-com­mit hook, any­thing else, in­clud­ing the lin­ters, be­longs to the build pipeline it­self. Do­ing oth­er­wise will nec­es­sar­i­ly have a neg­a­tive im­pact on the pro­duc­tiv­i­ty of the team, while the ben­e­fits are very ques­tion­able.