Re: [PATCH 2/2] gitlab-ci: add whitespace error check
On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:
Show 12 quoted lines
> 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?
Show 26 quoted lines
> 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.
Show 15 quoted lines
> + 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
Show 11 quoted lines
> +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
>
> 
Justin Tobler
