David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43602 )
Change subject: git-hooks: Add check-style for pre-commit hook ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@11 PS1, Line 11: Add check-style rule to Makefile
It would need to be added to meson in this commit as well. […]
I'm not sure I understand... The pre-commit hook doesn't care about the build system. For now it just calls `make check-style` but could easily be modified to run the meson equivalent (which we should definitely add, I or someone just needs to figure out how).
The broader make vs. meson issue is another discussion, as you point out.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@14 PS1, Line 14: - Import checkpatch.pl, along with certain dependencies (spelling.txt : and const_structs.checkpatch) and .checkpatch.conf.
I am not so keen, -2 not so keen, on having coreboot's unreasonbly slow pre-commit hook stuff land here as well.
I did a few tests with this patch and `git commit -m` took around 0.4 seconds. I agree that all of coreboot's checks are slow, and I haven't looked into which ones are most worst offenders, but at least this seems pretty fast.
If spelling and that sort of thing should be checked then a build target that is run by the bot and a report generated is more reasonable imho.
I'm not entirely sure what you mean, but FWIW I don't think the spell checker currently works with this patch so we can try to leave it out (if possible). I actually didn't add it intentionally, checkpatch just gave me an error when I didn't have it available.
Regardless, in it's current form it isn't fit for purpose here yet. Can we take it out and just land a working clang-format we can all agree on and have that done please?
I'm not sure why it's not fit for purpose, but yes, if desired we can omit the checkpatch stuff and just have `make check-style` that runs clang-format.
BTW, here is sample output: https://paste.flashrom.org/view.php?id=3345
The `make check-style` part is the diff that is shown first, the checkpatch part is below where it says "Running checkpatch".
If we do as you suggest then the user will only see the diff, not the rules that provide rationale for the changes. I personally think the checkpatch output is nice to have, but am willing to remove it if you prefer it that way.
https://review.coreboot.org/c/flashrom/+/43602/1//COMMIT_MSG@16 PS1, Line 16: Add .clang-format from CB:3
I am in favour of clang-format coming to Flashrom and it has been suggested a number of times now.
Great! Feel free to add your thoughts to CB:38673. Also have a look at CB:42556 which uses uncrustify and has some comments about limitations I encountered with clang-format.
I don't recall precisely what Nico had to say but it was along the lines of Flashrom having sometimes longer lines than usual in some cases? Nico please correct me here.
There was a discussion about this on the mailing list a while back (maybe 2 years ago?) where it was decided that we'll allow up to 112 columns (tables can be longer). I more info on that to the wiki: https://flashrom.org/Development_Guidelines#Coding_style
In any case, I would be in favour of this part of the changeset and having it hooked up to the build bot as a target that is run as well as a pre-commit hook.
Agreed.
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit File util/git-hooks/pre-commit:
https://review.coreboot.org/c/flashrom/+/43602/1/util/git-hooks/pre-commit@2... PS1, Line 28: sleep 5 We can reduce or remove this if desired.