pre-commit-hooks icon indicating copy to clipboard operation
pre-commit-hooks copied to clipboard

clang-format stops working when using --version

Open mintar opened this issue 2 years ago • 5 comments

With the default test repo (pre-commit-hooks/tests/test_repo) and the following config...

fail_fast: false
repos:
  - repo: https://github.com/pocc/pre-commit-hooks
    rev: v1.3.5
    hooks:
      - id: clang-format
        args: [--version=10.0.0, --style=Google]

... the following happens (incorrectly, it should detect the incorrect formatting):

$ pre-commit run -a
clang-format.............................................................Passed

But when I remove --version=10.0.0, it's working:

$ pre-commit run -a
clang-format.............................................................Failed
- hook id: clang-format
- exit code: 1

err.cpp
====================
--- original

+++ formatted

@@ -1,2 +1,5 @@

 #include <string>
-int main(){int i;return;}
+int main() {
+  int i;
+  return;
+}
err.c
====================
--- original

+++ formatted

@@ -1,2 +1,5 @@

 #include <stdio.h>
-int main(){int i;return;}
+int main() {
+  int i;
+  return;
+}

mintar avatar Apr 06 '22 18:04 mintar

Thank you for the feedback. I see this is an issue, and I'm investigating it.

pocc avatar Apr 13 '22 05:04 pocc

This is the case for clang-tidy as well. Any ideas on how to fix this?

nielskm avatar Sep 23 '22 10:09 nielskm

Isn't the problem simply that you don't remove the --version argument from args in Command.parse_args.

nielskm avatar Sep 23 '22 10:09 nielskm

I get the same error, specifying the version "fixes" it

ypearson avatar Nov 23 '22 15:11 ypearson

It seems likely that the problem is with assert_version(). That method exits with a success code if the correct version of the executable is found, which pre-commit interprets as the hook running successfully.

That means that execution never continues once assert_version()... either an exception is raised if the wrong version is found or the program exits successfully if the right version is found.

It seems like maybe the successful exit line should be removed so that the hook can actually run after the version is confirmed.

If that looks like the right fix, I'm happy to submit a PR if that's welcome @pocc

ascended121 avatar Jul 06 '23 16:07 ascended121