isle of beans

- or why isn't the github flow trunk based?

Abstract

I had worked with personal and feature branches for quite a time when I started to work in a new team. This team was blaming any kind of branching as the evil itself.

In the discussion about this topic I've tried to explain:

Each local repository is a branch from the way git sees them anyway.
Trunk based development hinders me in experimenting.
Trunk based development leads to a messed up commit history.
Trunk based development encourages developers to hold back their pushes until why are sure what nothing will break.

The teams view is:

Anything can be solved by feature toggles - read Martin Fowler about feature toggles.
As long as your'r cloning to the same branch name you don't need to know that your on a separate branch.
A good/disciplined developer will never hold back anything. He will always build the feature toggle first and will often push.
If you are working on a feature branch you will not be covered by the CI build.
If two developers are working on different branches way will have a hard time to merge if they have finished the feature.

To get a better view on the arguments of the team I'v looked around for articles about the topic.

They all have two problems (from my view) in common:

They always merge if they should rebase (while discussing not trunk based development).
They ignore the cleanliness of the commit history.

How merging instead of rebaseing has almost killed our repository even with trunk based development

Developers not knowing their tools are an accident waiting to happen.

We do have a large repository which is under heavy refactoring for more then 6 months now. The work of 4 developers over 3 weeks was removed from the HEAD by an wrong done merge.
What happen'd?

A fifth developer was on vacation for 3 weeks
On his first day back he has not pulled but started with his first task.
After he had finished his ui git tool has refused to push since he hadn't pulled first.
He pulled in merge mode since this is the default on his repositories.
He solved the merge conflict by clicking on 'take my changes'
He pushed without further checks.

OK - the accident was solved by a force push with the previous commit as HEAD. But you don't want to do that too often.

And guess - we had the same problem again on the next day with an other developer.

Now we have introduced the developers to setup 'git config --global branch.autoSetupRebase=true' but we should make sure they know their tools better.

How trunk based development messes up your commit history.

My main concern about trunk based development as it is described here is the messy commit history you'll get.

But the second thought is always: If developers don't know how to handle a simple rebase - do you really want them to do cherry picking?

Let's look how a trunk looks like if there are multiple developers working on multiple features.

You can do it with release cycles using release branches.

Or you can do it using continuous deployment where each commit leads to a potential release.

The resulting commit history of release v1.1.0 or its corresponding release from the continuous deployment repository will be a mixed up list of multiple features.

Feature-D commit message
Feature-C commit message
Feature-B bug fix commit message
Feature-D commit message
Feature-A bug fix commit message
Feature-C commit message
Feature-B commit message
Feature-A commit message

Is this really readable without filtering tools?

Let's imagine a bigger team and more features developed...

Feature toggles and their removal will not help but add additional commits to this mess.

How branched development dose help to create a cleaner history.

Let's start from a repository having two features finished, two features under development and two bugfix commits done to the two finished features.

For this demo it doesn't matter if you are using branches per developer or per feature. If there are more than one developer working on the same feature whey have to coordinate their rebasing but that should be possible.
But to keep it simple let's assume there is one developer per feature.

Both feature branches need to be rebased after the bugfixes took place to have them integrated with the actual work.

When the first feature is finished (Feature-C in this demo) it's branch will be merged into the master. This is the correct situation to use merge and not rebase.

The resulting potential release will contain Feature-A, Feature-B, the two bugfixes and Feature-C. But not the Feature-D which is still under development.

The branch used for Feature-D must now be rebased to include Feature-C and to make sure it'll not break any previous development.

When finally Feature-D is finished and merged into the trunk your repository is in the same state as it was with release v1.1.0 but with a much cleaner history.

In a direct comparison you'll see how the related commits are grouped in the branch based approach.

Trunk Based	Branch Based
Feature-D commit message Feature-C commit message Feature-B bug fix commit message Feature-D commit message Feature-A bug fix commit message Feature-C commit message Feature-B commit message Feature-A commit message	Feature-D commit message Feature-D commit message Feature-C commit message Feature-C commit message Feature-B bug fix commit message Feature-A bug fix commit message Feature-B commit message Feature-A commit message

Conclusions

I do agree that branches add extra effort to the CI build. But I'm sure it's worth the effort.
I'll do a future post on how to do CI builds on multiple branches.

The problem with using resources in Java

Using resources in Java is often the cause of problems whenever the close() Operation is not granted to be run in all cases. The only solution to this behaviour is to always close any used resource in the finally block of a surrounding try block.

