Skip to content

Conversation

@FintasticMan
Copy link
Member

@FintasticMan FintasticMan commented Jan 3, 2023

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

@Avamander
Copy link
Collaborator

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

@FintasticMan
Copy link
Member Author

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
Copy link
Member Author

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).

@Riksu9000 Riksu9000 added the maintenance Background work label Jan 10, 2023
@FintasticMan FintasticMan force-pushed the commit_hooks branch 3 times, most recently from 842c26d to 8d2477c Compare January 14, 2023 14:28
Copy link
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably follow the same conventions regarding variable naming.

@github-actions
Copy link

github-actions bot commented Feb 28, 2023

Build size and comparison to main:

Section Size Difference
text 377496B -16B
data 940B 0B
bss 63420B 0B

@FintasticMan FintasticMan force-pushed the commit_hooks branch 2 times, most recently from 8752c63 to a3ec514 Compare March 4, 2023 20:13
@FintasticMan
Copy link
Member Author

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
Copy link
Contributor

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

@FintasticMan
Copy link
Member Author

FintasticMan commented Mar 4, 2023

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.

@FintasticMan FintasticMan requested a review from a team November 17, 2023 08:45
@FintasticMan FintasticMan merged commit 0503248 into InfiniTimeOrg:main Jan 12, 2024
@FintasticMan FintasticMan deleted the commit_hooks branch January 12, 2024 13:42
@FintasticMan FintasticMan added this to the 1.15.0 milestone Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Background work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants