Use Gradle command exit code as hook exit code

[K4KYA]

The hook was using set -e, which meant that if the gradle command failed, then the saved unstaged changes in the diff file were never re-applied.

This change removes the use of set -e and Instead uses the exit code of the gradle task to indicate the success or failure of the hook after the unstaged changes are re-applied. This will ensure unstaged changes are always re-applied regardless of whether or not the check or format has failed or passed

[JLLeitschuh]

if the gradle command failed, then the saved unstaged changes in the diff file were never re-applied.

Sorry, I don't actually understand the implications of this. Could you elaborate a bit. Although I've been using git for years, my understanding of some of the terminology is somewhat lacking at times. A bit of a simplified explanation here would help me understand what this fix is attempting to resolve.

[K4KYA]

@JLLeitschuh Sure thing, I'd be happy to. Apologies if this goes a little too simple, but it's just in the interest of clarity.

On staging files in git

Let's say for example that we've made some changes in a repo where we've changed 2 files, Foo.kt and Bar.kt.
Now, let's say we want to commit the changes in 2 separate commits, committing Foo.kt first. First we'd stage the file we want to commit by adding the file to the index of staged files.

git add Foo.kt

Now Foo.kt is staged but Bar.kt is still un-staged. If at this point we run git commit we are only committing the changes in Foo.kt to the repo. The un-staged changes in Bar.kt are not included in this commit.

More on staging files can be found here

On how staged and un-staged changes are this is used in this git hook

So, now assume that our repo has this plugins pre-commit check git hook installed (just check, not format) and we want to commit the changes we made to Foo.kt. We run git commit add our commit message and then the pre-commit hook kicks off.

The pre-commit check first takes all of the un-staged changes, removes them from the repo and saves them to a file unstaged-ktlint-git-hook.diff, this happens when it runs this section of the hook:

diff=.git/unstaged-ktlint-git-hook.diff
git diff --color=never > $diff
if [ -s $diff ]; then
  git apply -R $diff
fi

If you were to run git status after this block was complete, you would not see any changes to Bar.kt in the working directory, staged or un-staged, it's as if the file was never touched. This is great because it means we're only running ktlint on the set of staged changes we want to commit.

Assuming there's no lint violations in Foo.kt, our ktlint check passes and we're satisfied that the staged changes are fine to commit. We can now re-apply the un-staged changes (leaving them un-staged because we don't want to commit them), this happens after the gradle command is run in our hook here:

if [ -s $diff ]; then
  git apply --ignore-whitespace $diff
fi
rm $diff

After this block, our changes to Bar.kt are back in our working directory, but will not be committed. The hook exits successfully (with an exit code 0) and our change to Foo.kt is committed to the index. This is all desired behaviour.

On the bug this PR fixes

So, imagine the above scenario again, except this time there's a lint violation in Foo.kt 😱 .

The hook script uses set -e so when the gradle command fails, the hook will stop after the gradle command and will exit with a non-zero error code. This indicates to the git process that the hook has failed the pre-commit check and the staged files should not be committed. But because we use set -e to exit immediately on failure, the remaining lines of the hook script after the gradle command will not run. This means that our changes to Bar.kt are never re-applied back to the repo!

At this point they do still exist in the .git/unstaged-ktlint-git-hook.diff file, but unless you were to inspect the hook script itself to find this filename, the author would just assume the changes had been wiped, and in fact if the author hadn't noticed the changes were gone and were to go ahead and fix the lint violation and try to commit Foo.kt again. Then when the hook eventually runs successfully, it would wipe the diff file and delete the unstaged changes forever. Depending on the authors workflow, this could constitute a significant portion of their work.

Examples

To illustrate this further, here's an example where I've added some additional logging to the script and ran it with and without a lint violation before and after this fix (with local paths for my repo redacted).

Condition	Output
Lint check pass
Lint check fail (Before fix)
Lint check fail (After fix)

I hope that helps. Do let me know if there's anything else I need to add to this change. From the checks, it looks like I need to update the changelog?

[JLLeitschuh]

First off, bravo on a very detailed and also very clear description of the issue. If you could figure out a way to encode this description into a comment (or at minimum link to your above comment) from a kotlin doc comment on the test you introduced that would be incredibly useful. Being able to, in the future, ask, "why is this test here the way it is" your explanation would be very useful.

At this point they do still exist in the .git/unstaged-ktlint-git-hook.diff file, but unless you were to inspect the hook script itself to find this filename, the author would just assume the changes had been wiped, and in fact if the author hadn't noticed the changes were gone and were to go ahead and fix the lint violation and try to commit Foo.kt again. Then when the hook eventually runs successfully, it would wipe the diff file and delete the unstaged changes forever. Depending on the authors workflow, this could constitute a significant portion of their work.

Wow, that does indeed seem bad. Thank you for this fix, let's get this in as soon as we can!

Please do add something to the changelog and add some sort of comment on the test you added and I'll be happy to merge this change. I'm hoping to do a release sometime this week.

[K4KYA]

Thanks for the feedback! 👍🏽

I've made those updates to the tests and changelog, so this should be good to go now. Would be great to get this into a release soon 🤞🏽

[JLLeitschuh]

Much appreciated! Thanks for this fix!

[ecabestan]

Hi,

Thanks a lot for this fix. I hop it will be intregated in a version soon.
Don't you think it is better in the title to explain what you fix (if possible from user point of view) instead of how you fix ?