To make this problem clear I'm going to do different implementations of a FirstLineReader interface.

public interface FirstLineReader {

    /**
     * Read the first line of the given {@link File} and close any open Stream
     * on that file after reading.
     * @param inputFile the {@link File} to read from
     * @return the first line of the {@link File}
     * @throws IOException If an I/O error occurs
     */
    String readFirstLineOfFile(File inputFile) throws IOException;
}

How this was done in a pre Java-7 environment.

The long form doing it all manually

public class PurePreJava7FirstLineReader implements FirstLineReader {
    private final BufferedReaderFactory readerFactory;

    public PurePreJava7FirstLineReader(BufferedReaderFactory readerFactory) {
        this.readerFactory = readerFactory;
    }

    @Override
    public String readFirstLineOfFile(File inputFile) throws IOException {
        BufferedReader in = null;
        try {
            in = readerFactory.createBufferedReader(inputFile);
            return in.readLine();
        } finally {
            if (in != null){
                try {
                    in.close();
                } catch (IOException e){
                    // Do handle the Exception from close Operation
                }
            }
        }
    }
}

This way to implement a reading method was hard to read since it is that chatty. It was also always hard to figure out what to do with the IOException from the close() method. You don't want to throw it since it may overwrite an previous thrown Exception from the readLine() method. So most implementations have silently swallowed the Exception.
An additional shortcoming of this solution is the need to have the "in" field declared outside the try block.

Using IOUtils from commons-io to close the Stream quietly

public class IoUtilsUsingFirstLineReader implements FirstLineReader {
    private final BufferedReaderFactory readerFactory;

    public IoUtilsUsingFirstLineReader(BufferedReaderFactory readerFactory) {
        this.readerFactory = readerFactory;
    }

    @Override
    public String readFirstLineOfFile(File inputFile) throws IOException {
        BufferedReader in = null;
        try {
            in = readerFactory.createBufferedReader(inputFile);
            return in.readLine();
        } finally {
            IOUtils.closeQuietly(in);
        }
    }
}

This solution is a bit more readable by hiding the chatty close() implementation into the closeQuietly() service-method. But now it is hard coded to have no Exception handling on the close() method and no informations if there was an Exception.
You still need to declare the "in" field outside the try block.

The Java-7 solution by using the try-with-resources Statement

Oracle has introduced a new form of the try statement with Java-7 http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
Now it is possible to declare resources in the header of the try block. This resources must implement java.lang.AutoCloseable . On runtime the close() method of any resource is called as if it is implemented in the finally block.
There are two major differences:

You do not need to declare the field of the resource outside the try block.
You can access any thrown Exception from the try block.

The second difference makes this statement that useful. Starting with Java-7 the java.lang.Throwable class can now have suppressed Exceptions to handle situations that may have more than one Exception to be thrown. You can access this suppressed Exceptions by the java.lang.Throwable.html#getSuppressed() method.

public class UsingTryWithResourcesFirstLineReader implements FirstLineReader {

    private final BufferedReaderFactory readerFactory;

    public UsingTryWithResourcesFirstLineReader(BufferedReaderFactory readerFactory) {
        this.readerFactory = readerFactory;
    }

    public String readFirstLineOfFile(File inputFile) throws IOException {
        try (BufferedReader lineReader = readerFactory.createBufferedReader(inputFile)){
            return lineReader.readLine();
        }
    }
}

Possible Exception cases by using the try-with-resouces Statement

Exception from inside the try block but no Exception from the close() method

The Exception from inside the try block is thrown to be handled outside the try block and provides an empty Throwable[] from getSuppressed().

Exception from the close() method but no Exception from inside the try block

The Exception from the close() method is thrown to be handled outside the try block and provides an empty Throwable[] from getSuppressed().

Exception from inside the try block and another Exception from the close() method

The Exception from inside the try block is thrown to be handled outside the try block and provides a not empty Throwable[] from getSuppressed(). The Throwable[] from getSuppressed() contains the Exception thrown by the close() method.

closeQuietly() is now a Anti-Pattern

In the past I've seen many discussions about using closeQuietly() but all the pros and cons where about readability of the code vs. traceability of runtime behaviour.
I really hope this try-with-resources statement will bring an end to all this discussions. The code using this statement is clean, readable and easy to implement. The traceability of runtime behaviour is given for all Exception cases.

I would like to hear your opinion if there are any reasons to still use closeQuietly() methods.