[v2,0/8] clang-format: add more rules and enable on CI

Message

karthik nayak July 11, 2024, 8:30 a.m. UTC

This is v2 of my series to add clang-format to the CI.

The series was mostly inspired by a patch sent recently to 'include
kh_foreach* macros in ForEachMacros' [1]. I was wondering why we don't
run the formatting on CI and reduce some of the workload of reviewers.

We have a '.clang-format' file to provide rules for code formatting.
The commits 1-6 aims to add more rules to the file while deprecating old
ones.

Commit 7 enables CI action on GitHub and GitLab to also check for the
code formatting. Currently, it is allowed to fail. This way we can build
confidence and fine tune the values as needed and finally enforce the
check in a future patch. I'm not well versed with GitHub workflows, and
I mostly tried to copy existing work there. Expecting some feedback in
that section!

Commit 8 fixes an existing issue with the 'check-whitespace' job, which
is failing as success in the GitLab CI.

1: https://lore.kernel.org/git/[e-mail]/

Changes against v1:
* Fixed a lot of typos.
* Added more details about the warnings specified by clang-format for
'RemoveBracesLLVM' in commit 5. Added more details too.
* The last two commits were modified to use "CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
and fallback to "CI_MERGE_REQUEST_DIFF_BASE_SHA" if the former is not
available in GitLab's CI.
* Use `!/bin/sh` for the run-style-check script.
* Modified the last commit to remove block around the && check in the
whitespace script.
* Use `git rev-parse --verify --quiet` to verify the base commit.


Karthik Nayak (8):
  clang-format: indent preprocessor directives after hash
  clang-format: avoid spacing around bitfield colon
  clang-format: ensure files end with newlines
  clang-format: replace deprecated option with 'SpacesInParens'
  clang-format: avoid braces on simple single-statement bodies
  clang-format: formalize some of the spacing rules
  ci: run style check on GitHub and GitLab
  check-whitespace: detect if no base_commit is provided

 .clang-format                     | 42 +++++++++++++++++++++++++++----
 .github/workflows/check-style.yml | 29 +++++++++++++++++++++
 .gitlab-ci.yml                    | 24 +++++++++++++++++-
 ci/check-whitespace.sh            | 10 ++++++--
 ci/install-dependencies.sh        |  2 +-
 ci/run-style-check.sh             |  8 ++++++
 6 files changed, 106 insertions(+), 9 deletions(-)
 create mode 100644 .github/workflows/check-style.yml
 create mode 100755 ci/run-style-check.sh

Range-diff against v1:
1:  6cf91ffc86 = 1:  6cf91ffc86 clang-format: indent preprocessor directives after hash
2:  beb002885f = 2:  beb002885f clang-format: avoid spacing around bitfield colon
3:  3031be43e7 = 3:  3031be43e7 clang-format: ensure files end with newlines
4:  bc1550e300 = 4:  bc1550e300 clang-format: replace deprecated option with 'SpacesInParens'
5:  cb6097aa22 ! 5:  840318a4c9 clang-format: avoid braces on simple single-statement bodies
    @@ Commit message
         Set the 'RemoveBracesLLVM' to 'true' which ensures that we avoid curly
         braces for single-statement bodies in conditional blocks.
     
    +    This option does come with two warnings:
    +
    +        This option will be renamed and expanded to support other styles.
    +
    +    and
    +
    +        Setting this option to true could lead to incorrect code formatting
    +        due to clang-format’s lack of complete semantic information. As
    +        such, extra care should be taken to review code changes made by
    +        this option.
    +
    +    The latter seems to be of concern. But since we only use clang-format to
    +    verify the format and not to apply formatting, we should be okay here.
    +
         Signed-off-by: Karthik Nayak <[e-mail]>
     
      ## .clang-format ##
    @@ .clang-format: IndentWrappedFunctionNames: false
      PointerAlignment: Right
      
     +# Remove optional braces of control statements (if, else, for, and while)
    -+# according to the LLVM coding style
    -+# This avoids braces on simple single-statement bodies of statements.
    ++# according to the LLVM coding style. This avoids braces on simple
    ++# single-statement bodies of statements but keeps braces if one side of
    ++# if/else if/.../else cascade has multi-statement body.
     +RemoveBracesLLVM: true
     +
      # Don't insert a space after a cast
6:  c4b6e1e82f ! 6:  0ecfd8d24b clang-format: formalize some of the spacing rules
    @@ Commit message
         clang-format: formalize some of the spacing rules
     
         There are some spacing rules that we follow in the project and it makes
    -    sen to formalize them:
    +    sense to formalize them:
         * Ensure there is no space inserted after the logical not '!' operator.
    -    * Ensure there is no space before the case statement's color.
    +    * Ensure there is no space before the case statement's colon.
         * Ensure there is no space before the first bracket '[' of an array.
         * Ensure there is no space in empty blocks.
     
7:  37f1e5c702 ! 7:  11a9ac4935 ci: run style check on GitHub and GitLab
    @@ Commit message
         this job to fail, so we can validate if this is useful and eventually
         enforce it.
     
    +    For GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by
    +    default to obtain the base SHA of the merged pipeline (which is only
    +    available for merged pipelines [1]). Otherwise we use the
    +    'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable.
    +
    +    [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines
    +
         Signed-off-by: Karthik Nayak <[e-mail]>
     
      ## .github/workflows/check-style.yml (new) ##
    @@ .gitlab-ci.yml: check-whitespace:
     +  before_script:
     +    - ./ci/install-dependencies.sh
     +  script:
    -+    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++    - |
    ++      if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then
    ++        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
    ++      else
    ++        ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    ++      fi
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
     +
    @@ ci/install-dependencies.sh: ubuntu-*)
     
      ## ci/run-style-check.sh (new) ##
     @@
    -+#!/usr/bin/env sh
    ++#!/bin/sh
     +#
     +# Perform style check
     +#
8:  c6d07402af < -:  ---------- check-whitespace: detect if no base_commit is provided
-:  ---------- > 8:  caccbf8264 check-whitespace: detect if no base_commit is provided

Comments

Junio C Hamano July 11, 2024, 6:11 p.m. UTC | #1

Karthik Nayak <[e-mail]> writes:

> This is v2 of my series to add clang-format to the CI.

Aside from a few comments, the series looked good.

Will queue.  Thanks.