[PATCH 0/2] Add GitLab CI to check for whitespace errors

[PATCH 0/2] Add GitLab CI to check for whitespace errors

[Justin Tobler]

Hello everyone,

The currently configured GitLab pipelines do not include a CI job to
check for whitespace errors. Instead of generalizing the existing GitHub
action in `.github/workflows/check-whitespace.yml`, this change opts to
create a separate generalized `ci/whitespace-check.sh` script to build
from. The existing action does extra work to create a GitHub specific
summary artifact that is markdown formatted with links to commits and
blobs. If this summary feature is not considered necessary, the GitHub
action could be reworked to use the general whitespace check.

He is a link to a successful execution of the CI job:
https://gitlab.com/gitlab-org/git/-/jobs/6744313860

I also added a small change to by default collapse grouped sections of
CI output to reduce noise.

-Justin

Justin Tobler (2):
  ci: pre-collapse GitLab CI sections
  gitlab-ci: add whitespace error check

 .gitlab-ci.yml         |  9 +++++++++
 ci/check-whitespace.sh | 16 ++++++++++++++++
 ci/lib.sh              |  2 +-
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100755 ci/check-whitespace.sh


base-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8
-- 
2.44.0

[PATCH 1/2] ci: pre-collapse GitLab CI sections

[Justin Tobler]

Sections of CI output defined by `begin_group()` and `end_group()` are
expanded in GitLab pipelines by default. This can make CI job output
rather noisy and harder to navigate. Update the behavior for GitLab
pipelines to now collapse sections by default.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..02e5e058dd 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI"
 then
 	begin_group () {
 		need_to_end_group=t
-		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n"
 		trap "end_group '$1'" EXIT
 		set -x
 	}
-- 
2.44.0

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

[Justin Tobler]

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.

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
+  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}
+
+git log --check --pretty=format:"---% h% s" ${baseSha}..
+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

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

[James Liu]

Thanks for putting this together, Justin!

> +check-whitespace:
> +  image: ubuntu:latest

I wonder if we should pin to `ubuntu:22.04` and only update this for
each LTS release. It seems like we've done this for the
`static-analysis` job above.

> +  before_script:
> +    - ./ci/install-docker-dependencies.sh
> +  script:
> +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA
> +  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

nit: there seems to be extra whitespace after the shebang :D

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

[Justin Tobler]

On 24/04/30 11:59AM, James Liu wrote:
> Thanks for putting this together, Justin!
> 
> > +check-whitespace:
> > +  image: ubuntu:latest
> 
> I wonder if we should pin to `ubuntu:22.04` and only update this for
> each LTS release. It seems like we've done this for the
> `static-analysis` job above.

I'm not sure if it particually matters. I know Patrick recently
submitted the following also using `ubuntu:latest`:
https://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r

I can change it though if there is strong opinion one way or the other.

> 
> > +  before_script:
> > +    - ./ci/install-docker-dependencies.sh
> > +  script:
> > +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA
> > +  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
> 
> nit: there seems to be extra whitespace after the shebang :D
> 
Whoops... I'll address this in a subsequent version.

-Justin

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

[Patrick Steinhardt]

[-- 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 --]

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

[Patrick Steinhardt]

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

On Mon, Apr 29, 2024 at 10:01:15PM -0500, Justin Tobler wrote:
> On 24/04/30 11:59AM, James Liu wrote:
> > Thanks for putting this together, Justin!
> > 
> > > +check-whitespace:
> > > +  image: ubuntu:latest
> > 
> > I wonder if we should pin to `ubuntu:22.04` and only update this for
> > each LTS release. It seems like we've done this for the
> > `static-analysis` job above.
> 
> I'm not sure if it particually matters. I know Patrick recently
> submitted the following also using `ubuntu:latest`:
> https://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r
> 
> I can change it though if there is strong opinion one way or the other.

I'd say it probably doesn't matter much for this case. But using
`ubuntu:latest` means that there's less maintenance going forward
because we don't ever have to update it.

Patrick

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

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

[Taylor Blau]

On Tue, Apr 30, 2024 at 07:04:46AM +0200, 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.
>
> 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?

This was my feeling as well when I read the cover letter. I don't think
we should either (a) have to remove functionality from the GitHub
specific version of this job, or (b) have to maintain two different but
highly similar implementations of the same job.

It seems more reasonable that we'd do some third option, which is what
Patrick suggests.

Thanks,
Taylor

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

[Justin Tobler]

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@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.

Thanks for the review Patrick! I'll address these in the next version :)

-Justin

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

[Patrick Steinhardt]

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

On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:
> 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

Okay, fair enough. I'm still of the opinion that the infra here should
be shared.

> > 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.

Well, we could print the output to the console so that a user can see it
when they open the failed job. The nice formatting may be kind of moot,
but on the other hand it doesn't hurt, either. I guess most people are
used to reading plain markdown-style docs anyway.

Patrick

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

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

[Justin Tobler]

On 24/04/30 04:05PM, Patrick Steinhardt wrote:
> On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:
> > 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
> 
> Okay, fair enough. I'm still of the opinion that the infra here should
> be shared.
> 
> > > 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.
> 
> Well, we could print the output to the console so that a user can see it
> when they open the failed job. The nice formatting may be kind of moot,
> but on the other hand it doesn't hurt, either. I guess most people are
> used to reading plain markdown-style docs anyway.

I'm thinking we can generalize the summary writing in some manner. When
run as a GitHub action, the summary can be markdown formatted and
written to `$GITHUB_STEP_SUMMARY` with no output to the console as done
today. When run as GitLab CI, the summary can be written directly to
console with links. All other runs just output normally to console.

-Justin

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

[Patrick Steinhardt]

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

On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote:
> On 24/04/30 04:05PM, Patrick Steinhardt wrote:
> > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:
> > > 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
> > 
> > Okay, fair enough. I'm still of the opinion that the infra here should
> > be shared.
> > 
> > > > 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.
> > 
> > Well, we could print the output to the console so that a user can see it
> > when they open the failed job. The nice formatting may be kind of moot,
> > but on the other hand it doesn't hurt, either. I guess most people are
> > used to reading plain markdown-style docs anyway.
> 
> I'm thinking we can generalize the summary writing in some manner. When
> run as a GitHub action, the summary can be markdown formatted and
> written to `$GITHUB_STEP_SUMMARY` with no output to the console as done
> today. When run as GitLab CI, the summary can be written directly to
> console with links. All other runs just output normally to console.

The script can probably be generalized to take a file name as argument.
For GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we
pass in a temporary filename that we than simply cat(1) to the console.
That'd allow us to move the CI-specific bits into the respective CIs
whereas the script itself remains generic.

Patrick

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

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

[Justin Tobler]

