Make pre-commit hook only look at changed files

[dinomite]

kotlinter-gradle has most of the wiring necessary for a pre-commit hook, but it was disabled because it checks all files, not just those that are part of the to-be-committed changeset.

Make the pre-commit hook only examine files that are part of the changeset and wire it as a new task.

[jeremymailen]

Yeah, it's a good idea. Been meaning to get to this, but after all the work to get kotlin 1.4 compatibility out I took a bit of a hiatus.

[Noctiflore]

I saw you plan to do it using a git diff command to get the list of changed files, but modifying files and staging them without staging unwanted modifications from the working directory would still be hard.
Why don't stash the workdir (and unstash before real commit), as done here ? git diff can still be used to check if the hook needs to be run at all, and if there are unstaged modifications to stash.
Lint-staged also uses git stashes for the very same use-case.

[jeremymailen]

Thank you @Noctiflore, these are good references. Will definitely try to make use of them.

[Noctiflore]

Some other useful readings and details:

• my first link is a good starting point but fails with partially staged files.
• git-format-staged discusses the different alternatives ; a bit outdated, but still useful.
• one of the answers here propose to use git merge -Xtheirs instead of git stash pop, which may not be what is expected (overwrite linter changes)
• this article propose a good solution to handle a stash, and the specific case when stash push is a no-op.

I ended up writing this, inspired from these articles and git documentation:

#!/bin/bash

# Git pre-commit hook applying Ktlint on staged files
# (using user-defined gradle task)

STASH_NAME=pre-commit-$(date +%s)-$RANDOM
if ! git diff-files --quiet ; then
    # dirty workdir: stash to work on staged files only
    git stash push -q --keep-index -m "$STASH_NAME"
fi

echo "Pre-commit hook: Running ktlint (fix)"
gradle ktlintFix
RESULT=$?
# stage modified files
git add --update

# restore workdir
# get stash ID (stash{#}) (empty if workdir was clean)
STASH_ID=$(git stash list | sed -En "s/(stash@\{.*\}).*$STASH_NAME.*/\1/p")

if [[ -n "$STASH_ID" ]] ; then
    # git stash pop would conflict with changes kept in index
    # in partially added files : use diff/apply.
    git diff $STASH_ID^2 $STASH_ID | git apply
    if [ $? -eq 0 ] ; then
        git stash drop -q $STASH_ID
    else
        echo "Pre-commit hook did conflicting changes (see previous messages)"
        echo "Previous workdir state is stashed in $STASH_ID: $STASH_NAME"
    fi
fi

exit $RESULT

EDIT: switch to POSIX-compliant sed options (required for iOS).

[Mahoney]

For reference ktlint's own pre-commit hook is basically a one-liner:

git diff --name-only --cached --relative | grep '\.kt[s"]\?$' | xargs ktlint --relative .

Adding a -F to the ktlint command would add formatting. git diff --cached prevents unstaged files being included.

If the formatKotlin task took a --includes command line flag of type List<String> that specified the specific files to include it could take the same approach?

git diff --name-only --cached --relative | grep '\.kt[s"]\?$' | xargs printf -- '--includes %s\n' | xargs $GRADLEW formatKotlin 

[Mahoney]

Actually it's a bit more complicated than that... I ended up with:

set -euo pipefail
changed=$(git diff --name-only --cached --relative | grep '\.kt[s"]\?$' || true)
echo "$changed" | xargs ktlint -F --relative .
echo "$changed" | xargs git add
if git diff --name-only --cached --quiet; then
  # After formatting, no files were changed! Exit, otherwise we get an empty commit
  exit 1
fi

Or in my imaginary future version of this plugin:

set -euo pipefail
changed=$(git diff --name-only --cached --relative | grep '\.kt[s"]\?$' || true)
echo "$changed" | xargs printf -- '--includes %s\n' | xargs $GRADLEW formatKotlin 
echo "$changed" | xargs git add
if git diff --name-only --cached --quiet; then
  # After formatting, no files were changed! Exit, otherwise we get an empty commit
  exit 1
fi

[jeremymailen]

Yeah, this is useful, thank you. Today the formatKotlin task is a parent of the individual source set format tasks, but we could implement a standalone formatKotlinFiles task accepting command args and otherwise unattached to the build lifecycle.

[KotlinIsland]

This is how ktlint-gradle does it
https://github.com/JLLeitschuh/ktlint-gradle/blob/master/plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/GitHook.kt

    diff=.git/unstaged-ktlint-git-hook.diff
    git diff --color=never > ${'$'}diff
    if [ -s ${'$'}diff ]; then
      git apply -R ${'$'}diff
    fi
    
    ${generateGradleCommand(taskName, gradleRootDirPrefix)}
    echo "Completed ktlint run."
    ${postCheck(shouldUpdateCommit)}
    
    if [ -s ${'$'}diff ]; then
      git apply --ignore-whitespace ${'$'}diff
    fi
    rm ${'$'}diff
    unset diff

[mvarnagiris]

Is this still something that is being considered to be done? Right now for me running formatKotlin or lintKotlin adds about 22 seconds each on my machine on my large project. So pre-push hook extends the whole push process by more than 40 seconds. And this is on a powerful machine. Having those commands run on modified files only would allow us to use this hook. Right now we can't.

[kenyee]

+1 looking for something like this too

[kenyee]

This is one way to do it from the other ktlint plugin: https://github.com/JLLeitschuh/ktlint-gradle/blob/d0ca26e0a04b51222ff9d3812c682d57b2142427/plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/GitHook.kt#L222

but having a top level formatKotlinFiles accepting args would be more useful IMHO. The other way would involve running ktlint on every module.

[kenyee]

To follow up on this, I ended up using @Mahoney's xargs script above and using the raw pinterest ktlint executable instead.

If this is done via the gradle task, it takes a lot longer to run because Gradle has to start up, load the project, then run the ktlintFormatFiles task and if you have a project with 200+ modules, this is a big lag. Use the ktlint executable provided by the pinterest ktlint repo instead and it'll be much faster for your users.

[Mahoney]

I wonder if some of the configuration caching in Gradle 8, plus an active gradle daemon, would invert that? But I don't have a suitably huge gradle project to test it on. ktlint takes 1 second to lint one file for me, and I presume that a great deal of that is the JVM's startup & loading classes, which the daemon might avoid?

Unless doing it in docker or equivalent it's painful to have to keep an installed ktlint's version in line with this plugin - e.g. at the moment the released version of the plugin is on ktlint 0.47.1., but homebrew is on 0.48.2, and they do not agree on formatting rules.

[kenyee]

I don't think the config caching would help.
You can also tell homebrew to install a specific version. Or you can download a specific executable from the pinterest repo releases (the latter is what I do).