Automatically reverting commits that fail the build

Question

A colleague of mine told me that he is thinking in making our CI server to revert commits that failed the build, so the HEAD in master is always stable (as in passing the build at least).

Is this a best practice or it may be more problematic than just leaving master broken until the developer fixes it?

My thinking is that reverting the commit will make more complex the task of readding the commit and fix (developer will have to revert the revert and then commit the fix, which will also clutter the git log) and we should just leave the commit and then commit the fix. Although I see some advantages in having master stable, this revert of failing commits does not convince me.

edit: Doesn't matter if it is master or any other development branch, but the question stays the same: should the CI system revert a commit that failed the build?

another (lenghty) edit: Ok, we're using git in a strange way. We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes, and adds time when you have to reintegrate your branch and deal with possible conflicts. If everyone commits to master this conflicts are reduced to the minimum and every commit passes all tests.

Of course, this forces you to push only stable (or you break the build) and program more carefully to not break backwards compatibility or do feature-toggling when introducing new features.

There are tradeoffs when doing CI this or that way, but that is out of the scope of question (see related question for this). If you prefer, I may reword the question: a small team of developers work together in a feature branch. If one developer commits something that breaks the build for that branch, should the CI system revert the commit or not?

Answer 1

I would be against doing this for the following reasons:

• Any time you set up an automated tool to change code on your behalf, there is the risk that it will get it wrong, or that a situation will arise where you need it to stop making that change (e.g., the latest version of Google Mock had a bug in it, so it's not your code failing) and you have to waste time reconfiguring it. Plus, there's always a slight risk that the build will fail because of a bug in the build system, rather than a bug in your code. For me, CI is about gaining confidence that my code is correct; this would merely turn it into another source of potential problems for me to worry about.

• The kinds of bugs that break "the build" should be silly mistakes that take very little time to fix (as you've indicated in a comment, this is true for you). If more subtle and complicated bugs are regularly making it onto master, then the correct solution is not to "fix it faster", it's to be more careful when reviewing feature branches before they get merged.

• Leaving master unbuildable for a few minutes while the bug gets fixed properly doesn't hurt anyone. It's not like the CEO will personally check out master and publish the code straight to clients at any random moment (at least, hopefully not without your involvement). In the highly unlikely event that you need to release something before you can fix the bug, then you can easily make the decision to revert manually before publishing.

Answer 2

Let's agree on terms first.

I personally use the terms Continuous Build and Continuous Integration to distinguish two different scenarios:

• Continuous Build: a tool that checks periodically if the repository changed since the last build, and build/test if it did.
• Continuous Integration: a tool that takes Pull Requests and validate them against the latest head prior to making them visible.

The latter, Continuous Integration, means that the repository it protects is always green¹: it is strictly better.

Your question only really makes sense for Continuous Build, so I will answer assuming this is your setup.

¹: Environmental causes can also screw up a build, for example a test with a hard-coded year (2015) may start to fail January of 2016, a disk can get full, ... And of course there is the plague of unstable tests. I haughtily ignore those issues here; else we'll never get anywhere.

---

If you have a Continuous Build setup, you can indeed automate reversal of commits that may have broken the build, however there are several subtleties.

• You cannot actually strip the commits: other co-workers might have already checked them out and will push them back in the next time they attempt to commit. Instead, a reversal should be committing a reverse diff. Oh, and co-workers will hate you for reverting their work when it was correct as they'll have to find a way to push it back again...
• You cannot actually only strip the last commit (it's a merge), but need to strip all commits... up to a certain point. For example, the last known good commit (beware when boot-strapping the system).
• You need to think about external causes (environmental issues), and avoid a setup that reverts everything to day 0. Thankfully, reverting to the last known good commit sidesteps this issue.
• You need to think that the last known good build might no longer build (environment issues), in which case it is likely that all further commits will be reversed. Ideally, in case of failure, and before reverting, you would checkout the last known good build, and re-test it. If it passes, revert, otherwise, raise an alert.

Note that with this system, in case of an unstable test or a co-worker often committing crap, many good commits will get reversed. Your co-workers will then hate you.

---

Hopefully my horror tale has exposed the issues of allowing a broken repository and you will now implement a proper Continuous Integration pipeline where PR are never directly pushed to the repository but instead queued up for merging in a work queue, and integrated one at a time (or by roll-ups):

• fetch head of repository locally
• apply pull request(s)
• build and test
• in case of success, push to repository, otherwise mark as failing
• move to next requests

Having attempted both, this is strictly better.

Answer 3

Is this a best practice or it may be more problematic than just leaving master broken until the developer fixes it?

It is problematic. A person deciding "the master HEAD is broken; I will revert the top change" is completely different than the CI system doing the same.

Here are a few disadvantages:

• Errors in the automated reversal process will screw up the repository;

• this assumes a single changeset (the topmost) screwed up the build (which is unrealistic)

• Maintainers will have more work to do to fix the issue, than just investigation and commit (they will also have to look at reverse history)

We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes, and adds time when you have to reintegrate your branch and deal with possible conflicts.

This belief (branches vs. CI) is incorrect. Consider keeping one stable branch, where you commit only unit tested changesets. The rest (feature branches and local branches) should the responsibility of each developer and not part of your CI policy in any way.

In feature branches you want to be isolated from other developers. This allows you to:

• perform exploratory coding

• experiment with the code base

• perform partial commits (effectively commit non-working code) to set up backup points (in case you screw up), to create more meaningfull change history (through commit messages), and to back up your work and switch completely to something else (in the time it takes you to write "git commit && git checkout ")

• perform low-priority tasks that take a long time (e.g. you want to perform a refactoring that alters all 80 classes of the data layer: you change two per day, until you change all of them an the code compiles (but you can do this without affecting anyone until you can make a single commit).

If one developer commits something that breaks the build for that branch, should the CI system revert the commit or not?

It shouldn't. Committing stable code on your CI branch is a responsibility of the committer, not an automated system.

Answer 4

I would suggest using a Gerrit + Jenkins environment to keep your master branch always in a good shape. People push their new code to Gerrit which triggers a Jenkins job to pull that patch, builds, tests, and so on. If other developers like your patch and Jenkins completes its job successfully, then Gerrit will merge that piece of code to your master branch.

Its a similar environment described by @brian-vandenberg

Besides keeping your branch in a good shape you also add a code review step which improves code quality and knowledge sharing about your software.

[1] Jenkins https://jenkins-ci.org/

[2] Gerrit https://www.gerritcodereview.com/

Answer 5

The CI should never alter the commit history of the repo.

The correct solution here is for no commits to be added to master branch if they haven't been tested and verified.

Do you work on feature branches, have the CI run automatically on those, and if the builds fail, don't merge them into master.

You can have an additional build that tests merges if those are a concern, by running on the feature branch, and during the build merging master/integration/whatever into the local branch, then running tests.

Answer 6

We use Jenkins for our build server and use the gatekeeper model for pushing commits -- where a combination of Jenkins and commit triggers (that ensure peer reviewers have done their job) is the gatekeeper.

Commits are pushed indirectly through a curl to Jenkins, where it clones the master repo then pulls in the commit(s) to be merged and performs all required builds (for Linux/solaris). If all builds complete, the commit gets pushed.

This avoids many if not all of the problems discussed so far:

• history alteration
• getting history correct if you're the dev who has to fix the breakage
• instability (in the form of broken builds) is never introduced

It also allows us to directly enforce other requirements such as unit tests completing successfully.

Answer 7

How many times have you gotten that automated email saying your last commit broke the build? How many times is it wrong? But now you have to go check to see if it really was you, or someone else who did another commit around the same time. Or maybe it was something environmental.

If the system doesn't know for sure, then I certainly don't want to automate it.

Answer 8

The question asked is flawed. I respect this statement though

"We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes"

What you should be doing though is these steps

• work off master if you like(that is fine and keep pulling changes from everyone) BUT do NOT commit to master locally
• JUST BEFORE you are going to commit your changes to master, create a branch with submit_XXXXXX
• have your automated build pick up all submit_XXX branches build
• Option 1: build breaks, or merge breaks...change rejected and it never lands on master
• Option 2: build works, jenkins pushes master and updates it

THEN, what we do is put a git commit hook in preventing EVERYONE from actualy committing to master. It works great....NO broken builds ever and NO reverting commits from master either.

later, Dean