Re: [PATCH 2/2] gitlab-ci: add whitespace error check

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:
> To check for whitespace errors introduced by a set of changes, there is
> the `.github/workflows/check-whitespace.yml` GitHub action. This script
> executes `git log --check` over a range containing the new commits and
> parses the output to generate a markdown formatted artifact that
> summarizes detected errors with GitHub links to the affected commits and
> blobs.
> 
> Since this script is rather specific to GitHub actions, a more general
> and simple `ci/check-whitespace.sh` is added instead that functions the
> same, but does not generate the markdown file for the action summary.
> From this, a new GitLab CI job is added to support the whitespace error
> check.

I still wonder whether we can unify these. Yes, the GitHub thing is
quite specific. But ultimately, what it does is to generate a proper
summary of where exactly the whitespaces issues are, which is something
that your version doesn't do. It's useful though for consumers of a
failed CI job to know exactly which commit has the issue.

So can't we pull out the logic into a script, refactor it such that it
knows to print both GitHub- and GitLab-style URLs, and then also print
the summary in GitLab CI?

> Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only
> available in GitLab merge request pipelines and therefore the CI job is
> configured to only run as part of those pipelines.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  .gitlab-ci.yml         |  9 +++++++++
>  ci/check-whitespace.sh | 16 ++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100755 ci/check-whitespace.sh
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index c0fa2fe90b..31cf420a11 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -102,3 +102,12 @@ static-analysis:
>    script:
>      - ./ci/run-static-analysis.sh
>      - ./ci/check-directional-formatting.bash
> +
> +check-whitespace:
> +  image: ubuntu:latest
> +  before_script:
> +    - ./ci/install-docker-dependencies.sh
> +  script:
> +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA

Let's quote this variable.

> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> new file mode 100755
> index 0000000000..1cad2d7374
> --- /dev/null
> +++ b/ci/check-whitespace.sh
> @@ -0,0 +1,16 @@
> +#! /bin/sh
> +#
> +# Check that commits after a specified point do not contain new or modified
> +# lines with whitespace errors.
> +#
> +
> +baseSha=${1}

Two nits: first, I wouldn't call it "sha" because it really is a commit
ID that may or may not be SHA if we were ever to grow a hash function
that is not SHA. So "baseCommit" would be more descriptive.

Second, our shell variables are typically written in all-uppercase. So
that'd make it "BASE_COMMIT".

> +git log --check --pretty=format:"---% h% s" ${baseSha}..

Nit: there's no need for the curly braces here. Also, let's quote the
value.

Patrick

> +if test $? -ne 0
> +then
> +	echo "A whitespace issue was found in one or more of the commits."
> +	echo "Run the following command to resolve whitespace issues:"
> +	echo "\tgit rebase --whitespace=fix ${baseSha}"
> +	exit 2
> +fi
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]