Added git-clang-format as a pre-commit hook

[kjetilly]

I am opening this PR also to create a discussion. Before you go on reading, please note that this can be selectively disabled by exporting the SKIP environment variable to contain git-clang-format-fix=1, so eg.

SKIP=git-clang-format-fix=1 git commit -m 'Did stuff I do not want formatted'

As far as I can tell, there is no ready-made pre-commit hook for this, so a bash script is the solution for now.

Do note that this applies git-clang-format, so it will only clang-format the diff.

Example workflow where I have added extra spacing that should not be there:

$ git commit -m 'Made meaning more spatious'
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
git-clang-format (diff-only, auto-fix)...................................Failed
- hook id: git-clang-format-fix
- exit code: 1

diff --git a/opm/models/blackoil/blackoilprimaryvariables.hh b/opm/models/blackoil/blackoilprimaryvariables.hh
index 8aa1524d8..8030ceaf7 100644
--- a/opm/models/blackoil/blackoilprimaryvariables.hh
+++ b/opm/models/blackoil/blackoilprimaryvariables.hh
@@ -181,8 +181,8 @@ public:
         result.primaryVarsMeaningGas_ = GasMeaning::Rv;
         result.primaryVarsMeaningPressure_ = PressureMeaning::Pg;
         result.primaryVarsMeaningWater_ = WaterMeaning::Rsw;