On 24/04/30 04:45PM, Patrick Steinhardt wrote:
> On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote:
> > On 24/04/30 04:05PM, Patrick Steinhardt wrote:
> > > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:
> > > > 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
> > > 
> > > Okay, fair enough. I'm still of the opinion that the infra here should
> > > be shared.
> > > 
> > > > > 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.
> > > 
> > > Well, we could print the output to the console so that a user can see it
> > > when they open the failed job. The nice formatting may be kind of moot,
> > > but on the other hand it doesn't hurt, either. I guess most people are
> > > used to reading plain markdown-style docs anyway.
> > 
> > I'm thinking we can generalize the summary writing in some manner. When
> > run as a GitHub action, the summary can be markdown formatted and
> > written to `$GITHUB_STEP_SUMMARY` with no output to the console as done
> > today. When run as GitLab CI, the summary can be written directly to
> > console with links. All other runs just output normally to console.
> 
> The script can probably be generalized to take a file name as argument.
> For GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we
> pass in a temporary filename that we than simply cat(1) to the console.
> That'd allow us to move the CI-specific bits into the respective CIs
> whereas the script itself remains generic.

This sounds good, I was also considering this. We will still need
CI-specific bits in the script in order to generate correctly formatted
links. That is, unless we also want to pass these as arguments, but that
might start to get a little messy.

-Justin

[PATCH v2 0/5] Add GitLab CI to check for whitespace errors

[Justin Tobler]

Hello again,

This is the second version of my patch series to add a GitLab CI job to
check for whitespace errors. The main differnece with this version is
that it first generalizes the existing GitHub whitespace check CI job
allowing the GitLab one to reuse it.

To validate that these changes do not break the existing GitHub CI, here
are some links to a successful run and a failed run:

- https://github.com/gitgitgadget/git/actions/runs/8929081916
- https://github.com/gitgitgadget/git/actions/runs/8928887225

To validate that these changes also work on GitLab CI, here some
additional links to a succdessful run and a failed run:

- https://gitlab.com/gitlab-org/git/-/jobs/6768304381
- https://gitlab.com/gitlab-org/git/-/jobs/6768282645

Thanks,
-Justin

Justin Tobler (5):
  ci: pre-collapse GitLab CI sections
  github-ci: fix link to whitespace error
  ci: separate whitespace check script
  ci: make the whitespace report optional
  gitlab-ci: add whitespace error check

 .github/workflows/check-whitespace.yml | 67 ++----------------
 .gitlab-ci.yml                         |  9 +++
 ci/check-whitespace.sh                 | 95 ++++++++++++++++++++++++++
 ci/lib.sh                              |  2 +-
 4 files changed, 109 insertions(+), 64 deletions(-)
 create mode 100755 ci/check-whitespace.sh

Range-diff against v1:
1:  924d3eb23c = 1:  924d3eb23c ci: pre-collapse GitLab CI sections
-:  ---------- > 2:  c8d8b444dc github-ci: fix link to whitespace error
-:  ---------- > 3:  6b44b21dda ci: separate whitespace check script
-:  ---------- > 4:  87dfd1d5a9 ci: make the whitespace report optional
2:  624e68a8d2 ! 5:  175b300e91 gitlab-ci: add whitespace error check
    @@ Metadata
      ## Commit message ##
         gitlab-ci: add whitespace error check
     
    -    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.
    +    GitLab CI does not have a job to check for whitespace errors introduced
    +    by a set of changes. Reuse the existing generic `whitespace-check.sh` to
    +    create the job for GitLab pipelines.
     
         Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only
         available in GitLab merge request pipelines and therefore the CI job is
    @@ .gitlab-ci.yml: static-analysis:
     +  before_script:
     +    - ./ci/install-docker-dependencies.sh
     +  script:
    -+    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA
    ++    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +  rules:
     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
    -
    - ## ci/check-whitespace.sh (new) ##
    -@@
    -+#! /bin/sh
    -+#
    -+# Check that commits after a specified point do not contain new or modified
    -+# lines with whitespace errors.
    -+#
    -+
    -+baseSha=${1}
    -+
    -+git log --check --pretty=format:"---% h% s" ${baseSha}..
    -+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.45.0

[PATCH v2 1/5] ci: pre-collapse GitLab CI sections

[Justin Tobler]

Sections of CI output defined by `begin_group()` and `end_group()` are
expanded in GitLab pipelines by default. This can make CI job output
rather noisy and harder to navigate. Update the behavior for GitLab
pipelines to now collapse sections by default.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..02e5e058dd 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI"
 then
 	begin_group () {
 		need_to_end_group=t
-		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n"
 		trap "end_group '$1'" EXIT
 		set -x
 	}
-- 
2.45.0

[PATCH v2 2/5] github-ci: fix link to whitespace error

[Justin Tobler]

When the `check-whitespace` CI job detects whitespace errors, a
formatted summary of the issue is generated. This summary contains links
to the commits and blobs responsible for the whitespace errors. The
generated links for blobs do not work and result in a 404.

Instead of using the reference name in the link, use the commit ID
directly. This fixes the broken link and also helps enable future
generalization of the script for other CI providers by removing one of
the GitHub specific CI variables used.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 .github/workflows/check-whitespace.yml | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index a241a63428..a3a6913ecc 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -31,14 +31,15 @@ jobs:
         commit=
         commitText=
         commitTextmd=
-        goodparent=
+        goodParent=
         while read dash sha etc
         do
           case "${dash}" in
-          "---")
-            if test -z "${commit}"
+          "---") # Line contains commit information.
+            if test -z "${goodParent}"
             then
-              goodparent=${sha}
+              # Assume the commit has no whitespace errors until detected otherwise.
+              goodParent=${sha}
             fi
             commit="${sha}"
             commitText="${sha} ${etc}"
@@ -46,18 +47,18 @@ jobs:
             ;;
           "")
             ;;
-          *)
-            if test -n "${commit}"
+          *) # Line contains whitespace error information for current commit.
+            if test -n "${goodParent}"
             then
               problems+=("1) --- ${commitTextmd}")
               echo ""
               echo "--- ${commitText}"
-              commit=
+              goodParent=
             fi
             case "${dash}" in
             *:[1-9]*:) # contains file and line number information
               dashend=${dash#*:}
-              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${{github.event.pull_request.head.ref}}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
+              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
               ;;
             *)
               problems+=("\`${dash} ${sha} ${etc}\`")
