Ooga-chaka: Git hooks to enforce code quality

    Ooga-chaka: Git hooks to enforce code quality
    Cover image: “Gold and Blue” by Edoardo Brotto — on flickr

    In this article we’ll see a great way to make sure you never, ever commit code that has not been properly formatted, using a little known but extremely powerful feature of Git, some Gradle hacks, and a simple script.


    If you work in a team you will surely have felt the pain of not being all on the same page when it comes to code style and formatting. This is even more common for open source projects, where external contributors might have not gotten a hang of the rules because they’re new to the project.

    Fortunately, that is a problem that we have mostly solved with a CI running static analysis in conjunction with protected branches, and with keeping IDE project settings in Git. This isn’t always enough though: even the best intentioned developer can forget to do that last cmd-alt-l before committing, or to run check on the project before pushing.

    We all know what comes next. Computer says ‘no’:

    It is particularly maddening to me to have a stray empty space at the end of a line that slipped through waste several minutes of CI time and require me to go back to the right branch, amend, and push. Not only it’s a silly mistake that I could’ve easily avoided, but I will also have to wait for the CI to green light it again before I can start spamming my PR on Slack, asking for reviews.

    There oughta be a way these sort of silly mistakes can be avoided!

    Luckily for you (and me!), there is a way to stop unformatted code to ever get to the CI, and it all starts with Git hooks.

    Git hooks

    Git hooks are a mechanism that allows arbitrary code to be run before, or after, certain Git lifecycle events occur. For example, one could have a hook into the commit-msg event to validate that the commit message structure follows the recommended format.

    The hooks can be any sort of executable code, including shell, PowerShell, Python, or any other scripts. Or they may be a binary executable. Anything goes! The only criteria is that hooks must be stored in the .git/hooks folder in the repo root, and that they must be named to match the corresponding events (as of Git 2.x):


    Using hooks to run static analysis tools

    Since hooks can consist of any arbitrary executable code or script, this means we can use them to run static analysis! After all, we already have everything we need to catch all the small and big snafus we’re trying to prevent. The checks are already implemented in most cases, since they’re exactly the same that the CI runs to validate builds. This means we only need to invoke the desired tools from a hook, and Git will make sure the operation is aborted if they find any problem.

    In Squanchy’s case, I decided to hook into the pre-commit hook as it seems the most appropriate for our team’s needs. In most cases the pre-push hook will work better for you, though. More on that later on.

    The only question left open is then what scripting language to use. We want something portable enough that can run on as many OSes as possible without requiring installation of extra languages/runtimes, and that developers are familiar with.

    I had initially considered a shell script that ran the CLI interfaces for Detekt and KtLint, but that requires additional set up in the script, which may diverge over time from the build run by the CI. In light of that, I opted for a simple hook that just runs the detektCheck Gradle task, instead:

    #!/bin/sh
    
    echo "Running static analysis..."
    
    # Inspect code using Detekt
    ./gradlew app:detektCheck --daemon
    
    status=$?
    
    if [ "$status" = 0 ] ; then
        echo "Static analysis found no problems."
        exit 0
    else
        echo 1>&2 "Static analysis found violations it could not fix."
        exit 1
    fi

    This hook, which is saved as $REPO_ROOT/.git/hooks/pre-commit, runs the Gradle wrapper, collects its exit code, and uses it to print a meaningful message and emit the appropriate exit code. This is a shell script, which makes it only compatible with *NIX systems (Linux and macOS), but given that Android developers tend to use those systems, I feel like it’s not a big issue. It could be very easily implemented for Windows as well, but I do not have a Windows dev machine to test on, so I left its implementation as a task for future me.

    We have everything in place now to ensure Detekt is happy with whatever we’re committing. While we’re at it, though, we could also run KtLint; since KtLint has a code formatter too, we could take advantage of that to minimise the amount of aborted commits due to formatting issues. KtLint’s formatter would still break the build if it runs into style issues it is not able to fix. After the formatting, we can run Detekt and KtLint analysis, which should not trigger for most formatting issues.

    In case you were wondering, Detekt is also able to format code, but its formatting is not as powerful as KtLint’s formatter, since it is basically just triggering IDEA’s command line formatter. Another annoyance is that it requires a local copy of IDEA/AS to be installed, which is not workable on a CI.

    The finished hook looks like this:

    #!/bin/sh
    
    echo "Running static analysis..."
    
    # Format code using KtLint, then run Detekt and KtLint static analysis
    ./gradlew app:ktlintFormat app:detektCheck app:ktlint --daemon
    
    status=$?
    
    if [ "$status" = 0 ] ; then
        echo "Static analysis found no problems."
        exit 0
    else
        echo 1>&2 "Static analysis found violations it could not fix."
        exit 1
    fi

    All good things take time

    When you commit, you’ll notice that the hook has a side effect: commits take some time to complete, instead of being near-instantaneous. This happens because before confirming that a commit is valid, our hooks needs to spin up Gradle, then run Detekt and KtLint.

    And here lies the critical point. You don’t want your commits to take too long, or you risk annoying your teammates and contributors. In and by itself, running Detekt and KtLint on Squanchy’s codebase requires at most a couple of seconds, but using Gradle carries some overhead. In Squanchy’s current setup, this pre-commit hook makes commits take between 3 and 5 seconds on my Macbook Pro.

    Five seconds is still acceptable, but as you start having more code to analyse, or if you add more tools to the hook, you’ll see times increasing. At that point, doing a commit becomes tediously slow.

    Because of that, we’re not running Findbugs, PMD, nor Checkstyle on Squanchy even though we still have some Java code, because these tools are much slower than Detekt and KtLint. In the same vein, you likely don’t want to run the tests every time you commit; for as nice as that would be, even running unit tests can be slow when there’s many of them, so you want to consider the UX tradeoffs when using hooks.

    Hooks are a powerful tool and they require care and consideration not to become annoying.

    What could we do if the time to perform a commit becomes unbearably slow, then? While we haven’t had that problem in Squanchy yet, the first approach would be to only leave the ktlintFormat task in the pre-commit hook, which should be quite fast to run. We could move detektCheck and ktlint to a pre-push hook, instead of dropping them entirely.

    The more we offset slower tasks onto such an inherently slow and low frequency event, we can better “disguise” the overhead they add to run, leaving the “hot path” UX of committing lean and fast. In the end, if things become too aggravating, team members will just start using --no-verify in all their commits, nullifying all the work we put in.

    Now that we have everything in place for our hooks, we can take a step back and see how this would work for our teammates. We’re not quite done yet, as we have still a few roadblocks on our way to automating away consistency issues.


    Roadblocks? Just hold my beer 🍺

    The first roadblock is represented by sharing the hooks themselves. As we mentioned, they live in the .git folder, which is where all the Git metadata is stored. As such, that folder is not under version control — and rightly so, since most of its contents are device and user-specific.

    How can we make sure our hooks are transmitted along with the rest of the repository to all the users of the codebase, then? The simplest way is to store them in some regular folder in the repo, and copy them over to .git/hooks on each machine.

    For us enthusiast automators this looks like an imperfect solution, though. Every user would have to manually copy their hooks from the folder into .git/hooks, and we’d fall again into the situation where we depend on manual user actions. It’s a vicious circle.

    Experience tells us that relying on users to perform non-essential manual operations, and even just reading documentation, is just a utopia. For this reason we need to come to a more “zero-click” solution, such as a script that installs the hooks (that is, copy it over) without any user intervention. Out of sight, out of mind.

    In Squanchy, which uses the standard Novoda team-props scaffolding, I have created a folder to put all the hooks in:

    As you’ll notice, the hook has a .sh extension. That’s just so that editors use the correct syntax highlighting, but it gets removed when the hooks are installed to .git/hooks.

    Once you have your hooks in the repository, go ahead and put them under version control. Next we’ll write the script to install them.

    Gradle as a scripting platform

    While I could’ve easily written a shell or python script to install the hooks, I instead created a custom Gradle task. Why?

    • Gradle tasks are platform-independent
    • Users aren’t required to use a specific shell, or to have a specific language runtime available (Python is not preinstalled on Windows, for example)
    • I felt like learning a bit more about how to write Gradle tasks 😄

    Here’s what I came up with:

    static def isLinuxOrMacOs() {
        def osName = System.getProperty('os.name').toLowerCase(Locale.ROOT)
        return osName.contains('linux') || osName.contains('mac os') || osName.contains('macos')
    }
    
    task copyGitHooks(type: Copy) {
        description 'Copies the git hooks from team-props/git-hooks to the .git folder.'
        from("${rootDir}/team-props/git-hooks/") {
            include '**/*.sh'
            rename '(.*).sh', '$1'
        }
        into "${rootDir}/.git/hooks"
        onlyIf { isLinuxOrMacOs() }
    }
    
    task installGitHooks(type: Exec) {
        description 'Installs the pre-commit git hooks from team-props/git-hooks.'
        group 'git hooks'
        workingDir rootDir
        commandLine 'chmod'
        args '-R', '+x', '.git/hooks/'
        dependsOn copyGitHooks
        onlyIf { isLinuxOrMacOs() }
        doLast {
            logger.info('Git hook installed successfully.')
        }
    }

    I created a new git-hooks.gradle file in the team-props folder to keep the build scripts tidy, but you could have this in the root build.gradle file too. If you use a separate file, don’t forget to import it into the root build.gradle too, so its tasks will be available to the root project:

    apply from: teamPropsFile('git-hooks.gradle')

    You might have noticed that the tasks don’t do anything if the build is run on Windows. This is not due to a limitation in the scripts or the hooks, but as mentioned earlier I just haven’t written a Windows-compatible script yet, so there’s no hooks to install on that platform.

    The logic consists of two tasks, copyGitHooks and installGitHooks. The former is a copy task that copies all the *.sh files from team-props/git-hooks to .git/hooks, at the same time stripping them of their extension. The latter task, which depends on copyGitHooks, is an exec task which ensures all the files in .git/hooks are executable by running chmod -R +x on the folder.

    Both tasks have an onlyIf condition that limits their execution: they only run if the OS is Linux or macOS, for the reasons outlined earlier. Depending on your setup, you may also want to add a “not running on a CI” condition in the onlyIf block, but for my needs — the CI never commits, and the workspace is pruned after jobs run — it wasn’t really necessary; running the task only takes a few milliseconds anyway, and even if it’s not strictly needed to run it, it’s not a big deal.

    We’re inching closer to our goal now, but we still have one roadblock: users need to run the task manually. Let’s take care of it.

    Gradle as a trojan horse

    Installing hooks is as simple as running a Gradle task, but it’s still a manual process. I know, for one, that I would forget to run installGitHooks when switching branches or when a new version was pushed to my branch. Is there a way to ensure team members and external contributors don’t need to do anything at all to enjoy our nitpicky static analysis on each commit?

    Well yes, there is. It’s a bit of a hack, and it’s absolutely a sneaky move; you might not like it, but we can solve this last problem by turning Gradle into our hook’s trojan horse:

    afterEvaluate {
        // We install the hook at the first occasion
        tasks['clean'].dependsOn installGitHooks
        tasks['assemble'].dependsOn installGitHooks
    }

    With these few lines, we’ve ensured whenever someone builds the project, or cleans it, will have the hooks installed. This way we can ensure that everyone is adhering to the same code formatting rules, and that the code everyone commits or pushes is at least superficially bug and lint free.

    If you’re wondering about the impact of this task on build times, then I have good news: there’s almost none. Copying a couple of tiny files and setting their permission takes a few milliseconds, and it makes no difference when compared to how long even the simplest of builds generally take.

    Conclusions

    We’ve found a way to potentially save ourselves and our team members a lot of precious time and frustration. Reformatting code automatically means nobody will see the CI or a reviewer blocking a PR because of some silly oversight. Running static analysis before allowing code to be committed or pushed means that we’re never going to push code that should be perfected, or contains potential sneaky bugs.

    Yay automation! ✨
    Sebastiano Poggi

    Sebastiano Poggi

    “It depends” 🤷‍♂️ — Google Developer Expert for Android, Flutter and Identity. A geek 🤓 who has a serious thing for good design ✨ and for emojis 🤟working at JetBrains (opinions my own)

    London and elsewhere https://sebastiano.dev