-        result.primaryVarsMeaningSolvent_          = SolventMeaning::Ss;
-        for (std::size_t i = 0; i < result.size(); ++i)              {
+        result.primaryVarsMeaningSolvent_ = SolventMeaning::Ss;
+        for (std::size_t i = 0; i < result.size(); ++i) {
             result[i] = i + 1;
         }
 
[git-clang-format] Applying formatting to staged changes...
changed files:
    opm/models/blackoil/blackoilprimaryvariables.hh

[git-clang-format] Changes have been applied.
Please review and re-run your commit.


opm-simulators on  precommit [$!?] via  v3.31.6 
$ git commit -m 'Made meaning more spatious''
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
git-clang-format (diff-only, auto-fix)...............(no files to check)Skipped
[precommit a4c90e0b7] added new precommit
 1 file changed, 2 insertions(+), 2 deletions(-)

Note that I have for the proof-of-concept kept the shell-script in the YAML-file. It is probably worthwhile to move this to a separate file, or even a separate repo if we want this for opm-common and grid as well, but before that I think it is wise to discuss wether this is something we want.

[atgeirr]

this can be selectively disabled

Does that mean this is opt-out if merged? Can we change to opt-in easily?

[kjetilly]

this can be selectively disabled

Does that mean this is opt-out if merged? Can we change to opt-in easily?

It is opt-out if you have pre-commit enabled in the repository (most people don't I believe).

It is possible to make it opt-in, but that is not a functionality that is supported natively in pre-commit, so we would have to manually support with adjusting the shell script to something like:

if [ -z "$I_WANT_FORMAT" ]; then <clang format stuff>; fi

I am a bit negative to such an approach, simply because it is not natively supported by pre-commit itself, and they have the machinery to disable already.

Alternatively, we could remove it altogether as a part of the pre-commit hook stage, and only make it runnable through manually calling pre-commit, but that kind of defeats the purpose I think.

[hakonhagland]

Maybe add a note that it will use the .clang-format file in the root? Also what will happen if git-clang-format is not installed?

[hakonhagland]

This will stage everything in the repo, right? We should not add back files that were not staged originally.

[hakonhagland]

This looks good. I am concerned about the column width of 120 in the .clang-format file which makes it difficult for me to read the source after formatting. I am using a laptop with a larger font and a width of more than 95-100 characters is gonna wrap in VS Code or cursor. That is the main reason I am not using clang-format.

[kjetilly]

I am all for reducing to 100 characters per line, but I think @atgeirr was the one to set the original limit of 120 characters, even though I've seen him code in an Aquamacs window with 80 characters per line.

[hakonhagland]

I am all for reducing to 100 characters per line

Added PR for this suggestion, see #6594

[blattms]

there should be no need for bash. I would rather use sh.

[blattms]

I am against this unless somebody excludes all files that are copies from somewhere else. I am just looking at code that got copied here for which we never made any attempt to contribute back. I am already having a hard time with humans making "janitor" changes.

With this we would now even start making automatic changes to those files just for sake of formatting. This will make it really hard to track any upstream or downstream changes.

[kjetilly]

I am against this unless somebody excludes all files that are copies from somewhere else. I

I think having a "no-go" list should be rather straightforward to implement, and might also be relevant for other files as well. This is something I can try to add, along with the sh-request.

Roughly how many files are we talking about that are "copies from somewhere else"? While not messing up the formatting makes it easier to contribute back, having a copy of them in the first place (from what I assume is an actively maintained codebase) seems much less ideal to begin with.

[hakonhagland]

This will make it really hard to track any upstream or downstream changes

@blattms Good catch. We should only autoformat files that are already formatted according to .clang-format style or for new files. Not mess up files that have a different formatting from the start.

[atgeirr]

With this we would now even start making automatic changes to those files just for sake of formatting

I assumed that this would only affect the staged changes in a commit, not the entire file? Still, even if only making changes to the staged code I see that can be problematic.

[kjetilly]

With this we would now even start making automatic changes to those files just for sake of formatting

I assumed that this would only affect the staged changes in a commit

Correct. It depends on the janitoring done, I suppose. Suddenly a smaller change can make big formatting differences, making merging back difficult.

As always, there should probably be made a distinction between files we have inherited and don't plan to contribute back, and files we are simply "borrowing". And of course, for files that are simply too far away from the clang-format-style. Maybe a pragmatic approach here is to expand the "no go" list as we go along.

[atgeirr]

Maybe a pragmatic approach here is to expand the "no go" list as we go along.

Maybe we could put something in each no-go file like this:

// File should not deviate significantly from its origin dune/somedir/somefile.hh
// clang-format: off

That would be better than maintaining a list I think, it would also make clear why one should not autoformat it.

[blattms]

As always, there should probably be made a distinction between files we have inherited and don't plan to contribute back, and files we are simply "borrowing".

There simply should be no files that you copy from somewhere else and never contribute back or never check for bug fixes. Ideally you would never copy files as they might become ticking bombs...

[kjetilly]

As always, there should probably be made a distinction between files we have inherited and don't plan to contribute back, and files we are simply "borrowing".

There simply should be no files that you copy from somewhere else and never contribute back or never check for bug fixes. Ideally you would never copy files as they might become ticking bombs...

Ideally I agree with you, but sometimes files are copied from abandoned or archived repositories. Sometimes the source repository has a very different goal where it is impossible to join forces.

Again, ideally I agree, but realistically I think it is not always implementable.

[blattms]

BTW: I tried to run clang-format on my Debian 12 system and it fails:

$ clang-format opm/simulators/linalg/mixed/*
/home/mblatt/src/dune/opm/opm-simulators/.clang-format:17:9: error: unknown key 'InsertNewlineAtEOF'
        InsertNewlineAtEOF: true,
        ^~~~~~~~~~~~~~~~~~
Error reading /home/mblatt/src/dune/opm/opm-simulators/.clang-format: Invalid argument

Am I doing something wrong?

[kjetilly]

BTW: I tried to run clang-format on my Debian 12 system and it fails:

$ clang-format opm/simulators/linalg/mixed/*
/home/mblatt/src/dune/opm/opm-simulators/.clang-format:17:9: error: unknown key 'InsertNewlineAtEOF'
        InsertNewlineAtEOF: true,
        ^~~~~~~~~~~~~~~~~~
Error reading /home/mblatt/src/dune/opm/opm-simulators/.clang-format: Invalid argument

Am I doing something wrong?

Debian 12 supplies clang-format 14 as far as I can tell, while the InsertNewlineAtEOF was introduced in clang-format 16, and clang-format does not like parsing files with unknown keywords. In Ubuntu 24.04 LTS clang-format is at version 16, so should support this keyword.

Since we anyway have the new pre-commit hook (in master) adding newlines, I guess this is no longer needed in clang-format, so we could remove it.