Gated checkins are evil

Arseni Mourzenko
Founder and lead developer
March 27, 2018
Tags: productivity 36 testing 7 rant 34

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:

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:

Pipeline showing the following steps: style check, linter, fast tests, slow tests, deployment and canary tests

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?

Pipeline with separation arrows just after style check step

Or maybe here?

Pipeline with separation arrows after style check and linter steps

Or here?

Pipeline with separation arrows after deployment step

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.

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:

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.