On 24/04/30 07:04AM, Patrick Steinhardt wrote: > 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. Just to clarify, this new CI job still prints the output of `git log --check` which details the exact commit and file with line number of the whitespace error. The difference is that it does not write an additional markdown file with links to the commit and blob. Here is a failed execution of the GitLab whitespace check job: https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289 > > 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? We can do this, but for GitLab CI there probably isn't a point to generating a summary file since there is nothing that would pick it up and display it. Having links though directly in the job output would be nice. I'll give it another go. > > > 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@xxxxxxxxx> > > --- > > .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. Thanks for the review Patrick! I'll address these in the next version :) -Justin
