GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Update pre-commit hook by FintasticMan · Pull Request #1511 · InfiniTimeOrg/InfiniTime

Conversation

@FintasticMan

@FintasticMan FintasticMan commented Jan 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Riksu9000
Riksu9000 requested a review from Avamander January 4, 2023 21:01
@FintasticMan
FintasticMan force-pushed the commit_hooks branch from fbb8283 to 7cfe2b9 Compare January 9, 2023 00:55
@Avamander

Avamander commented Jan 9, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Jan 9, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Jan 9, 2023

Copy link
Copy Markdown
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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@FintasticMan
FintasticMan force-pushed the commit_hooks branch from 8d2477c to fcdf486 Compare January 21, 2023 21:55
Riksu9000
Riksu9000 reviewed Feb 1, 2023
View reviewed changes
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Riksu9000
Riksu9000 reviewed Feb 25, 2023
View reviewed changes

@Riksu9000 Riksu9000 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan
FintasticMan force-pushed the commit_hooks branch from fcdf486 to 3457040 Compare February 28, 2023 20:17
@github-actions

github-actions Bot commented Feb 28, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build size and comparison to main:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Riksu9000
Riksu9000 suggested changes Feb 28, 2023
View reviewed changes
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

Riksu9000
Riksu9000 suggested changes Feb 28, 2023
View reviewed changes
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan
FintasticMan force-pushed the commit_hooks branch from 3457040 to 7c2fabb Compare February 28, 2023 22:29
Riksu9000
Riksu9000 suggested changes Mar 4, 2023
View reviewed changes
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

FintasticMan commented Mar 4, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Riksu9000

Riksu9000 commented Mar 4, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan

FintasticMan commented Mar 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Riksu9000
Riksu9000 reviewed Mar 4, 2023
View reviewed changes
Comment thread hooks/pre-commit Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FintasticMan
FintasticMan force-pushed the commit_hooks branch from a3ec514 to 5b3db43 Compare March 4, 2023 20:40
FintasticMan added 3 commits November 16, 2023 12:11
@FintasticMan
hook: Update pre-commit git hook
46e8341
@FintasticMan
hook: Use clang-format with highest version
a7f130f
@FintasticMan
hook: Find correct clang-format version better
77af275
@FintasticMan
FintasticMan force-pushed the commit_hooks branch from 5b3db43 to 77af275 Compare November 16, 2023 11:14
@FintasticMan
FintasticMan requested a review from a team November 17, 2023 08:45
JF002
JF002 approved these changes Jan 10, 2024
View reviewed changes
@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

Reviewers

@JF002 JF002 JF002 approved these changes
@Avamander Avamander Awaiting requested review from Avamander
+1 more reviewer
@Riksu9000 Riksu9000 Riksu9000 requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

maintenance Background work

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@FintasticMan @Avamander @Riksu9000 @JF002
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.