On 2025-06-19 at 16:38:35, Junio C Hamano wrote: > Cc-list chosen from "git shortlog --since=12.months --no-merges .clang-format". > > I am wondering how often our developers use "make style" aka > > git clang-format --style file --diff --extensions c,h > > and also wondering if the suggested style fixes are really > "improvements". For example, taking randomly the latest patch I > just injested into my tree, i.e. > > $ git am a-single-patch-file.txt > $ git reset --soft HEAD^ > $ make style I don't at the moment. In my view, the main utility of tidy tools is that the project has picked a style, whatever everyone may think of it[0], and we apply the tool consistently on every change and enforce it. Then, the application of the tidy tool becomes a rote keystroke in one's editor and one does not need to think about it. This is how things work in most Rust and Go projects, for instance, since they have well-defined tidy tools. That is not how things work here, however. > I got the output attached at the end of the message. The result is > a mixed bag (I commented on the "patch" as if it were a patch > submission). > > I have this suspicion that nobody complained these sub-par > suggestions the tool makes based on what we have in .clang-format > because not many folks run "make style", and "make style" is not > very easy to use after you record your changes into a commit. IOW, > there is nothing packaged to help "I have four commits on top of the > upstream, I want to run style checks before running format-patch", > i.e. > > git clang-format --diff HEAD~4 I agree most people probably do not use it, probably for the reasons I don't. I don't know if clang-format produces stable output: that is, using a newer version of clang-format with the same config does not result in diff changes. If it does, then we can simply pick a set of style configs and a minimum version and tell people to apply it. We can then check it in CI and if CI fails, we can output a base64-encoded diff (since it's going to have lots of whitespace, base64 encoding will be practically useful) that the author can apply. Then people using esoteric systems without clang-format can simply apply the diff from CI. If clang-format does not produce stable output, we're going to have a bunch of practical problems. I use Debian unstable at home and I know Peff does as well, but I also use Ubuntu 24.04 at work. Some contributors use Fedora or Cygwin, and we're all going to have a giant problem picking a consistent version of clang-format to use such that people don't have to compile their own or use external packages. Perhaps we can create a small script that does the tidying in a Linux Docker/Podman container in that case. > Even the output from the tool is of mixed quality, there are good > pieces that can be used to improve your patches. So we may prefer > to see the tool used more often, but not in a way to suggest its > output is always better than what the human developer has written. I really would prefer us to pick a set of standards that is good enough and just apply them. I agree clang-format may not produce ideal output, but I really do not want to think about formatting and style and whether my lines exceed 80 characters. Fixing those style issues is annoying and I can say that it often delays me getting to re-rolls. > For that, there are a few things we'd probably need to do: > > - Improve our tooling so that the develper can check a range of > commits they made before running format-patch, and other > situations. I agree better tooling would be valuable. > - Improve .clang-format rules to reduce false positives. I think we should iterate on the rules a bit to get them to good enough and then commit to the style. [0] Go's tool, gofmt, even acknowledges that the style it uses is nobody's favourite, but having it is better than bikeshedding arguments over style. -- brian m. carlson (they/them) Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
