Transition to Post-Commit Hooks

[b-kaiser]

This pull request suggests transitioning from pre-commit to post-commit hooks to offer a less intrusive experience and reduce the risk of potential work loss. It also includes minor improvements to both the installation script and the hook logic.

[cuveland]

I think here we should first talk about the intended use of git clang-format.

In the current setup, as far as we know, there is no risk of data loss as the automatically executed script does not make any changes. The proposed changes would, in my opinion, make it much easier to create incorrectly formatted commits.

Maybe some of the problems occurred because the local-setup script was not called but sourced in the current shell instead?

[b-kaiser]

TL;DR

• I generally avoid pre-commit hooks.
• Do you mind if I make it a hidden option to install the script as post-commit hook? If not, I am happy to withdraw the pull-request.
• Do we care about each and every commit being correctly formatted? Or is it sufficient that only correctly formatted code gets merged into master (allowing incorrectly formatted commits in between).
• Would it be welcomed to (additionally) enforce correct formatting by other means, i.e. a GitHub Action that prevents merging of pull-requests that result in incorrectly formatted code on the master branch?

I agree that with the current setup the risk of accidental data loss is low and that the proposed changes intentionally enable incorrectly formatted commits. Also, I definitely agree that enforcing good formatting is a high priority. Although I faced issues with the script, those are likely specific to my setup rather then a widespread issue.

Personally, I tend to avoid pre-commit hooks, which motivated my suggestion for a change. Since the current setup is simple and likely works well for most users, I am happy to withdraw my suggestion. And I will disable the pre-commit hooks for my workflow only. Alternatively, what about modifying my PR and making it a hidden option to the install script to use a post-commit hooks instead? Would that be an option?

I believe that protecting the code base against incorrectly formatted commits is best achieved through automated tests that must pass before a pull request can be merged. To setup such a measure, one can write a GitHub Action that runs clang-format

on:
  pull_request:
    branch: master

on the code base and fails if the code is not formatted correctly. Afterwards, it is possible to set the corresponding setting Require status checks to pass before merging, which can be found under Settings -> Branches -> Branch protection rules, and selecting the master branch (possibly creating a new rule if necessary) and the GitHub Action as the required status check. I am happy to offer to write such a GitHub Action if something like this is desired.

In contrast to pre-commit hooks, this approach has the downside that not every commit is checked for correct formatting (well, technically one can write an appropriate GitHub Action for this). However, the advantage is that it can not be bypassed by a developer unaware of or choosing not to use the pre-commit hooks due to personal preference (such as myself).

The only question that remains is whether we are interested in having every single commit in the history correctly formatted or whether it is sufficient to not allow merges to master that result in incorrectly formatted code? So far, I did not take this into consideration.

[cuveland]

Please let me briefly explain why we arrived at the present solution.

Our thoughts:

• We would like to have mostly consistent formatting without unreasonable inconvenience.
• We would like to avoid formatting changes of existing code.
• We would like to be nudged to commit properly formatted code.

Our findings:

• We found that automatic formatting using clang-format works reasonably well, and that we can agree on the default options for almost all settings.
• We also found that different versions of clang-format may differ in their implementation of the formatting rules.

Our conclusions:

• Only the newly added or modified lines should be automatically formatted with everything else untouched. Luckily, git-clang-format offers exactly this feature.
• The automatic setup of a convenient Git pre-commit hook makes it easy to check if we forgot to run git-clang-format before committing.

There are certainly other possible solutions. But in our opinion, they have severe downsides. For example:

• We could automatically format all source code all the time in a CI. But this would create lots of automatic commits on every change to clang-format. As a result, the Git history would be full of minor formatting commits. And every time this happens, most branches/pull requests would be invalidated.
• We could specify a reference version of clang-format to use for everybody and "forever". Then, we could check the formatting on the server side in a pull request CI job before merging. This is exactly what the cbmroot project does, and it is extremely inconvenient. Because the reference version of clang-format may not be installed on your system, you often only find out after the action pipeline of the merge request has run what you should change. Many merge requests have a corresponding "Make CheckFormatting happy" commit accordingly. And the additional turnaround cycle further delays the merge request.

[b-kaiser]

Thank you @cuveland for your detailed answer in https://github.com/cbm-fles/flesnet/pull/165#issuecomment-2147349894 .

Would it make sense to put its content to some doc/code-formatting.md?