search

nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.84k stars 636 forks source link

Recommended git hook commits unstaged changes if file has staged lines #223

Closed Cyberbeni closed 4 years ago

Cyberbeni commented 6 years ago

Now I don't have my work laptop with me, so I couldn't try it out, but I'm 99.9% sure this is what happens.

Here is the git hook I used for sorting imports in objective C files, that solves this problem: https://github.com/Cyberbeni/GitHooks/blob/master/pre-commit

tl;dr: git stash --keep-index before formatting and adding changes git stash pop after formatting and adding changes

Not sure if formatting can cause merge conflicts when popping the stash(sorting imports never did). In that case, I'd do a round of formatting without adding changes before stashing and then format again, add changes, pop stash.

isadon commented 6 years ago

I can confirm this exact problem as happening for me after I noticed super bad behavior (as described above) when using the hook as provided by the repo. This needs to be surfaced up and fixed immediately as it could be causing major (silent) damage to peoples commits!

@Cyberbeni How would you fix my config:

#!/bin/bash
 SWIFTFORMAT_FILEPATH=".swiftformat"

git diff --diff-filter=d --staged --name-only | grep -e '\(.*\).swift$' | while read line; do
    swiftformat --config $SWIFTFORMAT_FILEPATH $line "${line}";
    git add "$line";
done

I went and changed it to:

#!/bin/sh
#pre-commit git hook
SWIFTFORMAT_FILEPATH=".swiftformat"
CHANGED_FILES="$(git diff-index --no-commit-id --name-only --cached -r HEAD)"

git stash --keep-index
swiftformat --config $SWIFTFORMAT_FILEPATH $CHANGED_FILES
git add $CHANGED_FILES
git stash pop

Hope I wont have any issues šŸ˜•

nicklockwood commented 6 years ago

@donileo if you find a solution please open a PR.

isadon commented 6 years ago

Hi @nicklockwood, first wanted to say thanks for such a great binary! As far as fixes so far the changes I listed above are working and is definitely better than what is currently in master here but I want to do some further testing before a PR.

nicklockwood commented 6 years ago

@donileo any update on this?

isadon commented 6 years ago

@nicklockwood A little after using the above solution I continued having issues, which made made me pull back on using that solution. Iā€™ve just settled on manually running swift format via a simple script that runs it on all swift files, the script isnā€™t anything special.

I didnā€™t pursue finding a good commit hook, just havenā€™t had too much free time as of late.

nicklockwood commented 6 years ago

@donileo ok, thanks.

Cyberbeni commented 5 years ago

Totally forgot about this.

@donileo try this:

#!/bin/sh
#pre-commit git hook
CONFIG_PATH=".swiftformat"
CHANGED_FILES="$(git diff-index --no-commit-id --name-only --cached -r HEAD | grep -e '\(.*\).swift$')"

if [ "$CHANGED_FILES" == "" ]; then
    exit 0
fi

git diff --diff-filter=d --staged --name-only | grep -e '\(.*\).swift$' | while read line; do
    /usr/local/bin/swiftformat --config $CONFIG_PATH "$line";
done
git stash --keep-index
git diff --diff-filter=d --staged --name-only | grep -e '\(.*\).swift$' | while read line; do
    /usr/local/bin/swiftformat --config $CONFIG_PATH "$line";
    git add "$line";
done
git stash pop

This way you will have the formatted version of all changes in local and formatted version of staged changes in origin

Cyberbeni commented 5 years ago

I had some time to test the hook and fixed my previous comment. Now I just need to find the correct way to force stash pop

raphaeloliveira commented 4 years ago

@Cyberbeni, did you end up finalising your script? As you mentioned, the stash pop would cause a conflict.

Cyberbeni commented 4 years ago

@raphaeloliveira moved formatting to pre build phase as it's important to check if your code builds/runs correctly before commiting.

You could try this instead of git stash pop: git stash show -p | git apply && git stash drop

raphaeloliveira commented 4 years ago

Thanks for the answer @Cyberbeni. Unfortunately, that's what I'm trying to move away from. Currently, my build phases are too big and a simple rebuild without any code changes takes 5 seconds, which I'm trying to improve. Swiftformat takes 0.5~1 seconds, even when there are no changes. That's why I'm trying to move it to a pre-commit hook script that solves this unstaged files issue. I'm currently looking at this script https://github.com/hallettj/git-format-staged/blob/master/README.md.

raphaeloliveira commented 4 years ago

For those still searching for a script, I believe I found one that works well - https://github.com/hallettj/git-format-staged / https://www.olioapps.com/blog/automatic-code-formatting/.

I added the script on my project and run it on the pre-commit hook

./scripts/git-format-staged.sh --formatter 'swiftformat --swiftversion 5.0 --config ".swiftformat" stdin' './*.swift'

The stdin was the trick to make it work with git-format-staged.

It now correctly formats only the staged code. ā­ļø