Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pre-commit hook #1511

Merged
merged 3 commits into from
Jan 12, 2024
Merged

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
hooks/pre-commit Outdated Show resolved Hide resolved
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.

hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
@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

hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
hooks/pre-commit Outdated Show resolved Hide resolved
@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.

hooks/pre-commit Outdated Show resolved Hide resolved
@FintasticMan FintasticMan requested a review from a team November 17, 2023 08:45
@FintasticMan FintasticMan merged commit 0503248 into InfiniTimeOrg:main Jan 12, 2024
5 checks passed
@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