Git – Automatically Reverting Commits That Fail the Build

continuous integrationgit

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?

Best Answer

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.

Related Solutions

Git – Does Squashing Pull Requests Break Git’s Merging Algorithm?

With Git, commits

are immutable,
and form a directed acyclic graph.

Squashing does not combine commits. Instead, it records a new commit with the changes from multiple other commits. Rebasing is similar, but doesn't combine commits. Recording a new commit with the same changes as an existing commit is called history rewriting. But as existing commits are immutable, this should be understood as “writing an alternative history.”

Merging tries to combine the changes of two commit's histories (branches) starting from a common ancestor commit.

So let's look at your history:

                                 F  feature2
                                /
               1---2---3---4---5    feature1 (old)
              /
-o---o---o---A---o---o---S          master

A is the common ancestor, 1–5 the original feature branch, F the new feature branch, and S the squashed commit that contains the same changes as 1–5. As you can see, the common ancestor of F and S is A. As far as git is concerned, there is no relation between S and 1–5. So merging master with S on one side and feature2 with 1–5 on the other will conflict. Resolving these conflicts is not difficult, but it's unnecessary, tedious work.

Because of these constraints, there are two approaches for dealing with merging/squashing:

Either you use history rewriting, in which case you will get multiple commits that represent the same changes. You would then rebase the second feature branch onto the squashed commit:

                                 F  feature2 (old)
                                /
               1---2---3---4---5    feature1 (old)
              /
-o---o---o---A---o---o---S          master
                          \
                           F'       feature2

Or you don't use history rewriting, in which case you might get extra merge commits:

                                 F  feature2
                                /
               1---2---3---4---5    feature1 (old)
              /                 \
-o---o---o---A---o---o-----------M  master

When feature2 and master are merged, the common ancestor will be commit 5.

In both cases will you have some merging effort. This effort doesn't depend very much on which of the above two strategies you choose. But make sure that

branches are short-lived, to limit how far they can drift from the master branch, and that
you regularly merge master into your feature branch, or rebase the feature branch on master to keep the branches in sync.

When working in a team, it is helpful to coordinate who is currently working on what. This helps to keep the number of features under development small, and can reduce the number of merge conflicts.

Best Answer

Related Solutions

Git – Does Squashing Pull Requests Break Git’s Merging Algorithm?

Related Topic