InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.69k stars 921 forks source link

Update pre-commit hook #1511

Closed FintasticMan closed 8 months ago

FintasticMan commented 1 year ago

This updates the pre-commit hook to use git-clang-format.

Avamander commented 1 year ago

Detecting older and incompatible clang-format would be kinda nice.

FintasticMan commented 1 year ago

I could add a simple check that makes sure the version is at least 14.0.0. I could also add some slightly more complicated code that goes through all clang-format* executables in the PATH, and chooses the one with the highest version, and then makes sure that it's at least 14.0.0. Which of these do you think would be better?

FintasticMan commented 1 year ago

I've just implemented the more complicated option. It isn't very elegant sadly, if someone can come up with a better way to do it, I would appreciate it a lot. It should work in basically all situations though (except if the user has a space in their PATH, which they really shouldn't have and will probably break other things as well).

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 377496B -16B
data 940B 0B
bss 63420B 0B
FintasticMan commented 1 year ago

I've switched to a more robust method of getting the files that have changed. This doesn't rely on using human-readable output that could potentially be changed in an update.

I have a git stash with a bit more logic that more conclusively finds the highest clang-format version, by using git clang-format's --binary option. It does make the code less readable, and I think that this is probably alright as it is.

Riksu9000 commented 1 year ago

I'd be interested to see your new clang-format finding code regardless, if you could post a patch.

FintasticMan commented 1 year ago
diff --git a/hooks/pre-commit b/hooks/pre-commit
index e03b4217..60a01e34 100755
--- a/hooks/pre-commit
+++ b/hooks/pre-commit
@@ -1,23 +1,29 @@
 #!/bin/sh

+name="clang-format"
+
+if [ -z "$(command -v "git-$name")" ]; then
+  name="$(basename -a $(find $(echo "$PATH" | tr ':' ' ') -maxdepth 1 -type f -executable -name 'git-clang-format*') | sort | tail -n 1 | sed 's/^git-//')"
+fi
+
 minVersion="14.0.0"

-for file in $(find $(echo "$PATH" | tr ':' ' ') -maxdepth 1 -type f -executable -name 'git-clang-format*'); do
-  curName="$(basename "$file" | sed 's/^git-//')"
-  curVersion="$("$curName" --version | cut -d ' ' -f 3)"
+for file in $(find $(echo "$PATH" | tr ':' ' ') -maxdepth 1 -type f -executable -name 'clang-format*'); do
+  curBin="$file"
+  curVersion="$("$curBin" --version | cut -d ' ' -f 3)"

   if [ "$(printf '%s\n' "$curVersion" "$version" "$minVersion" | sort -V | tail -n 1)" = "$curVersion" ]; then
-    name="$curName"
+    bin="$curBin"
     version="$curVersion"
   fi
 done

-if [ -z "$name" ]; then
+if [ -z "$name" ] || [ -z "$bin" ]; then
   echo "Could not find a suitable clang-format installation. Install clang-format that includes the git-clang-format script, with at least version $minVersion"
   exit 1
 fi

-args='-q --extensions cpp,h --style file --staged -- :!src/FreeRTOS :!src/libs'
+args="--binary $bin -q --extensions cpp,h --style file --staged -- :!src/FreeRTOS :!src/libs"

 changedFiles="$(git "$name" --diffstat $args)"
 git "$name" $args

Here is a patch. The code for finding the clang-format binary is mostly the same, but the script now stores both the git-clang-format script name and the actual clang-format binary.