@@ -70,15 +71,15 @@ jobs:
 
         if test ${#problems[*]} -gt 0
         then
-          if test -z "${commit}"
+          if test -z "${goodParent}"
           then
-            goodparent=${baseSha: 0:7}
+            goodParent=${baseSha: 0:7}
           fi
           echo "🛑 Please review the Summary output for further information."
           echo "### :x: A whitespace issue was found in one or more of the commits." >$GITHUB_STEP_SUMMARY
           echo "" >>$GITHUB_STEP_SUMMARY
           echo "Run these commands to correct the problem:" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git rebase --whitespace=fix ${goodparent}\`" >>$GITHUB_STEP_SUMMARY
+          echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>$GITHUB_STEP_SUMMARY
           echo "1. \`git push --force\`" >>$GITHUB_STEP_SUMMARY
           echo " " >>$GITHUB_STEP_SUMMARY
           echo "Errors:" >>$GITHUB_STEP_SUMMARY
-- 
2.45.0

[PATCH v2 3/5] ci: separate whitespace check script

[Justin Tobler]

The `check-whitespace` CI job is only available as a GitHub action. To
help enable this job with other CI providers, first separate the logic
performing the whitespace check into its own script. In subsequent
commits, this script is further generalized allowing its reuse.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 .github/workflows/check-whitespace.yml | 68 ++---------------------
 ci/check-whitespace.sh                 | 74 ++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 64 deletions(-)
 create mode 100755 ci/check-whitespace.sh

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index a3a6913ecc..d0a78fc426 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -26,67 +26,7 @@ jobs:
     - name: git log --check
       id: check_out
       run: |
-        baseSha=${{github.event.pull_request.base.sha}}
-        problems=()
-        commit=
-        commitText=
-        commitTextmd=
-        goodParent=
-        while read dash sha etc
-        do
-          case "${dash}" in
-          "---") # Line contains commit information.
-            if test -z "${goodParent}"
-            then
-              # Assume the commit has no whitespace errors until detected otherwise.
-              goodParent=${sha}
-            fi
-            commit="${sha}"
-            commitText="${sha} ${etc}"
-            commitTextmd="[${sha}](https://github.com/${{ github.repository }}/commit/${sha}) ${etc}"
-            ;;
-          "")
-            ;;
-          *) # Line contains whitespace error information for current commit.
-            if test -n "${goodParent}"
-            then
-              problems+=("1) --- ${commitTextmd}")
-              echo ""
-              echo "--- ${commitText}"
-              goodParent=
-            fi
-            case "${dash}" in
-            *:[1-9]*:) # contains file and line number information
-              dashend=${dash#*:}
-              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
-              ;;
-            *)
-              problems+=("\`${dash} ${sha} ${etc}\`")
-              ;;
-            esac
-            echo "${dash} ${sha} ${etc}"
-            ;;
-          esac
-        done <<< $(git log --check --pretty=format:"---% h% s" ${baseSha}..)
-
-        if test ${#problems[*]} -gt 0
-        then
-          if test -z "${goodParent}"
-          then
-            goodParent=${baseSha: 0:7}
-          fi
-          echo "🛑 Please review the Summary output for further information."
-          echo "### :x: A whitespace issue was found in one or more of the commits." >$GITHUB_STEP_SUMMARY
-          echo "" >>$GITHUB_STEP_SUMMARY
-          echo "Run these commands to correct the problem:" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git push --force\`" >>$GITHUB_STEP_SUMMARY
-          echo " " >>$GITHUB_STEP_SUMMARY
-          echo "Errors:" >>$GITHUB_STEP_SUMMARY
-          for i in "${problems[@]}"
-          do
-            echo "${i}" >>$GITHUB_STEP_SUMMARY
-          done
-
-          exit 2
-        fi
+        ./ci/check-whitespace.sh \
+          "${{github.event.pull_request.base.sha}}" \
+          "$GITHUB_STEP_SUMMARY" \
+          "https://github.com/${{github.repository}}"
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
new file mode 100755
index 0000000000..f57d1ff5f0
--- /dev/null
+++ b/ci/check-whitespace.sh
@@ -0,0 +1,74 @@
+#!/bin/bash
+
+baseCommit=$1
+outputFile=$2
+url=$3
+
+problems=()
+commit=
+commitText=
+commitTextmd=
+goodParent=
+
+while read dash sha etc
+do
+	case "${dash}" in
+	"---") # Line contains commit information.
+		if test -z "${goodParent}"
+		then
+			# Assume the commit has no whitespace errors until detected otherwise.
+			goodParent=${sha}
+		fi
+
+		commit="${sha}"
+		commitText="${sha} ${etc}"
+		commitTextmd="[${sha}](${url}/commit/${sha}) ${etc}"
+		;;
+	"")
+		;;
+	*) # Line contains whitespace error information for current commit.
+		if test -n "${goodParent}"
+		then
+			problems+=("1) --- ${commitTextmd}")
+			echo ""
+			echo "--- ${commitText}"
+			goodParent=
+		fi
+
+		case "${dash}" in
+		*:[1-9]*:) # contains file and line number information
+			dashend=${dash#*:}
+			problems+=("[${dash}](${url}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
+			;;
+		*)
+			problems+=("\`${dash} ${sha} ${etc}\`")
+			;;
+		esac
+		echo "${dash} ${sha} ${etc}"
+		;;
+	esac
+done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
+
+if test ${#problems[*]} -gt 0
+then
+	if test -z "${goodParent}"
+	then
+		goodParent=${baseCommit: 0:7}
+	fi
+
+	echo "🛑 Please review the Summary output for further information."
+	echo "### :x: A whitespace issue was found in one or more of the commits." >"$outputFile"
+	echo "" >>"$outputFile"
+	echo "Run these commands to correct the problem:" >>"$outputFile"
+	echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>"$outputFile"
+	echo "1. \`git push --force\`" >>"$outputFile"
+	echo " " >>"$outputFile"
+	echo "Errors:" >>"$outputFile"
+
+	for i in "${problems[@]}"
+	do
+		echo "${i}" >>"$outputFile"
+	done
+
+	exit 2
+fi
-- 
2.45.0

[PATCH v2 4/5] ci: make the whitespace report optional

[Justin Tobler]

The `check-whitespace` CI job generates a formatted output file
containing whitespace error information. As not all CI providers support
rendering a formatted summary, make its generation optional.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index f57d1ff5f0..fabd6ecde5 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -1,9 +1,20 @@
 #!/bin/bash
+#
+# Check that commits after a specified point do not contain new or modified
+# lines with whitespace errors. An optional formatted summary can be generated
+# by providing an output file path and url as additional arguments.
+#
 
 baseCommit=$1
 outputFile=$2
 url=$3
 
+if test "$#" -eq 0 || test "$#" -gt 3
+then
+	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
+	exit 1
+fi
+
 problems=()
 commit=
 commitText=
@@ -56,19 +67,29 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "🛑 Please review the Summary output for further information."
-	echo "### :x: A whitespace issue was found in one or more of the commits." >"$outputFile"
-	echo "" >>"$outputFile"
-	echo "Run these commands to correct the problem:" >>"$outputFile"
-	echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>"$outputFile"
-	echo "1. \`git push --force\`" >>"$outputFile"
-	echo " " >>"$outputFile"
-	echo "Errors:" >>"$outputFile"
+	echo "A whitespace issue was found in onen of more of the commits."
+	echo "Run the following command to resolve whitespace issues:"
+	echo "git rebase --whitespace=fix ${goodParent}"
+
+	# If target output file is provided, write formatted ouput.
+	if test -n "$outputFile"
+	then
+		echo "🛑 Please review the Summary output for further information."
+		(
+			echo "### :x: A whitespace issue was found in one or more of the commits."
+			echo ""
+			echo "Run these commands to correct the problem:"
+			echo "1. \`git rebase --whitespace=fix ${goodParent}\`"
+			echo "1. \`git push --force\`"
+			echo ""
+			echo "Errors:"
 
-	for i in "${problems[@]}"
-	do
-		echo "${i}" >>"$outputFile"
-	done
+			for i in "${problems[@]}"
+			do
+				echo "${i}"
+			done
+		) >"$outputFile"
+	fi
 
 	exit 2
 fi
-- 
2.45.0

[PATCH v2 5/5] gitlab-ci: add whitespace error check

[Justin Tobler]

GitLab CI does not have a job to check for whitespace errors introduced
by a set of changes. Reuse the existing generic `whitespace-check.sh` to
create the job for GitLab pipelines.

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 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..6d046ce409 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"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
-- 
2.45.0

Re: [PATCH v2 0/5] Add GitLab CI to check for whitespace errors

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

> This is the second version of my patch series to add a GitLab CI job to
> check for whitespace errors. The main differnece with this version is
> that it first generalizes the existing GitHub whitespace check CI job
> allowing the GitLab one to reuse it.

Will queue.  Thanks.  The extraction and reuse of a common script is
excellent.

Re: [PATCH v2 3/5] ci: separate whitespace check script

[Patrick Steinhardt]

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

On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:
> The `check-whitespace` CI job is only available as a GitHub action. To
> help enable this job with other CI providers, first separate the logic
> performing the whitespace check into its own script. In subsequent
> commits, this script is further generalized allowing its reuse.
> 
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
[snip]
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> new file mode 100755
> index 0000000000..f57d1ff5f0
> --- /dev/null
> +++ b/ci/check-whitespace.sh
> @@ -0,0 +1,74 @@
> +#!/bin/bash

This needs to be either "/bin/sh" or "/usr/bin/env bash".

> +baseCommit=$1
> +outputFile=$2

I think the script would be more flexible if we just didn't have an
output file in the first place and handle redirection of the output at
the caller's side. That for example also allows you to easily append to
a potential output file.

Edit: I see you change this in the next patch, so this is okay.

> +url=$3

We should check that we got 3 arguments in the first place.

Edit: I see that you add those checks in the next commit, but it does
leave the reader wondering at this point. Maybe we should have a strict
check here and then loosen it in the next commit where you make it
optional.

Patrick

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

Re: [PATCH v2 4/5] ci: make the whitespace report optional

[Patrick Steinhardt]

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

On Thu, May 02, 2024 at 02:38:38PM -0500, Justin Tobler wrote:
> The `check-whitespace` CI job generates a formatted output file
> containing whitespace error information. As not all CI providers support
> rendering a formatted summary, make its generation optional.
> 
> Signed-off-by: Justin Tobler <jltobler@gmail.com>
> ---
>  ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> index f57d1ff5f0..fabd6ecde5 100755
> --- a/ci/check-whitespace.sh
> +++ b/ci/check-whitespace.sh
> @@ -1,9 +1,20 @@
>  #!/bin/bash
> +#
> +# Check that commits after a specified point do not contain new or modified
> +# lines with whitespace errors. An optional formatted summary can be generated
> +# by providing an output file path and url as additional arguments.
> +#
>  
>  baseCommit=$1
>  outputFile=$2
>  url=$3
>  
> +if test "$#" -eq 0 || test "$#" -gt 3

That check is wrong, isn't it? Based on the usage below you either
accept exactly 1 or exactly 3 arguments. But the condition here also
accepts 2 arguments just fine. The following may be a bit easier to
follow as it is more explicit:

    if test "$#" -ne 2 && test "$#" -ne 3
    then
        ...
    fi

> +then
> +	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
> +	exit 1
> +fi

Ah, you make the output file optional here. Fair enough, then you can
scratch that comment from my preceding mail as it did serve a purpose.

Patrick

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

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

[Patrick Steinhardt]

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

On Thu, May 02, 2024 at 02:38:39PM -0500, Justin Tobler wrote:
> GitLab CI does not have a job to check for whitespace errors introduced
> by a set of changes. Reuse the existing generic `whitespace-check.sh` to
> create the job for GitLab pipelines.
> 
> 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 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index c0fa2fe90b..6d046ce409 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"
> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'

Nice :)

Patrick

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

Re: [PATCH v2 3/5] ci: separate whitespace check script

[Justin Tobler]

On 24/05/03 08:56AM, Patrick Steinhardt wrote:
> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:
> > The `check-whitespace` CI job is only available as a GitHub action. To
> > help enable this job with other CI providers, first separate the logic
> > performing the whitespace check into its own script. In subsequent
> > commits, this script is further generalized allowing its reuse.
> > 
> > Helped-by: Patrick Steinhardt <ps@pks.im>
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> [snip]
> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> > new file mode 100755
> > index 0000000000..f57d1ff5f0
> > --- /dev/null
> > +++ b/ci/check-whitespace.sh
> > @@ -0,0 +1,74 @@
> > +#!/bin/bash
> 
> This needs to be either "/bin/sh" or "/usr/bin/env bash".

Since the script is using some shell specific features, I'll update this
to "/usr/bin/env bash" in the next version.

> 
> > +baseCommit=$1
> > +outputFile=$2
> 
> I think the script would be more flexible if we just didn't have an
> output file in the first place and handle redirection of the output at
> the caller's side. That for example also allows you to easily append to
> a potential output file.
> 
> Edit: I see you change this in the next patch, so this is okay.
> 
> > +url=$3
> 
> We should check that we got 3 arguments in the first place.
> 
> Edit: I see that you add those checks in the next commit, but it does
> leave the reader wondering at this point. Maybe we should have a strict
> check here and then loosen it in the next commit where you make it
> optional.

For this patch specifically, I was trying to really only factor out the
whitespace check into its own script and keep changes outside of that to
a minimum. The next patch focuses on all the actual script changes and I
was hoping it might be easier to review that way. :)

-Justin

Re: [PATCH v2 4/5] ci: make the whitespace report optional

[Justin Tobler]

On 24/05/03 08:56AM, Patrick Steinhardt wrote:
> On Thu, May 02, 2024 at 02:38:38PM -0500, Justin Tobler wrote:
> > The `check-whitespace` CI job generates a formatted output file
> > containing whitespace error information. As not all CI providers support
> > rendering a formatted summary, make its generation optional.
> > 
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> >  ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------
> >  1 file changed, 33 insertions(+), 12 deletions(-)
> > 
> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> > index f57d1ff5f0..fabd6ecde5 100755
> > --- a/ci/check-whitespace.sh
> > +++ b/ci/check-whitespace.sh
> > @@ -1,9 +1,20 @@
> >  #!/bin/bash
> > +#
> > +# Check that commits after a specified point do not contain new or modified
> > +# lines with whitespace errors. An optional formatted summary can be generated
> > +# by providing an output file path and url as additional arguments.
> > +#
> >  
> >  baseCommit=$1
> >  outputFile=$2
> >  url=$3
> >  
> > +if test "$#" -eq 0 || test "$#" -gt 3
> 
> That check is wrong, isn't it? Based on the usage below you either
> accept exactly 1 or exactly 3 arguments. But the condition here also
> accepts 2 arguments just fine. The following may be a bit easier to
> follow as it is more explicit:
> 
>     if test "$#" -ne 2 && test "$#" -ne 3
>     then
>         ...
>     fi

Ya, good point. We should only accept 1 or 3 arguments as valid options.
I'll update this. Thanks!

-Justin

> > +then
> > +	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
> > +	exit 1
> > +fi
> 
> Ah, you make the output file optional here. Fair enough, then you can
> scratch that comment from my preceding mail as it did serve a purpose.

Re: [PATCH v2 0/5] Add GitLab CI to check for whitespace errors

[Justin Tobler]

On 24/05/02 02:45PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > This is the second version of my patch series to add a GitLab CI job to
> > check for whitespace errors. The main differnece with this version is
> > that it first generalizes the existing GitHub whitespace check CI job
> > allowing the GitLab one to reuse it.
> 
> Will queue.  Thanks.  The extraction and reuse of a common script is
> excellent.

Thanks! I will send a V3 here shortly which makes some small suggested
edits.

-Justin

Re: [PATCH v2 3/5] ci: separate whitespace check script

[Junio C Hamano]

Justin Tobler <jltobler@gmail.com> writes:

> On 24/05/03 08:56AM, Patrick Steinhardt wrote:
>> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:
>> > The `check-whitespace` CI job is only available as a GitHub action. To
>> > help enable this job with other CI providers, first separate the logic
>> > performing the whitespace check into its own script. In subsequent
>> > commits, this script is further generalized allowing its reuse.
>> > 
>> > Helped-by: Patrick Steinhardt <ps@pks.im>
>> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
>> > ---
>> [snip]
>> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
>> > new file mode 100755
>> > index 0000000000..f57d1ff5f0
>> > --- /dev/null
>> > +++ b/ci/check-whitespace.sh
>> > @@ -0,0 +1,74 @@
>> > +#!/bin/bash
>> 
>> This needs to be either "/bin/sh" or "/usr/bin/env bash".
>
> Since the script is using some shell specific features, I'll update this
> to "/usr/bin/env bash" in the next version.

This is a question to Patrick, but what makes it bad to assume
"bash" is in "/bin" when it is OK to assume that "env" is always in
"/usr/bin"?

All other comments by Patrick I found very sensible.

Thanks, both of you.

[PATCH v3 0/5] Add GitLab CI to check for whitespace errors

[Justin Tobler]

Hello again,

This is the third version of my patch series to add a GitLab CI job to
check for whitespace errors.

Changes since V2:

- In 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next,
  2024-05-01), `install-docker-dependencies.sh` script was merged into 
  `install-dependencies.sh` and removed. Thus this patch series now 
  depends on that change and is updated to use the other script.
- Changed the shebang to "#!/usr/bin/env bash".
- Made the argument count validation more strict.
- Fixed a typo.

GitHub CI:

- https://github.com/gitgitgadget/git/actions/runs/8941666020?pr=1727
- https://github.com/gitgitgadget/git/actions/runs/8941721282?pr=1727

GitLab CI:

- https://gitlab.com/gitlab-org/git/-/jobs/6776032414
- https://gitlab.com/gitlab-org/git/-/jobs/6776038234

Thanks for the reviews,
-Justin

Justin Tobler (5):
  ci: pre-collapse GitLab CI sections
  github-ci: fix link to whitespace error
  ci: separate whitespace check script
  ci: make the whitespace report optional
  gitlab-ci: add whitespace error check

 .github/workflows/check-whitespace.yml | 67 ++----------------
 .gitlab-ci.yml                         |  9 +++
 ci/check-whitespace.sh                 | 95 ++++++++++++++++++++++++++
 ci/lib.sh                              |  2 +-
 4 files changed, 109 insertions(+), 64 deletions(-)
 create mode 100755 ci/check-whitespace.sh

Range-diff against v2:
1:  924d3eb23c = 1:  924d3eb23c ci: pre-collapse GitLab CI sections
2:  c8d8b444dc = 2:  c8d8b444dc github-ci: fix link to whitespace error
3:  6b44b21dda ! 3:  933e0c33f9 ci: separate whitespace check script
    @@ .github/workflows/check-whitespace.yml: jobs:
     
      ## ci/check-whitespace.sh (new) ##
     @@
    -+#!/bin/bash
    ++#!/usr/bin/env bash
     +
     +baseCommit=$1
     +outputFile=$2
4:  87dfd1d5a9 ! 4:  374735c60f ci: make the whitespace report optional
    @@ Commit message
     
      ## ci/check-whitespace.sh ##
     @@
    - #!/bin/bash
    + #!/usr/bin/env bash
     +#
     +# Check that commits after a specified point do not contain new or modified
     +# lines with whitespace errors. An optional formatted summary can be generated
    @@ ci/check-whitespace.sh
      outputFile=$2
      url=$3
      
    -+if test "$#" -eq 0 || test "$#" -gt 3
    ++if test "$#" -ne 1 && test "$#" -ne 3
     +then
     +	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
     +	exit 1
    @@ ci/check-whitespace.sh: then
     +	echo "Run the following command to resolve whitespace issues:"
     +	echo "git rebase --whitespace=fix ${goodParent}"
     +
    -+	# If target output file is provided, write formatted ouput.
    ++	# If target output file is provided, write formatted output.
     +	if test -n "$outputFile"
     +	then
     +		echo "🛑 Please review the Summary output for further information."
5:  175b300e91 ! 5:  a7deaeadc7 gitlab-ci: add whitespace error check
    @@ .gitlab-ci.yml: static-analysis:
     +check-whitespace:
     +  image: ubuntu:latest
     +  before_script:
    -+    - ./ci/install-docker-dependencies.sh
    ++    - ./ci/install-dependencies.sh
     +  script:
     +    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
     +  rules:
-- 
2.45.0

[PATCH v3 1/5] ci: pre-collapse GitLab CI sections

[Justin Tobler]

Sections of CI output defined by `begin_group()` and `end_group()` are
expanded in GitLab pipelines by default. This can make CI job output
rather noisy and harder to navigate. Update the behavior for GitLab
pipelines to now collapse sections by default.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 0a73fc7bd1..02e5e058dd 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI"
 then
 	begin_group () {
 		need_to_end_group=t
-		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n"
 		trap "end_group '$1'" EXIT
 		set -x
 	}
-- 
2.45.0

[PATCH v3 2/5] github-ci: fix link to whitespace error

[Justin Tobler]

When the `check-whitespace` CI job detects whitespace errors, a
formatted summary of the issue is generated. This summary contains links
to the commits and blobs responsible for the whitespace errors. The
generated links for blobs do not work and result in a 404.

Instead of using the reference name in the link, use the commit ID
directly. This fixes the broken link and also helps enable future
generalization of the script for other CI providers by removing one of
the GitHub specific CI variables used.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 .github/workflows/check-whitespace.yml | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index a241a63428..a3a6913ecc 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -31,14 +31,15 @@ jobs:
         commit=
         commitText=
         commitTextmd=
-        goodparent=
+        goodParent=
         while read dash sha etc
         do
           case "${dash}" in
-          "---")
-            if test -z "${commit}"
+          "---") # Line contains commit information.
+            if test -z "${goodParent}"
             then
-              goodparent=${sha}
+              # Assume the commit has no whitespace errors until detected otherwise.
+              goodParent=${sha}
             fi
             commit="${sha}"
             commitText="${sha} ${etc}"
@@ -46,18 +47,18 @@ jobs:
             ;;
           "")
             ;;
-          *)
-            if test -n "${commit}"
+          *) # Line contains whitespace error information for current commit.
+            if test -n "${goodParent}"
             then
               problems+=("1) --- ${commitTextmd}")
               echo ""
               echo "--- ${commitText}"
-              commit=
+              goodParent=
             fi
             case "${dash}" in
             *:[1-9]*:) # contains file and line number information
               dashend=${dash#*:}
-              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${{github.event.pull_request.head.ref}}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
+              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
               ;;
             *)
               problems+=("\`${dash} ${sha} ${etc}\`")
@@ -70,15 +71,15 @@ jobs:
 
         if test ${#problems[*]} -gt 0
         then
-          if test -z "${commit}"
+          if test -z "${goodParent}"
           then
-            goodparent=${baseSha: 0:7}
+            goodParent=${baseSha: 0:7}
           fi
           echo "🛑 Please review the Summary output for further information."
           echo "### :x: A whitespace issue was found in one or more of the commits." >$GITHUB_STEP_SUMMARY
           echo "" >>$GITHUB_STEP_SUMMARY
           echo "Run these commands to correct the problem:" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git rebase --whitespace=fix ${goodparent}\`" >>$GITHUB_STEP_SUMMARY
+          echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>$GITHUB_STEP_SUMMARY
           echo "1. \`git push --force\`" >>$GITHUB_STEP_SUMMARY
           echo " " >>$GITHUB_STEP_SUMMARY
           echo "Errors:" >>$GITHUB_STEP_SUMMARY
-- 
2.45.0

[PATCH v3 3/5] ci: separate whitespace check script

[Justin Tobler]

The `check-whitespace` CI job is only available as a GitHub action. To
help enable this job with other CI providers, first separate the logic
performing the whitespace check into its own script. In subsequent
commits, this script is further generalized allowing its reuse.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 .github/workflows/check-whitespace.yml | 68 ++---------------------
 ci/check-whitespace.sh                 | 74 ++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 64 deletions(-)
 create mode 100755 ci/check-whitespace.sh

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index a3a6913ecc..d0a78fc426 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -26,67 +26,7 @@ jobs:
     - name: git log --check
       id: check_out
       run: |
-        baseSha=${{github.event.pull_request.base.sha}}
-        problems=()
-        commit=
-        commitText=
-        commitTextmd=
-        goodParent=
-        while read dash sha etc
-        do
-          case "${dash}" in
-          "---") # Line contains commit information.
-            if test -z "${goodParent}"
-            then
-              # Assume the commit has no whitespace errors until detected otherwise.
-              goodParent=${sha}
-            fi
-            commit="${sha}"
-            commitText="${sha} ${etc}"
-            commitTextmd="[${sha}](https://github.com/${{ github.repository }}/commit/${sha}) ${etc}"
-            ;;
-          "")
-            ;;
-          *) # Line contains whitespace error information for current commit.
-            if test -n "${goodParent}"
-            then
-              problems+=("1) --- ${commitTextmd}")
-              echo ""
-              echo "--- ${commitText}"
-              goodParent=
-            fi
-            case "${dash}" in
-            *:[1-9]*:) # contains file and line number information
-              dashend=${dash#*:}
-              problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
-              ;;
-            *)
-              problems+=("\`${dash} ${sha} ${etc}\`")
-              ;;
-            esac
-            echo "${dash} ${sha} ${etc}"
-            ;;
-          esac
-        done <<< $(git log --check --pretty=format:"---% h% s" ${baseSha}..)
-
-        if test ${#problems[*]} -gt 0
-        then
-          if test -z "${goodParent}"
-          then
-            goodParent=${baseSha: 0:7}
-          fi
-          echo "🛑 Please review the Summary output for further information."
-          echo "### :x: A whitespace issue was found in one or more of the commits." >$GITHUB_STEP_SUMMARY
-          echo "" >>$GITHUB_STEP_SUMMARY
-          echo "Run these commands to correct the problem:" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>$GITHUB_STEP_SUMMARY
-          echo "1. \`git push --force\`" >>$GITHUB_STEP_SUMMARY
-          echo " " >>$GITHUB_STEP_SUMMARY
-          echo "Errors:" >>$GITHUB_STEP_SUMMARY
-          for i in "${problems[@]}"
-          do
-            echo "${i}" >>$GITHUB_STEP_SUMMARY
-          done
-
-          exit 2
-        fi
+        ./ci/check-whitespace.sh \
+          "${{github.event.pull_request.base.sha}}" \
+          "$GITHUB_STEP_SUMMARY" \
+          "https://github.com/${{github.repository}}"
diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
new file mode 100755
index 0000000000..9cc496da40
--- /dev/null
+++ b/ci/check-whitespace.sh
@@ -0,0 +1,74 @@
+#!/usr/bin/env bash
+
+baseCommit=$1
+outputFile=$2
+url=$3
+
+problems=()
+commit=
+commitText=
+commitTextmd=
+goodParent=
+
+while read dash sha etc
+do
+	case "${dash}" in
+	"---") # Line contains commit information.
+		if test -z "${goodParent}"
+		then
+			# Assume the commit has no whitespace errors until detected otherwise.
+			goodParent=${sha}
+		fi
+
+		commit="${sha}"
+		commitText="${sha} ${etc}"
+		commitTextmd="[${sha}](${url}/commit/${sha}) ${etc}"
+		;;
+	"")
+		;;
+	*) # Line contains whitespace error information for current commit.
+		if test -n "${goodParent}"
+		then
+			problems+=("1) --- ${commitTextmd}")
+			echo ""
+			echo "--- ${commitText}"
+			goodParent=
+		fi
+
+		case "${dash}" in
+		*:[1-9]*:) # contains file and line number information
+			dashend=${dash#*:}
+			problems+=("[${dash}](${url}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}")
+			;;
+		*)
+			problems+=("\`${dash} ${sha} ${etc}\`")
+			;;
+		esac
+		echo "${dash} ${sha} ${etc}"
+		;;
+	esac
+done <<< "$(git log --check --pretty=format:"---% h% s" "${baseCommit}"..)"
+
+if test ${#problems[*]} -gt 0
+then
+	if test -z "${goodParent}"
+	then
+		goodParent=${baseCommit: 0:7}
+	fi
+
+	echo "🛑 Please review the Summary output for further information."
+	echo "### :x: A whitespace issue was found in one or more of the commits." >"$outputFile"
+	echo "" >>"$outputFile"
+	echo "Run these commands to correct the problem:" >>"$outputFile"
+	echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>"$outputFile"
+	echo "1. \`git push --force\`" >>"$outputFile"
+	echo " " >>"$outputFile"
+	echo "Errors:" >>"$outputFile"
+
+	for i in "${problems[@]}"
+	do
+		echo "${i}" >>"$outputFile"
+	done
+
+	exit 2
+fi
-- 
2.45.0

[PATCH v3 4/5] ci: make the whitespace report optional

[Justin Tobler]

The `check-whitespace` CI job generates a formatted output file
containing whitespace error information. As not all CI providers support
rendering a formatted summary, make its generation optional.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
index 9cc496da40..db399097a5 100755
--- a/ci/check-whitespace.sh
+++ b/ci/check-whitespace.sh
@@ -1,9 +1,20 @@
 #!/usr/bin/env bash
+#
+# Check that commits after a specified point do not contain new or modified
+# lines with whitespace errors. An optional formatted summary can be generated
+# by providing an output file path and url as additional arguments.
+#
 
 baseCommit=$1
 outputFile=$2
 url=$3
 
+if test "$#" -ne 1 && test "$#" -ne 3
+then
+	echo "USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]"
+	exit 1
+fi
+
 problems=()
 commit=
 commitText=
@@ -56,19 +67,29 @@ then
 		goodParent=${baseCommit: 0:7}
 	fi
 
-	echo "🛑 Please review the Summary output for further information."
-	echo "### :x: A whitespace issue was found in one or more of the commits." >"$outputFile"
-	echo "" >>"$outputFile"
-	echo "Run these commands to correct the problem:" >>"$outputFile"
-	echo "1. \`git rebase --whitespace=fix ${goodParent}\`" >>"$outputFile"
-	echo "1. \`git push --force\`" >>"$outputFile"
-	echo " " >>"$outputFile"
-	echo "Errors:" >>"$outputFile"
+	echo "A whitespace issue was found in onen of more of the commits."
+	echo "Run the following command to resolve whitespace issues:"
+	echo "git rebase --whitespace=fix ${goodParent}"
+
+	# If target output file is provided, write formatted output.
+	if test -n "$outputFile"
+	then
+		echo "🛑 Please review the Summary output for further information."
+		(
+			echo "### :x: A whitespace issue was found in one or more of the commits."
+			echo ""
+			echo "Run these commands to correct the problem:"
+			echo "1. \`git rebase --whitespace=fix ${goodParent}\`"
+			echo "1. \`git push --force\`"
+			echo ""
+			echo "Errors:"
 
-	for i in "${problems[@]}"
-	do
-		echo "${i}" >>"$outputFile"
-	done
+			for i in "${problems[@]}"
+			do
+				echo "${i}"
+			done
+		) >"$outputFile"
+	fi
 
 	exit 2
 fi
-- 
2.45.0

[PATCH v3 5/5] gitlab-ci: add whitespace error check

[Justin Tobler]

GitLab CI does not have a job to check for whitespace errors introduced
by a set of changes. Reuse the existing generic `whitespace-check.sh` to
create the job for GitLab pipelines.

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 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index c0fa2fe90b..619bf729fa 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-dependencies.sh
+  script:
+    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
+  rules:
+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
-- 
2.45.0

Re: [PATCH v2 3/5] ci: separate whitespace check script

[Patrick Steinhardt]

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

On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > On 24/05/03 08:56AM, Patrick Steinhardt wrote:
> >> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:
> >> > The `check-whitespace` CI job is only available as a GitHub action. To
> >> > help enable this job with other CI providers, first separate the logic
> >> > performing the whitespace check into its own script. In subsequent
> >> > commits, this script is further generalized allowing its reuse.
> >> > 
> >> > Helped-by: Patrick Steinhardt <ps@pks.im>
> >> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> >> > ---
> >> [snip]
> >> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh
> >> > new file mode 100755
> >> > index 0000000000..f57d1ff5f0
> >> > --- /dev/null
> >> > +++ b/ci/check-whitespace.sh
> >> > @@ -0,0 +1,74 @@
> >> > +#!/bin/bash
> >> 
> >> This needs to be either "/bin/sh" or "/usr/bin/env bash".
> >
> > Since the script is using some shell specific features, I'll update this
> > to "/usr/bin/env bash" in the next version.
> 
> This is a question to Patrick, but what makes it bad to assume
> "bash" is in "/bin" when it is OK to assume that "env" is always in
> "/usr/bin"?

My own bias. I know of systems without "/bin/bash" (NixOS), but I don't
know of any without "/usr/bin/env". But you're right, "/usr/bin/env" is
not part of POSIX and thus not really portable, either.

Ultimately I don't think there is any way to write the shebang so that
it is fully POSIX compliant. So I'd rather go with the option which is
supported on more systems, which is to the best of my knowlede env(1).

Patrick

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

Re: [PATCH v2 3/5] ci: separate whitespace check script

[Chris Torek]

On Fri, May 3, 2024 at 11:27 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote:
> > This is a question to Patrick, but what makes it bad to assume
> > "bash" is in "/bin" when it is OK to assume that "env" is always in
> > "/usr/bin"?
>
> My own bias. I know of systems without "/bin/bash" (NixOS), but I don't
> know of any without "/usr/bin/env". But you're right, "/usr/bin/env" is
> not part of POSIX and thus not really portable, either.
>
> Ultimately I don't think there is any way to write the shebang so that
> it is fully POSIX compliant. So I'd rather go with the option which is
> supported on more systems, which is to the best of my knowlede env(1).

The various BSDs mostly stick bash in /usr/local/bin; some versions
of macOS did not have a /bin/bash either, as I recall, though my current
mac-laptop does.

In any case, #! /usr/bin/env <program> is pretty darn common; it's found
in a lot of Python scripts, for instance. It works well on old SunOS, on
the various BSDs, on macOS, and on Linux, provided of course that
the given <program> is installed at all.

The *most* portable method is generally to use only POSIX /bin/sh
constructs, of course. :-)

Chris

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

[Patrick Steinhardt]

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

On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote:
> GitLab CI does not have a job to check for whitespace errors introduced
> by a set of changes. Reuse the existing generic `whitespace-check.sh` to
> create the job for GitLab pipelines.
> 
> 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 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index c0fa2fe90b..619bf729fa 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-dependencies.sh

Do we actually need to install dependencies? I imagine all that's needed
would be Git.

Other than this question the patch series looks good to me, thanks!

Patrick

> +  script:
> +    - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> -- 
> 2.45.0
> 
> 

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

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

[Justin Tobler]

On 24/05/06 08:55AM, Patrick Steinhardt wrote:
> On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote:
> > GitLab CI does not have a job to check for whitespace errors introduced
> > by a set of changes. Reuse the existing generic `whitespace-check.sh` to
> > create the job for GitLab pipelines.
> > 
> > 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 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index c0fa2fe90b..619bf729fa 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-dependencies.sh
> 
> Do we actually need to install dependencies? I imagine all that's needed
> would be Git.

You are correct. Git is really the only dependency we need. If we want
to remove the need for the install dependencies script, we could fetch
Git ourselves during the pre-script. Another option could be to use a
different container image that has Git baked in.

-Justin

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +check-whitespace:
>> +  image: ubuntu:latest
>> +  before_script:
>> +    - ./ci/install-dependencies.sh
>
> Do we actually need to install dependencies? I imagine all that's needed
> would be Git.
>
> Other than this question the patch series looks good to me, thanks!

I am a bit puzzled.  Is the proposal to check our sources with a
pre-built Git (which by definition would be a bit older than what is
being tested)?

Not that I have serious trouble in that direction---I am just trying
to make sure what is being proposed.

Thanks.

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

[Justin Tobler]

On 24/05/06 10:17AM, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> +check-whitespace:
> >> +  image: ubuntu:latest
> >> +  before_script:
> >> +    - ./ci/install-dependencies.sh
> >
> > Do we actually need to install dependencies? I imagine all that's needed
> > would be Git.
> >
> > Other than this question the patch series looks good to me, thanks!
> 
> I am a bit puzzled.  Is the proposal to check our sources with a
> pre-built Git (which by definition would be a bit older than what is
> being tested)?

The GitLab `check-whitespace` CI job only needs Git to run and uses
`ci/install-dependencies.sh` to download a pre-built Git package via
`apt-get` since `ubuntu:latest` is the container image used. The 
`ci/install-dependencies.sh` script also fetches a bunch of other 
dependencies which are not needed. 

I think Patrick is proposing, to further simplify, we avoid using
`ci/install-dependencies.sh` and only fetch Git. Patrick please correct
me if I misunderstand :)

-Justin

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

[Patrick Steinhardt]

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

On Mon, May 06, 2024 at 02:21:52PM -0500, Justin Tobler wrote:
> On 24/05/06 10:17AM, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > >> +check-whitespace:
> > >> +  image: ubuntu:latest
> > >> +  before_script:
> > >> +    - ./ci/install-dependencies.sh
> > >
> > > Do we actually need to install dependencies? I imagine all that's needed
> > > would be Git.
> > >
> > > Other than this question the patch series looks good to me, thanks!
> > 
> > I am a bit puzzled.  Is the proposal to check our sources with a
> > pre-built Git (which by definition would be a bit older than what is
> > being tested)?
> 
> The GitLab `check-whitespace` CI job only needs Git to run and uses
> `ci/install-dependencies.sh` to download a pre-built Git package via
> `apt-get` since `ubuntu:latest` is the container image used. The 
> `ci/install-dependencies.sh` script also fetches a bunch of other 
> dependencies which are not needed. 
> 
> I think Patrick is proposing, to further simplify, we avoid using
> `ci/install-dependencies.sh` and only fetch Git. Patrick please correct
> me if I misunderstand :)

I just wondered how GitHub Workflows manages without installing any
dependencies at all. Is Git already part of the default images? If so,
there is no need to install anything and we can just execute the script
directly, which saves some time.

If there is a need to install Git we could either just manually install
it in the `before_script` or leave it as-is. I don't mind it much either
way.

Patrick

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

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

[Justin Tobler]

On 24/05/07 06:12AM, Patrick Steinhardt wrote:
 
> I just wondered how GitHub Workflows manages without installing any
> dependencies at all. Is Git already part of the default images? If so,
> there is no need to install anything and we can just execute the script
> directly, which saves some time.

Git is not bundled by default in the "ubuntu:latest" container image. We
would have to install it ourselves. As for why GitHub Workflows do not
have to install Git, it looks like each runner has a defined set of
included software which happens to include Git.

https://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md

> If there is a need to install Git we could either just manually install
> it in the `before_script` or leave it as-is. I don't mind it much either
> way.

I don't have strong opinions, but I think I would prefer to leave it
as-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another
version unless there is addional feedback. Thanks

-Justin

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

[Patrick Steinhardt]

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

On Tue, May 07, 2024 at 01:06:04PM -0500, Justin Tobler wrote:
> On 24/05/07 06:12AM, Patrick Steinhardt wrote:
>  
> > I just wondered how GitHub Workflows manages without installing any
> > dependencies at all. Is Git already part of the default images? If so,
> > there is no need to install anything and we can just execute the script
> > directly, which saves some time.
> 
> Git is not bundled by default in the "ubuntu:latest" container image. We
> would have to install it ourselves. As for why GitHub Workflows do not
> have to install Git, it looks like each runner has a defined set of
> included software which happens to include Git.

Okay.

> https://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md
> 
> > If there is a need to install Git we could either just manually install
> > it in the `before_script` or leave it as-is. I don't mind it much either
> > way.
> 
> I don't have strong opinions, but I think I would prefer to leave it
> as-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another
> version unless there is addional feedback. Thanks

Fair enough. Let's not overthink this then and get this merged.

Patrick

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