Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> We don't run style checks on our CI, even though we have a
> '.clang-format' setup in the repository. Let's add one, the job will
> validate only against the new commits added and will only run on merge
> requests.

I hope "against new commits" means what I think it does ;-) I am
worried about the case where a new commit touches a file that has
existing style violations but the commit does not make the situation
any worse at all.

OK, "git clang-format" is used to for this thing and it is designed
to address only lines that differ, so hopefully that would be good.

> Since we're introducing it for the first time, let's allow
> this job to fail, so we can validate if this is useful and eventually
> enforce it.

Very good idea.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++
>  .gitlab-ci.yml                    | 12 ++++++++++++
>  ci/install-dependencies.sh        |  2 +-
>  ci/run-style-check.sh             |  8 ++++++++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 .github/workflows/check-style.yml
>  create mode 100755 ci/run-style-check.sh
>
> diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml
> new file mode 100644
> index 0000000000..27276dfe5e
> --- /dev/null
> +++ b/.github/workflows/check-style.yml
> @@ -0,0 +1,29 @@
> +name: check-style
> +
> +# Get the repository with all commits to ensure that we can analyze
> +# all of the commits contributed via the Pull Request.
> +
> +on:
> +  pull_request:
> +    types: [opened, synchronize]
> +
> +# Avoid unnecessary builds. Unlike the main CI jobs, these are not
> +# ci-configurable (but could be).
> +concurrency:
> +  group: ${{ github.workflow }}-${{ github.ref }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  check-style:
> +    runs-on: ubuntu-latest
> +    steps:
> +    - uses: actions/checkout@v4
> +      with:
> +        fetch-depth: 0
> +
> +    - name: git clang-format
> +      continue-on-error: true
> +      id: check_out
> +      run: |
> +        ./ci/run-style-check.sh \
> +          "${{github.event.pull_request.base.sha}}"
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 37b991e080..65fd261e5e 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -123,6 +123,18 @@ check-whitespace:
>    rules:
>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>  
> +check-style:
> +  image: ubuntu:latest
> +  allow_failure: true
> +  variables:
> +    CC: clang
> +  before_script:
> +    - ./ci/install-dependencies.sh
> +  script:
> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> +
>  documentation:
>    image: ubuntu:latest
>    variables:
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 6ec0f85972..46fe12a690 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -43,7 +43,7 @@ ubuntu-*)
>  		make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \
>  		tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \
>  		libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
> -		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE
> +		${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format
>  
>  	mkdir --parents "$CUSTOM_PATH"
>  	wget --quiet --directory-prefix="$CUSTOM_PATH" \
> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
> new file mode 100755
> index 0000000000..9d4c4089c1
> --- /dev/null
> +++ b/ci/run-style-check.sh
> @@ -0,0 +1,8 @@
> +#!/usr/bin/env sh

Under ci/ hierarchy we are very inconsistent.  Most use the bog
standard "#!/bin/sh" (which is my preference by the way), but
see what we have here right now:

        $ git grep -e '^#![a-z/]*/bin/[a-z]*sh' -e '^#![a-z/]*bin/env ' ci |
          sort -t: -k2
        ci/check-directional-formatting.bash:#!/bin/bash
        ci/install-dependencies.sh:#!/bin/sh
        ci/make-test-artifacts.sh:#!/bin/sh
        ci/mount-fileshare.sh:#!/bin/sh
        ci/print-test-failures.sh:#!/bin/sh
        ci/run-build-and-minimal-fuzzers.sh:#!/bin/sh
        ci/run-build-and-tests.sh:#!/bin/sh
        ci/run-docker-build.sh:#!/bin/sh
        ci/run-docker.sh:#!/bin/sh
        ci/run-static-analysis.sh:#!/bin/sh
        ci/run-test-slice.sh:#!/bin/sh
        ci/util/extract-trash-dirs.sh:#!/bin/sh
        ci/check-whitespace.sh:#!/usr/bin/env bash
        ci/test-documentation.sh:#!/usr/bin/env bash

Unless you have a strong reason to suspect that in your CI
environment /bin/sh is an unusuably broken shell, please do not
spread the inconsistencies.

I think the consensus from the last discussion we had was to allow
scripts that rely on bash-isms to say "#!/usr/bin/env bash" because
we know /bin/sh can legitimately be not bash and we assume bash may
not be installed as /bin/bash.  As all of them would run in the CI
environment that we have some control over what required packages
are installed at what path, it is OK to assume that "bash" would be
found on the $PATH by using /usr/bin/env (but it does assume
everybody installs "env" there, not /bin/env or /usr/local/bin/env,
with a bit of chicken-and-egg issue).

> +#
> +# Perform style check
> +#
> +
> +baseCommit=$1
> +
> +git clang-format --style file --diff --extensions c,h "$baseCommit"

OK, "git clang-format" compares the working tree with the named
commit, so if we have the tip of the topic branch proposed to be
merged checked out and compare it with the base commit of the topic,
that would give us exactly what we want.  Nice.

Thanks.