{"thread":{"id":"61395","subject":"[PATCH 0/2] Add GitLab CI to check for whitespace errors","startedAt":"2024-04-30T00:35:00Z","lastAt":"2024-05-07T18:12:41Z","messageCount":42,"participants":["Justin Tobler","James Liu","Patrick Steinhardt","Taylor Blau","Junio C Hamano","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"493713","messageId":"20240430003323.6210-1-jltobler@gmail.com","threadId":"61395","inReplyTo":null,"subject":"[PATCH 0/2] Add GitLab CI to check for whitespace errors","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T00:33:21Z","receivedAt":"2024-04-30T00:35:00Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Hello everyone,\n\nThe currently configured GitLab pipelines do not include a CI job to\ncheck for whitespace errors. Instead of generalizing the existing GitHub\naction in `.github/workflows/check-whitespace.yml`, this change opts to\ncreate a separate generalized `ci/whitespace-check.sh` script to build\nfrom. The existing action does extra work to create a GitHub specific\nsummary artifact that is markdown formatted with links to commits and\nblobs. If this summary feature is not considered necessary, the GitHub\naction could be reworked to use the general whitespace check.\n\nHe is a link to a successful execution of the CI job:\nhttps://gitlab.com/gitlab-org/git/-/jobs/6744313860\n\nI also added a small change to by default collapse grouped sections of\nCI output to reduce noise.\n\n-Justin\n\nJustin Tobler (2):\n  ci: pre-collapse GitLab CI sections\n  gitlab-ci: add whitespace error check\n\n .gitlab-ci.yml         |  9 +++++++++\n ci/check-whitespace.sh | 16 ++++++++++++++++\n ci/lib.sh              |  2 +-\n 3 files changed, 26 insertions(+), 1 deletion(-)\n create mode 100755 ci/check-whitespace.sh\n\n\nbase-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8\n-- \n2.44.0\n\n"},{"id":"493714","messageId":"20240430003323.6210-2-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240430003323.6210-1-jltobler@gmail.com","subject":"[PATCH 1/2] ci: pre-collapse GitLab CI sections","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T00:33:22Z","receivedAt":"2024-04-30T00:35:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Sections of CI output defined by `begin_group()` and `end_group()` are\nexpanded in GitLab pipelines by default. This can make CI job output\nrather noisy and harder to navigate. Update the behavior for GitLab\npipelines to now collapse sections by default.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 0a73fc7bd1..02e5e058dd 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -18,7 +18,7 @@ elif test true = \"$GITLAB_CI\"\n then\n \tbegin_group () {\n \t\tneed_to_end_group=t\n-\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)\\r\\e[0K$1\\n\"\n+\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)[collapsed=true]\\r\\e[0K$1\\n\"\n \t\ttrap \"end_group '$1'\" EXIT\n \t\tset -x\n \t}\n-- \n2.44.0\n\n"},{"id":"493715","messageId":"20240430003323.6210-3-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240430003323.6210-1-jltobler@gmail.com","subject":"[PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T00:33:23Z","receivedAt":"2024-04-30T00:35:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"To check for whitespace errors introduced by a set of changes, there is\nthe `.github/workflows/check-whitespace.yml` GitHub action. This script\nexecutes `git log --check` over a range containing the new commits and\nparses the output to generate a markdown formatted artifact that\nsummarizes detected errors with GitHub links to the affected commits and\nblobs.\n\nSince this script is rather specific to GitHub actions, a more general\nand simple `ci/check-whitespace.sh` is added instead that functions the\nsame, but does not generate the markdown file for the action summary.\nFrom this, a new GitLab CI job is added to support the whitespace error\ncheck.\n\nNote that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\navailable in GitLab merge request pipelines and therefore the CI job is\nconfigured to only run as part of those pipelines.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .gitlab-ci.yml         |  9 +++++++++\n ci/check-whitespace.sh | 16 ++++++++++++++++\n 2 files changed, 25 insertions(+)\n create mode 100755 ci/check-whitespace.sh\n\ndiff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\nindex c0fa2fe90b..31cf420a11 100644\n--- a/.gitlab-ci.yml\n+++ b/.gitlab-ci.yml\n@@ -102,3 +102,12 @@ static-analysis:\n   script:\n     - ./ci/run-static-analysis.sh\n     - ./ci/check-directional-formatting.bash\n+\n+check-whitespace:\n+  image: ubuntu:latest\n+  before_script:\n+    - ./ci/install-docker-dependencies.sh\n+  script:\n+    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n+  rules:\n+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\ndiff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\nnew file mode 100755\nindex 0000000000..1cad2d7374\n--- /dev/null\n+++ b/ci/check-whitespace.sh\n@@ -0,0 +1,16 @@\n+#! /bin/sh\n+#\n+# Check that commits after a specified point do not contain new or modified\n+# lines with whitespace errors.\n+#\n+\n+baseSha=${1}\n+\n+git log --check --pretty=format:\"---% h% s\" ${baseSha}..\n+if test $? -ne 0\n+then\n+\techo \"A whitespace issue was found in one or more of the commits.\"\n+\techo \"Run the following command to resolve whitespace issues:\"\n+\techo \"\\tgit rebase --whitespace=fix ${baseSha}\"\n+\texit 2\n+fi\n-- \n2.44.0\n\n"},{"id":"493719","messageId":"D0X33QRECNTC.33V5ZK9NYIE1F@jamesliu.io","threadId":"61395","inReplyTo":"20240430003323.6210-3-jltobler@gmail.com","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-30T01:59:45Z","receivedAt":"2024-04-30T01:59:50Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Thanks for putting this together, Justin!\n\n> +check-whitespace:\n> +  image: ubuntu:latest\n\nI wonder if we should pin to `ubuntu:22.04` and only update this for\neach LTS release. It seems like we've done this for the\n`static-analysis` job above.\n\n> +  before_script:\n> +    - ./ci/install-docker-dependencies.sh\n> +  script:\n> +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n> +  rules:\n> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> new file mode 100755\n> index 0000000000..1cad2d7374\n> --- /dev/null\n> +++ b/ci/check-whitespace.sh\n> @@ -0,0 +1,16 @@\n> +#! /bin/sh\n\nnit: there seems to be extra whitespace after the shebang :D\n\n"},{"id":"493720","messageId":"5p5ot4juki7v2lycups4jhadxcw5yzpqtjkwgxzwrlrpu6mlmt@gis6xmwjjh6o","threadId":"61395","inReplyTo":"D0X33QRECNTC.33V5ZK9NYIE1F@jamesliu.io","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T03:01:15Z","receivedAt":"2024-04-30T03:02:39Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/30 11:59AM, James Liu wrote:\n> Thanks for putting this together, Justin!\n> \n> > +check-whitespace:\n> > +  image: ubuntu:latest\n> \n> I wonder if we should pin to `ubuntu:22.04` and only update this for\n> each LTS release. It seems like we've done this for the\n> `static-analysis` job above.\n\nI'm not sure if it particually matters. I know Patrick recently\nsubmitted the following also using `ubuntu:latest`:\nhttps://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r\n\nI can change it though if there is strong opinion one way or the other.\n\n> \n> > +  before_script:\n> > +    - ./ci/install-docker-dependencies.sh\n> > +  script:\n> > +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n> > +  rules:\n> > +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> > new file mode 100755\n> > index 0000000000..1cad2d7374\n> > --- /dev/null\n> > +++ b/ci/check-whitespace.sh\n> > @@ -0,0 +1,16 @@\n> > +#! /bin/sh\n> \n> nit: there seems to be extra whitespace after the shebang :D\n> \nWhoops... I'll address this in a subsequent version.\n\n-Justin\n"},{"id":"493728","messageId":"ZjB77nSMou0ssu-V@tanuki","threadId":"61395","inReplyTo":"20240430003323.6210-3-jltobler@gmail.com","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T05:04:46Z","receivedAt":"2024-04-30T05:04:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> To check for whitespace errors introduced by a set of changes, there is\n> the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> executes `git log --check` over a range containing the new commits and\n> parses the output to generate a markdown formatted artifact that\n> summarizes detected errors with GitHub links to the affected commits and\n> blobs.\n> \n> Since this script is rather specific to GitHub actions, a more general\n> and simple `ci/check-whitespace.sh` is added instead that functions the\n> same, but does not generate the markdown file for the action summary.\n> From this, a new GitLab CI job is added to support the whitespace error\n> check.\n\nI still wonder whether we can unify these. Yes, the GitHub thing is\nquite specific. But ultimately, what it does is to generate a proper\nsummary of where exactly the whitespaces issues are, which is something\nthat your version doesn't do. It's useful though for consumers of a\nfailed CI job to know exactly which commit has the issue.\n\nSo can't we pull out the logic into a script, refactor it such that it\nknows to print both GitHub- and GitLab-style URLs, and then also print\nthe summary in GitLab CI?\n\n> Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n> available in GitLab merge request pipelines and therefore the CI job is\n> configured to only run as part of those pipelines.\n> \n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n>  .gitlab-ci.yml         |  9 +++++++++\n>  ci/check-whitespace.sh | 16 ++++++++++++++++\n>  2 files changed, 25 insertions(+)\n>  create mode 100755 ci/check-whitespace.sh\n> \n> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> index c0fa2fe90b..31cf420a11 100644\n> --- a/.gitlab-ci.yml\n> +++ b/.gitlab-ci.yml\n> @@ -102,3 +102,12 @@ static-analysis:\n>    script:\n>      - ./ci/run-static-analysis.sh\n>      - ./ci/check-directional-formatting.bash\n> +\n> +check-whitespace:\n> +  image: ubuntu:latest\n> +  before_script:\n> +    - ./ci/install-docker-dependencies.sh\n> +  script:\n> +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n\nLet's quote this variable.\n\n> +  rules:\n> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> new file mode 100755\n> index 0000000000..1cad2d7374\n> --- /dev/null\n> +++ b/ci/check-whitespace.sh\n> @@ -0,0 +1,16 @@\n> +#! /bin/sh\n> +#\n> +# Check that commits after a specified point do not contain new or modified\n> +# lines with whitespace errors.\n> +#\n> +\n> +baseSha=${1}\n\nTwo nits: first, I wouldn't call it \"sha\" because it really is a commit\nID that may or may not be SHA if we were ever to grow a hash function\nthat is not SHA. So \"baseCommit\" would be more descriptive.\n\nSecond, our shell variables are typically written in all-uppercase. So\nthat'd make it \"BASE_COMMIT\".\n\n> +git log --check --pretty=format:\"---% h% s\" ${baseSha}..\n\nNit: there's no need for the curly braces here. Also, let's quote the\nvalue.\n\nPatrick\n\n> +if test $? -ne 0\n> +then\n> +\techo \"A whitespace issue was found in one or more of the commits.\"\n> +\techo \"Run the following command to resolve whitespace issues:\"\n> +\techo \"\\tgit rebase --whitespace=fix ${baseSha}\"\n> +\texit 2\n> +fi\n> -- \n> 2.44.0\n> \n> \n"},{"id":"493729","messageId":"ZjB79eQbuQKrTzF8@tanuki","threadId":"61395","inReplyTo":"5p5ot4juki7v2lycups4jhadxcw5yzpqtjkwgxzwrlrpu6mlmt@gis6xmwjjh6o","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T05:04:53Z","receivedAt":"2024-04-30T05:04:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 29, 2024 at 10:01:15PM -0500, Justin Tobler wrote:\n> On 24/04/30 11:59AM, James Liu wrote:\n> > Thanks for putting this together, Justin!\n> > \n> > > +check-whitespace:\n> > > +  image: ubuntu:latest\n> > \n> > I wonder if we should pin to `ubuntu:22.04` and only update this for\n> > each LTS release. It seems like we've done this for the\n> > `static-analysis` job above.\n> \n> I'm not sure if it particually matters. I know Patrick recently\n> submitted the following also using `ubuntu:latest`:\n> https://lore.kernel.org/git/01fb94999f8e2014ba4d09ce7451a4f5d315ee72.1714371146.git.ps@pks.im/#r\n> \n> I can change it though if there is strong opinion one way or the other.\n\nI'd say it probably doesn't matter much for this case. But using\n`ubuntu:latest` means that there's less maintenance going forward\nbecause we don't ever have to update it.\n\nPatrick\n"},{"id":"493758","messageId":"ZjDgm602Zry2inpa@nand.local","threadId":"61395","inReplyTo":"ZjB77nSMou0ssu-V@tanuki","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-30T12:14:19Z","receivedAt":"2024-04-30T12:14:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 30, 2024 at 07:04:46AM +0200, Patrick Steinhardt wrote:\n> On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > To check for whitespace errors introduced by a set of changes, there is\n> > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > executes `git log --check` over a range containing the new commits and\n> > parses the output to generate a markdown formatted artifact that\n> > summarizes detected errors with GitHub links to the affected commits and\n> > blobs.\n> >\n> > Since this script is rather specific to GitHub actions, a more general\n> > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > same, but does not generate the markdown file for the action summary.\n> > From this, a new GitLab CI job is added to support the whitespace error\n> > check.\n>\n> I still wonder whether we can unify these. Yes, the GitHub thing is\n> quite specific. But ultimately, what it does is to generate a proper\n> summary of where exactly the whitespaces issues are, which is something\n> that your version doesn't do. It's useful though for consumers of a\n> failed CI job to know exactly which commit has the issue.\n>\n> So can't we pull out the logic into a script, refactor it such that it\n> knows to print both GitHub- and GitLab-style URLs, and then also print\n> the summary in GitLab CI?\n\nThis was my feeling as well when I read the cover letter. I don't think\nwe should either (a) have to remove functionality from the GitHub\nspecific version of this job, or (b) have to maintain two different but\nhighly similar implementations of the same job.\n\nIt seems more reasonable that we'd do some third option, which is what\nPatrick suggests.\n\nThanks,\nTaylor\n"},{"id":"493777","messageId":"zphrzgroei4civl7q2ls5cvabyzjbexfq6tjhdy476rmaqu6m5@ik3pedton6ww","threadId":"61395","inReplyTo":"ZjB77nSMou0ssu-V@tanuki","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T14:00:59Z","receivedAt":"2024-04-30T14:02:15Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/30 07:04AM, Patrick Steinhardt wrote:\n> On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > To check for whitespace errors introduced by a set of changes, there is\n> > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > executes `git log --check` over a range containing the new commits and\n> > parses the output to generate a markdown formatted artifact that\n> > summarizes detected errors with GitHub links to the affected commits and\n> > blobs.\n> > \n> > Since this script is rather specific to GitHub actions, a more general\n> > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > same, but does not generate the markdown file for the action summary.\n> > From this, a new GitLab CI job is added to support the whitespace error\n> > check.\n> \n> I still wonder whether we can unify these. Yes, the GitHub thing is\n> quite specific. But ultimately, what it does is to generate a proper\n> summary of where exactly the whitespaces issues are, which is something\n> that your version doesn't do. It's useful though for consumers of a\n> failed CI job to know exactly which commit has the issue.\n\nJust to clarify, this new CI job still prints the output of \n`git log --check` which details the exact commit and file with line\nnumber of the whitespace error. The difference is that it does not write\nan additional markdown file with links to the commit and blob.\n\nHere is a failed execution of the GitLab whitespace check job:\nhttps://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289\n\n> \n> So can't we pull out the logic into a script, refactor it such that it\n> knows to print both GitHub- and GitLab-style URLs, and then also print\n> the summary in GitLab CI?\n\nWe can do this, but for GitLab CI there probably isn't a point to\ngenerating a summary file since there is nothing that would pick it up\nand display it. Having links though directly in the job output would be\nnice. I'll give it another go.\n\n> \n> > Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n> > available in GitLab merge request pipelines and therefore the CI job is\n> > configured to only run as part of those pipelines.\n> > \n> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> > ---\n> >  .gitlab-ci.yml         |  9 +++++++++\n> >  ci/check-whitespace.sh | 16 ++++++++++++++++\n> >  2 files changed, 25 insertions(+)\n> >  create mode 100755 ci/check-whitespace.sh\n> > \n> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> > index c0fa2fe90b..31cf420a11 100644\n> > --- a/.gitlab-ci.yml\n> > +++ b/.gitlab-ci.yml\n> > @@ -102,3 +102,12 @@ static-analysis:\n> >    script:\n> >      - ./ci/run-static-analysis.sh\n> >      - ./ci/check-directional-formatting.bash\n> > +\n> > +check-whitespace:\n> > +  image: ubuntu:latest\n> > +  before_script:\n> > +    - ./ci/install-docker-dependencies.sh\n> > +  script:\n> > +    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n> \n> Let's quote this variable.\n> \n> > +  rules:\n> > +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> > new file mode 100755\n> > index 0000000000..1cad2d7374\n> > --- /dev/null\n> > +++ b/ci/check-whitespace.sh\n> > @@ -0,0 +1,16 @@\n> > +#! /bin/sh\n> > +#\n> > +# Check that commits after a specified point do not contain new or modified\n> > +# lines with whitespace errors.\n> > +#\n> > +\n> > +baseSha=${1}\n> \n> Two nits: first, I wouldn't call it \"sha\" because it really is a commit\n> ID that may or may not be SHA if we were ever to grow a hash function\n> that is not SHA. So \"baseCommit\" would be more descriptive.\n> \n> Second, our shell variables are typically written in all-uppercase. So\n> that'd make it \"BASE_COMMIT\".\n> \n> > +git log --check --pretty=format:\"---% h% s\" ${baseSha}..\n> \n> Nit: there's no need for the curly braces here. Also, let's quote the\n> value.\n\nThanks for the review Patrick! I'll address these in the next version :)\n\n-Justin\n"},{"id":"493778","messageId":"ZjD6xNuncrq9fSXZ@tanuki","threadId":"61395","inReplyTo":"zphrzgroei4civl7q2ls5cvabyzjbexfq6tjhdy476rmaqu6m5@ik3pedton6ww","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T14:05:56Z","receivedAt":"2024-04-30T14:06:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:\n> On 24/04/30 07:04AM, Patrick Steinhardt wrote:\n> > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > > To check for whitespace errors introduced by a set of changes, there is\n> > > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > > executes `git log --check` over a range containing the new commits and\n> > > parses the output to generate a markdown formatted artifact that\n> > > summarizes detected errors with GitHub links to the affected commits and\n> > > blobs.\n> > > \n> > > Since this script is rather specific to GitHub actions, a more general\n> > > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > > same, but does not generate the markdown file for the action summary.\n> > > From this, a new GitLab CI job is added to support the whitespace error\n> > > check.\n> > \n> > I still wonder whether we can unify these. Yes, the GitHub thing is\n> > quite specific. But ultimately, what it does is to generate a proper\n> > summary of where exactly the whitespaces issues are, which is something\n> > that your version doesn't do. It's useful though for consumers of a\n> > failed CI job to know exactly which commit has the issue.\n> \n> Just to clarify, this new CI job still prints the output of \n> `git log --check` which details the exact commit and file with line\n> number of the whitespace error. The difference is that it does not write\n> an additional markdown file with links to the commit and blob.\n> \n> Here is a failed execution of the GitLab whitespace check job:\n> https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289\n\nOkay, fair enough. I'm still of the opinion that the infra here should\nbe shared.\n\n> > So can't we pull out the logic into a script, refactor it such that it\n> > knows to print both GitHub- and GitLab-style URLs, and then also print\n> > the summary in GitLab CI?\n> \n> We can do this, but for GitLab CI there probably isn't a point to\n> generating a summary file since there is nothing that would pick it up\n> and display it. Having links though directly in the job output would be\n> nice. I'll give it another go.\n\nWell, we could print the output to the console so that a user can see it\nwhen they open the failed job. The nice formatting may be kind of moot,\nbut on the other hand it doesn't hurt, either. I guess most people are\nused to reading plain markdown-style docs anyway.\n\nPatrick\n"},{"id":"493784","messageId":"l55hxxuv5ohtno3gfjgq6mu7cqakfohon4vd7r535ztzadarin@iz4fbuwjeeug","threadId":"61395","inReplyTo":"ZjD6xNuncrq9fSXZ@tanuki","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T14:41:47Z","receivedAt":"2024-04-30T14:43:03Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/30 04:05PM, Patrick Steinhardt wrote:\n> On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:\n> > On 24/04/30 07:04AM, Patrick Steinhardt wrote:\n> > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > > > To check for whitespace errors introduced by a set of changes, there is\n> > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > > > executes `git log --check` over a range containing the new commits and\n> > > > parses the output to generate a markdown formatted artifact that\n> > > > summarizes detected errors with GitHub links to the affected commits and\n> > > > blobs.\n> > > > \n> > > > Since this script is rather specific to GitHub actions, a more general\n> > > > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > > > same, but does not generate the markdown file for the action summary.\n> > > > From this, a new GitLab CI job is added to support the whitespace error\n> > > > check.\n> > > \n> > > I still wonder whether we can unify these. Yes, the GitHub thing is\n> > > quite specific. But ultimately, what it does is to generate a proper\n> > > summary of where exactly the whitespaces issues are, which is something\n> > > that your version doesn't do. It's useful though for consumers of a\n> > > failed CI job to know exactly which commit has the issue.\n> > \n> > Just to clarify, this new CI job still prints the output of \n> > `git log --check` which details the exact commit and file with line\n> > number of the whitespace error. The difference is that it does not write\n> > an additional markdown file with links to the commit and blob.\n> > \n> > Here is a failed execution of the GitLab whitespace check job:\n> > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289\n> \n> Okay, fair enough. I'm still of the opinion that the infra here should\n> be shared.\n> \n> > > So can't we pull out the logic into a script, refactor it such that it\n> > > knows to print both GitHub- and GitLab-style URLs, and then also print\n> > > the summary in GitLab CI?\n> > \n> > We can do this, but for GitLab CI there probably isn't a point to\n> > generating a summary file since there is nothing that would pick it up\n> > and display it. Having links though directly in the job output would be\n> > nice. I'll give it another go.\n> \n> Well, we could print the output to the console so that a user can see it\n> when they open the failed job. The nice formatting may be kind of moot,\n> but on the other hand it doesn't hurt, either. I guess most people are\n> used to reading plain markdown-style docs anyway.\n\nI'm thinking we can generalize the summary writing in some manner. When\nrun as a GitHub action, the summary can be markdown formatted and\nwritten to `$GITHUB_STEP_SUMMARY` with no output to the console as done\ntoday. When run as GitLab CI, the summary can be written directly to\nconsole with links. All other runs just output normally to console.\n\n-Justin\n"},{"id":"493785","messageId":"ZjEEF-5xGl6tjOAG@tanuki","threadId":"61395","inReplyTo":"l55hxxuv5ohtno3gfjgq6mu7cqakfohon4vd7r535ztzadarin@iz4fbuwjeeug","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T14:45:43Z","receivedAt":"2024-04-30T14:45:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote:\n> On 24/04/30 04:05PM, Patrick Steinhardt wrote:\n> > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:\n> > > On 24/04/30 07:04AM, Patrick Steinhardt wrote:\n> > > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > > > > To check for whitespace errors introduced by a set of changes, there is\n> > > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > > > > executes `git log --check` over a range containing the new commits and\n> > > > > parses the output to generate a markdown formatted artifact that\n> > > > > summarizes detected errors with GitHub links to the affected commits and\n> > > > > blobs.\n> > > > > \n> > > > > Since this script is rather specific to GitHub actions, a more general\n> > > > > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > > > > same, but does not generate the markdown file for the action summary.\n> > > > > From this, a new GitLab CI job is added to support the whitespace error\n> > > > > check.\n> > > > \n> > > > I still wonder whether we can unify these. Yes, the GitHub thing is\n> > > > quite specific. But ultimately, what it does is to generate a proper\n> > > > summary of where exactly the whitespaces issues are, which is something\n> > > > that your version doesn't do. It's useful though for consumers of a\n> > > > failed CI job to know exactly which commit has the issue.\n> > > \n> > > Just to clarify, this new CI job still prints the output of \n> > > `git log --check` which details the exact commit and file with line\n> > > number of the whitespace error. The difference is that it does not write\n> > > an additional markdown file with links to the commit and blob.\n> > > \n> > > Here is a failed execution of the GitLab whitespace check job:\n> > > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289\n> > \n> > Okay, fair enough. I'm still of the opinion that the infra here should\n> > be shared.\n> > \n> > > > So can't we pull out the logic into a script, refactor it such that it\n> > > > knows to print both GitHub- and GitLab-style URLs, and then also print\n> > > > the summary in GitLab CI?\n> > > \n> > > We can do this, but for GitLab CI there probably isn't a point to\n> > > generating a summary file since there is nothing that would pick it up\n> > > and display it. Having links though directly in the job output would be\n> > > nice. I'll give it another go.\n> > \n> > Well, we could print the output to the console so that a user can see it\n> > when they open the failed job. The nice formatting may be kind of moot,\n> > but on the other hand it doesn't hurt, either. I guess most people are\n> > used to reading plain markdown-style docs anyway.\n> \n> I'm thinking we can generalize the summary writing in some manner. When\n> run as a GitHub action, the summary can be markdown formatted and\n> written to `$GITHUB_STEP_SUMMARY` with no output to the console as done\n> today. When run as GitLab CI, the summary can be written directly to\n> console with links. All other runs just output normally to console.\n\nThe script can probably be generalized to take a file name as argument.\nFor GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we\npass in a temporary filename that we than simply cat(1) to the console.\nThat'd allow us to move the CI-specific bits into the respective CIs\nwhereas the script itself remains generic.\n\nPatrick\n"},{"id":"493788","messageId":"nroeeqyqav4pxye7s4o3fa763btsjhbsmggw6pqh5f7vgcjuym@mxcz4cbmsqvs","threadId":"61395","inReplyTo":"ZjEEF-5xGl6tjOAG@tanuki","subject":"Re: [PATCH 2/2] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-04-30T14:58:34Z","receivedAt":"2024-04-30T14:59:50Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/04/30 04:45PM, Patrick Steinhardt wrote:\n> On Tue, Apr 30, 2024 at 09:41:47AM -0500, Justin Tobler wrote:\n> > On 24/04/30 04:05PM, Patrick Steinhardt wrote:\n> > > On Tue, Apr 30, 2024 at 09:00:59AM -0500, Justin Tobler wrote:\n> > > > On 24/04/30 07:04AM, Patrick Steinhardt wrote:\n> > > > > On Mon, Apr 29, 2024 at 07:33:23PM -0500, Justin Tobler wrote:\n> > > > > > To check for whitespace errors introduced by a set of changes, there is\n> > > > > > the `.github/workflows/check-whitespace.yml` GitHub action. This script\n> > > > > > executes `git log --check` over a range containing the new commits and\n> > > > > > parses the output to generate a markdown formatted artifact that\n> > > > > > summarizes detected errors with GitHub links to the affected commits and\n> > > > > > blobs.\n> > > > > > \n> > > > > > Since this script is rather specific to GitHub actions, a more general\n> > > > > > and simple `ci/check-whitespace.sh` is added instead that functions the\n> > > > > > same, but does not generate the markdown file for the action summary.\n> > > > > > From this, a new GitLab CI job is added to support the whitespace error\n> > > > > > check.\n> > > > > \n> > > > > I still wonder whether we can unify these. Yes, the GitHub thing is\n> > > > > quite specific. But ultimately, what it does is to generate a proper\n> > > > > summary of where exactly the whitespaces issues are, which is something\n> > > > > that your version doesn't do. It's useful though for consumers of a\n> > > > > failed CI job to know exactly which commit has the issue.\n> > > > \n> > > > Just to clarify, this new CI job still prints the output of \n> > > > `git log --check` which details the exact commit and file with line\n> > > > number of the whitespace error. The difference is that it does not write\n> > > > an additional markdown file with links to the commit and blob.\n> > > > \n> > > > Here is a failed execution of the GitLab whitespace check job:\n> > > > https://gitlab.com/gitlab-org/git/-/jobs/6749580210#L1289\n> > > \n> > > Okay, fair enough. I'm still of the opinion that the infra here should\n> > > be shared.\n> > > \n> > > > > So can't we pull out the logic into a script, refactor it such that it\n> > > > > knows to print both GitHub- and GitLab-style URLs, and then also print\n> > > > > the summary in GitLab CI?\n> > > > \n> > > > We can do this, but for GitLab CI there probably isn't a point to\n> > > > generating a summary file since there is nothing that would pick it up\n> > > > and display it. Having links though directly in the job output would be\n> > > > nice. I'll give it another go.\n> > > \n> > > Well, we could print the output to the console so that a user can see it\n> > > when they open the failed job. The nice formatting may be kind of moot,\n> > > but on the other hand it doesn't hurt, either. I guess most people are\n> > > used to reading plain markdown-style docs anyway.\n> > \n> > I'm thinking we can generalize the summary writing in some manner. When\n> > run as a GitHub action, the summary can be markdown formatted and\n> > written to `$GITHUB_STEP_SUMMARY` with no output to the console as done\n> > today. When run as GitLab CI, the summary can be written directly to\n> > console with links. All other runs just output normally to console.\n> \n> The script can probably be generalized to take a file name as argument.\n> For GitHub we'd then pass `$GITHUB_STEP_SUMMARY`, whereas for GitLab we\n> pass in a temporary filename that we than simply cat(1) to the console.\n> That'd allow us to move the CI-specific bits into the respective CIs\n> whereas the script itself remains generic.\n\nThis sounds good, I was also considering this. We will still need\nCI-specific bits in the script in order to generate correctly formatted\nlinks. That is, unless we also want to pass these as arguments, but that\nmight start to get a little messy.\n\n-Justin\n\n"},{"id":"493938","messageId":"20240502193840.105355-1-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240430003323.6210-1-jltobler@gmail.com","subject":"[PATCH v2 0/5] Add GitLab CI to check for whitespace errors","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:34Z","receivedAt":"2024-05-02T19:41:15Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Hello again,\n\nThis is the second version of my patch series to add a GitLab CI job to\ncheck for whitespace errors. The main differnece with this version is\nthat it first generalizes the existing GitHub whitespace check CI job\nallowing the GitLab one to reuse it.\n\nTo validate that these changes do not break the existing GitHub CI, here\nare some links to a successful run and a failed run:\n\n- https://github.com/gitgitgadget/git/actions/runs/8929081916\n- https://github.com/gitgitgadget/git/actions/runs/8928887225\n\nTo validate that these changes also work on GitLab CI, here some\nadditional links to a succdessful run and a failed run:\n\n- https://gitlab.com/gitlab-org/git/-/jobs/6768304381\n- https://gitlab.com/gitlab-org/git/-/jobs/6768282645\n\nThanks,\n-Justin\n\nJustin Tobler (5):\n  ci: pre-collapse GitLab CI sections\n  github-ci: fix link to whitespace error\n  ci: separate whitespace check script\n  ci: make the whitespace report optional\n  gitlab-ci: add whitespace error check\n\n .github/workflows/check-whitespace.yml | 67 ++----------------\n .gitlab-ci.yml                         |  9 +++\n ci/check-whitespace.sh                 | 95 ++++++++++++++++++++++++++\n ci/lib.sh                              |  2 +-\n 4 files changed, 109 insertions(+), 64 deletions(-)\n create mode 100755 ci/check-whitespace.sh\n\nRange-diff against v1:\n1:  924d3eb23c = 1:  924d3eb23c ci: pre-collapse GitLab CI sections\n-:  ---------- > 2:  c8d8b444dc github-ci: fix link to whitespace error\n-:  ---------- > 3:  6b44b21dda ci: separate whitespace check script\n-:  ---------- > 4:  87dfd1d5a9 ci: make the whitespace report optional\n2:  624e68a8d2 ! 5:  175b300e91 gitlab-ci: add whitespace error check\n    @@ Metadata\n      ## Commit message ##\n         gitlab-ci: add whitespace error check\n     \n    -    To check for whitespace errors introduced by a set of changes, there is\n    -    the `.github/workflows/check-whitespace.yml` GitHub action. This script\n    -    executes `git log --check` over a range containing the new commits and\n    -    parses the output to generate a markdown formatted artifact that\n    -    summarizes detected errors with GitHub links to the affected commits and\n    -    blobs.\n    -\n    -    Since this script is rather specific to GitHub actions, a more general\n    -    and simple `ci/check-whitespace.sh` is added instead that functions the\n    -    same, but does not generate the markdown file for the action summary.\n    -    From this, a new GitLab CI job is added to support the whitespace error\n    -    check.\n    +    GitLab CI does not have a job to check for whitespace errors introduced\n    +    by a set of changes. Reuse the existing generic `whitespace-check.sh` to\n    +    create the job for GitLab pipelines.\n     \n         Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n         available in GitLab merge request pipelines and therefore the CI job is\n    @@ .gitlab-ci.yml: static-analysis:\n     +  before_script:\n     +    - ./ci/install-docker-dependencies.sh\n     +  script:\n    -+    - ./ci/check-whitespace.sh $CI_MERGE_REQUEST_TARGET_BRANCH_SHA\n    ++    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n     +  rules:\n     +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n    -\n    - ## ci/check-whitespace.sh (new) ##\n    -@@\n    -+#! /bin/sh\n    -+#\n    -+# Check that commits after a specified point do not contain new or modified\n    -+# lines with whitespace errors.\n    -+#\n    -+\n    -+baseSha=${1}\n    -+\n    -+git log --check --pretty=format:\"---% h% s\" ${baseSha}..\n    -+if test $? -ne 0\n    -+then\n    -+\techo \"A whitespace issue was found in one or more of the commits.\"\n    -+\techo \"Run the following command to resolve whitespace issues:\"\n    -+\techo \"\\tgit rebase --whitespace=fix ${baseSha}\"\n    -+\texit 2\n    -+fi\n-- \n2.45.0\n\n"},{"id":"493939","messageId":"20240502193840.105355-2-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v2 1/5] ci: pre-collapse GitLab CI sections","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:35Z","receivedAt":"2024-05-02T19:41:19Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Sections of CI output defined by `begin_group()` and `end_group()` are\nexpanded in GitLab pipelines by default. This can make CI job output\nrather noisy and harder to navigate. Update the behavior for GitLab\npipelines to now collapse sections by default.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 0a73fc7bd1..02e5e058dd 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -18,7 +18,7 @@ elif test true = \"$GITLAB_CI\"\n then\n \tbegin_group () {\n \t\tneed_to_end_group=t\n-\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)\\r\\e[0K$1\\n\"\n+\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)[collapsed=true]\\r\\e[0K$1\\n\"\n \t\ttrap \"end_group '$1'\" EXIT\n \t\tset -x\n \t}\n-- \n2.45.0\n\n"},{"id":"493940","messageId":"20240502193840.105355-3-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v2 2/5] github-ci: fix link to whitespace error","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:36Z","receivedAt":"2024-05-02T19:41:20Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When the `check-whitespace` CI job detects whitespace errors, a\nformatted summary of the issue is generated. This summary contains links\nto the commits and blobs responsible for the whitespace errors. The\ngenerated links for blobs do not work and result in a 404.\n\nInstead of using the reference name in the link, use the commit ID\ndirectly. This fixes the broken link and also helps enable future\ngeneralization of the script for other CI providers by removing one of\nthe GitHub specific CI variables used.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .github/workflows/check-whitespace.yml | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex a241a63428..a3a6913ecc 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -31,14 +31,15 @@ jobs:\n         commit=\n         commitText=\n         commitTextmd=\n-        goodparent=\n+        goodParent=\n         while read dash sha etc\n         do\n           case \"${dash}\" in\n-          \"---\")\n-            if test -z \"${commit}\"\n+          \"---\") # Line contains commit information.\n+            if test -z \"${goodParent}\"\n             then\n-              goodparent=${sha}\n+              # Assume the commit has no whitespace errors until detected otherwise.\n+              goodParent=${sha}\n             fi\n             commit=\"${sha}\"\n             commitText=\"${sha} ${etc}\"\n@@ -46,18 +47,18 @@ jobs:\n             ;;\n           \"\")\n             ;;\n-          *)\n-            if test -n \"${commit}\"\n+          *) # Line contains whitespace error information for current commit.\n+            if test -n \"${goodParent}\"\n             then\n               problems+=(\"1) --- ${commitTextmd}\")\n               echo \"\"\n               echo \"--- ${commitText}\"\n-              commit=\n+              goodParent=\n             fi\n             case \"${dash}\" in\n             *:[1-9]*:) # contains file and line number information\n               dashend=${dash#*:}\n-              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${{github.event.pull_request.head.ref}}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n+              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n               ;;\n             *)\n               problems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n@@ -70,15 +71,15 @@ jobs:\n \n         if test ${#problems[*]} -gt 0\n         then\n-          if test -z \"${commit}\"\n+          if test -z \"${goodParent}\"\n           then\n-            goodparent=${baseSha: 0:7}\n+            goodParent=${baseSha: 0:7}\n           fi\n           echo \"🛑 Please review the Summary output for further information.\"\n           echo \"### :x: A whitespace issue was found in one or more of the commits.\" >$GITHUB_STEP_SUMMARY\n           echo \"\" >>$GITHUB_STEP_SUMMARY\n           echo \"Run these commands to correct the problem:\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git rebase --whitespace=fix ${goodparent}\\`\" >>$GITHUB_STEP_SUMMARY\n+          echo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>$GITHUB_STEP_SUMMARY\n           echo \"1. \\`git push --force\\`\" >>$GITHUB_STEP_SUMMARY\n           echo \" \" >>$GITHUB_STEP_SUMMARY\n           echo \"Errors:\" >>$GITHUB_STEP_SUMMARY\n-- \n2.45.0\n\n"},{"id":"493941","messageId":"20240502193840.105355-4-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v2 3/5] ci: separate whitespace check script","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:37Z","receivedAt":"2024-05-02T19:41:21Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The `check-whitespace` CI job is only available as a GitHub action. To\nhelp enable this job with other CI providers, first separate the logic\nperforming the whitespace check into its own script. In subsequent\ncommits, this script is further generalized allowing its reuse.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .github/workflows/check-whitespace.yml | 68 ++---------------------\n ci/check-whitespace.sh                 | 74 ++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 64 deletions(-)\n create mode 100755 ci/check-whitespace.sh\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex a3a6913ecc..d0a78fc426 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -26,67 +26,7 @@ jobs:\n     - name: git log --check\n       id: check_out\n       run: |\n-        baseSha=${{github.event.pull_request.base.sha}}\n-        problems=()\n-        commit=\n-        commitText=\n-        commitTextmd=\n-        goodParent=\n-        while read dash sha etc\n-        do\n-          case \"${dash}\" in\n-          \"---\") # Line contains commit information.\n-            if test -z \"${goodParent}\"\n-            then\n-              # Assume the commit has no whitespace errors until detected otherwise.\n-              goodParent=${sha}\n-            fi\n-            commit=\"${sha}\"\n-            commitText=\"${sha} ${etc}\"\n-            commitTextmd=\"[${sha}](https://github.com/${{ github.repository }}/commit/${sha}) ${etc}\"\n-            ;;\n-          \"\")\n-            ;;\n-          *) # Line contains whitespace error information for current commit.\n-            if test -n \"${goodParent}\"\n-            then\n-              problems+=(\"1) --- ${commitTextmd}\")\n-              echo \"\"\n-              echo \"--- ${commitText}\"\n-              goodParent=\n-            fi\n-            case \"${dash}\" in\n-            *:[1-9]*:) # contains file and line number information\n-              dashend=${dash#*:}\n-              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n-              ;;\n-            *)\n-              problems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n-              ;;\n-            esac\n-            echo \"${dash} ${sha} ${etc}\"\n-            ;;\n-          esac\n-        done <<< $(git log --check --pretty=format:\"---% h% s\" ${baseSha}..)\n-\n-        if test ${#problems[*]} -gt 0\n-        then\n-          if test -z \"${goodParent}\"\n-          then\n-            goodParent=${baseSha: 0:7}\n-          fi\n-          echo \"🛑 Please review the Summary output for further information.\"\n-          echo \"### :x: A whitespace issue was found in one or more of the commits.\" >$GITHUB_STEP_SUMMARY\n-          echo \"\" >>$GITHUB_STEP_SUMMARY\n-          echo \"Run these commands to correct the problem:\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git push --force\\`\" >>$GITHUB_STEP_SUMMARY\n-          echo \" \" >>$GITHUB_STEP_SUMMARY\n-          echo \"Errors:\" >>$GITHUB_STEP_SUMMARY\n-          for i in \"${problems[@]}\"\n-          do\n-            echo \"${i}\" >>$GITHUB_STEP_SUMMARY\n-          done\n-\n-          exit 2\n-        fi\n+        ./ci/check-whitespace.sh \\\n+          \"${{github.event.pull_request.base.sha}}\" \\\n+          \"$GITHUB_STEP_SUMMARY\" \\\n+          \"https://github.com/${{github.repository}}\"\ndiff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\nnew file mode 100755\nindex 0000000000..f57d1ff5f0\n--- /dev/null\n+++ b/ci/check-whitespace.sh\n@@ -0,0 +1,74 @@\n+#!/bin/bash\n+\n+baseCommit=$1\n+outputFile=$2\n+url=$3\n+\n+problems=()\n+commit=\n+commitText=\n+commitTextmd=\n+goodParent=\n+\n+while read dash sha etc\n+do\n+\tcase \"${dash}\" in\n+\t\"---\") # Line contains commit information.\n+\t\tif test -z \"${goodParent}\"\n+\t\tthen\n+\t\t\t# Assume the commit has no whitespace errors until detected otherwise.\n+\t\t\tgoodParent=${sha}\n+\t\tfi\n+\n+\t\tcommit=\"${sha}\"\n+\t\tcommitText=\"${sha} ${etc}\"\n+\t\tcommitTextmd=\"[${sha}](${url}/commit/${sha}) ${etc}\"\n+\t\t;;\n+\t\"\")\n+\t\t;;\n+\t*) # Line contains whitespace error information for current commit.\n+\t\tif test -n \"${goodParent}\"\n+\t\tthen\n+\t\t\tproblems+=(\"1) --- ${commitTextmd}\")\n+\t\t\techo \"\"\n+\t\t\techo \"--- ${commitText}\"\n+\t\t\tgoodParent=\n+\t\tfi\n+\n+\t\tcase \"${dash}\" in\n+\t\t*:[1-9]*:) # contains file and line number information\n+\t\t\tdashend=${dash#*:}\n+\t\t\tproblems+=(\"[${dash}](${url}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n+\t\t\t;;\n+\t\t*)\n+\t\t\tproblems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n+\t\t\t;;\n+\t\tesac\n+\t\techo \"${dash} ${sha} ${etc}\"\n+\t\t;;\n+\tesac\n+done <<< \"$(git log --check --pretty=format:\"---% h% s\" \"${baseCommit}\"..)\"\n+\n+if test ${#problems[*]} -gt 0\n+then\n+\tif test -z \"${goodParent}\"\n+\tthen\n+\t\tgoodParent=${baseCommit: 0:7}\n+\tfi\n+\n+\techo \"🛑 Please review the Summary output for further information.\"\n+\techo \"### :x: A whitespace issue was found in one or more of the commits.\" >\"$outputFile\"\n+\techo \"\" >>\"$outputFile\"\n+\techo \"Run these commands to correct the problem:\" >>\"$outputFile\"\n+\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>\"$outputFile\"\n+\techo \"1. \\`git push --force\\`\" >>\"$outputFile\"\n+\techo \" \" >>\"$outputFile\"\n+\techo \"Errors:\" >>\"$outputFile\"\n+\n+\tfor i in \"${problems[@]}\"\n+\tdo\n+\t\techo \"${i}\" >>\"$outputFile\"\n+\tdone\n+\n+\texit 2\n+fi\n-- \n2.45.0\n\n"},{"id":"493942","messageId":"20240502193840.105355-5-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v2 4/5] ci: make the whitespace report optional","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:38Z","receivedAt":"2024-05-02T19:41:30Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The `check-whitespace` CI job generates a formatted output file\ncontaining whitespace error information. As not all CI providers support\nrendering a formatted summary, make its generation optional.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------\n 1 file changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\nindex f57d1ff5f0..fabd6ecde5 100755\n--- a/ci/check-whitespace.sh\n+++ b/ci/check-whitespace.sh\n@@ -1,9 +1,20 @@\n #!/bin/bash\n+#\n+# Check that commits after a specified point do not contain new or modified\n+# lines with whitespace errors. An optional formatted summary can be generated\n+# by providing an output file path and url as additional arguments.\n+#\n \n baseCommit=$1\n outputFile=$2\n url=$3\n \n+if test \"$#\" -eq 0 || test \"$#\" -gt 3\n+then\n+\techo \"USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]\"\n+\texit 1\n+fi\n+\n problems=()\n commit=\n commitText=\n@@ -56,19 +67,29 @@ then\n \t\tgoodParent=${baseCommit: 0:7}\n \tfi\n \n-\techo \"🛑 Please review the Summary output for further information.\"\n-\techo \"### :x: A whitespace issue was found in one or more of the commits.\" >\"$outputFile\"\n-\techo \"\" >>\"$outputFile\"\n-\techo \"Run these commands to correct the problem:\" >>\"$outputFile\"\n-\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>\"$outputFile\"\n-\techo \"1. \\`git push --force\\`\" >>\"$outputFile\"\n-\techo \" \" >>\"$outputFile\"\n-\techo \"Errors:\" >>\"$outputFile\"\n+\techo \"A whitespace issue was found in onen of more of the commits.\"\n+\techo \"Run the following command to resolve whitespace issues:\"\n+\techo \"git rebase --whitespace=fix ${goodParent}\"\n+\n+\t# If target output file is provided, write formatted ouput.\n+\tif test -n \"$outputFile\"\n+\tthen\n+\t\techo \"🛑 Please review the Summary output for further information.\"\n+\t\t(\n+\t\t\techo \"### :x: A whitespace issue was found in one or more of the commits.\"\n+\t\t\techo \"\"\n+\t\t\techo \"Run these commands to correct the problem:\"\n+\t\t\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\"\n+\t\t\techo \"1. \\`git push --force\\`\"\n+\t\t\techo \"\"\n+\t\t\techo \"Errors:\"\n \n-\tfor i in \"${problems[@]}\"\n-\tdo\n-\t\techo \"${i}\" >>\"$outputFile\"\n-\tdone\n+\t\t\tfor i in \"${problems[@]}\"\n+\t\t\tdo\n+\t\t\t\techo \"${i}\"\n+\t\t\tdone\n+\t\t) >\"$outputFile\"\n+\tfi\n \n \texit 2\n fi\n-- \n2.45.0\n\n"},{"id":"493943","messageId":"20240502193840.105355-6-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v2 5/5] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-02T19:38:39Z","receivedAt":"2024-05-02T19:41:33Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"GitLab CI does not have a job to check for whitespace errors introduced\nby a set of changes. Reuse the existing generic `whitespace-check.sh` to\ncreate the job for GitLab pipelines.\n\nNote that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\navailable in GitLab merge request pipelines and therefore the CI job is\nconfigured to only run as part of those pipelines.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .gitlab-ci.yml | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\nindex c0fa2fe90b..6d046ce409 100644\n--- a/.gitlab-ci.yml\n+++ b/.gitlab-ci.yml\n@@ -102,3 +102,12 @@ static-analysis:\n   script:\n     - ./ci/run-static-analysis.sh\n     - ./ci/check-directional-formatting.bash\n+\n+check-whitespace:\n+  image: ubuntu:latest\n+  before_script:\n+    - ./ci/install-docker-dependencies.sh\n+  script:\n+    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n+  rules:\n+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n-- \n2.45.0\n\n"},{"id":"493952","messageId":"xmqqzft7c2gb.fsf@gitster.g","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"Re: [PATCH v2 0/5] Add GitLab CI to check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-02T21:45:56Z","receivedAt":"2024-05-02T21:46:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> This is the second version of my patch series to add a GitLab CI job to\n> check for whitespace errors. The main differnece with this version is\n> that it first generalizes the existing GitHub whitespace check CI job\n> allowing the GitLab one to reuse it.\n\nWill queue.  Thanks.  The extraction and reuse of a common script is\nexcellent.\n"},{"id":"493964","messageId":"ZjSKqaJxd55328Fn@tanuki","threadId":"61395","inReplyTo":"20240502193840.105355-4-jltobler@gmail.com","subject":"Re: [PATCH v2 3/5] ci: separate whitespace check script","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-03T06:56:41Z","receivedAt":"2024-05-03T06:56:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:\n> The `check-whitespace` CI job is only available as a GitHub action. To\n> help enable this job with other CI providers, first separate the logic\n> performing the whitespace check into its own script. In subsequent\n> commits, this script is further generalized allowing its reuse.\n> \n> Helped-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n[snip]\n> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> new file mode 100755\n> index 0000000000..f57d1ff5f0\n> --- /dev/null\n> +++ b/ci/check-whitespace.sh\n> @@ -0,0 +1,74 @@\n> +#!/bin/bash\n\nThis needs to be either \"/bin/sh\" or \"/usr/bin/env bash\".\n\n> +baseCommit=$1\n> +outputFile=$2\n\nI think the script would be more flexible if we just didn't have an\noutput file in the first place and handle redirection of the output at\nthe caller's side. That for example also allows you to easily append to\na potential output file.\n\nEdit: I see you change this in the next patch, so this is okay.\n\n> +url=$3\n\nWe should check that we got 3 arguments in the first place.\n\nEdit: I see that you add those checks in the next commit, but it does\nleave the reader wondering at this point. Maybe we should have a strict\ncheck here and then loosen it in the next commit where you make it\noptional.\n\nPatrick\n"},{"id":"493965","messageId":"ZjSKsGooraoQDU2I@tanuki","threadId":"61395","inReplyTo":"20240502193840.105355-5-jltobler@gmail.com","subject":"Re: [PATCH v2 4/5] ci: make the whitespace report optional","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-03T06:56:48Z","receivedAt":"2024-05-03T06:56:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 02, 2024 at 02:38:38PM -0500, Justin Tobler wrote:\n> The `check-whitespace` CI job generates a formatted output file\n> containing whitespace error information. As not all CI providers support\n> rendering a formatted summary, make its generation optional.\n> \n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n>  ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------\n>  1 file changed, 33 insertions(+), 12 deletions(-)\n> \n> diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> index f57d1ff5f0..fabd6ecde5 100755\n> --- a/ci/check-whitespace.sh\n> +++ b/ci/check-whitespace.sh\n> @@ -1,9 +1,20 @@\n>  #!/bin/bash\n> +#\n> +# Check that commits after a specified point do not contain new or modified\n> +# lines with whitespace errors. An optional formatted summary can be generated\n> +# by providing an output file path and url as additional arguments.\n> +#\n>  \n>  baseCommit=$1\n>  outputFile=$2\n>  url=$3\n>  \n> +if test \"$#\" -eq 0 || test \"$#\" -gt 3\n\nThat check is wrong, isn't it? Based on the usage below you either\naccept exactly 1 or exactly 3 arguments. But the condition here also\naccepts 2 arguments just fine. The following may be a bit easier to\nfollow as it is more explicit:\n\n    if test \"$#\" -ne 2 && test \"$#\" -ne 3\n    then\n        ...\n    fi\n\n> +then\n> +\techo \"USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]\"\n> +\texit 1\n> +fi\n\nAh, you make the output file optional here. Fair enough, then you can\nscratch that comment from my preceding mail as it did serve a purpose.\n\nPatrick\n"},{"id":"493966","messageId":"ZjSKt73ohMkJcpH5@tanuki","threadId":"61395","inReplyTo":"20240502193840.105355-6-jltobler@gmail.com","subject":"Re: [PATCH v2 5/5] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-03T06:56:55Z","receivedAt":"2024-05-03T06:57:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 02, 2024 at 02:38:39PM -0500, Justin Tobler wrote:\n> GitLab CI does not have a job to check for whitespace errors introduced\n> by a set of changes. Reuse the existing generic `whitespace-check.sh` to\n> create the job for GitLab pipelines.\n> \n> Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n> available in GitLab merge request pipelines and therefore the CI job is\n> configured to only run as part of those pipelines.\n> \n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n>  .gitlab-ci.yml | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n> \n> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> index c0fa2fe90b..6d046ce409 100644\n> --- a/.gitlab-ci.yml\n> +++ b/.gitlab-ci.yml\n> @@ -102,3 +102,12 @@ static-analysis:\n>    script:\n>      - ./ci/run-static-analysis.sh\n>      - ./ci/check-directional-formatting.bash\n> +\n> +check-whitespace:\n> +  image: ubuntu:latest\n> +  before_script:\n> +    - ./ci/install-docker-dependencies.sh\n> +  script:\n> +    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n> +  rules:\n> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n\nNice :)\n\nPatrick\n"},{"id":"494006","messageId":"awbex2f6p4sqdneevwrsba4gayidhvxrdyqjqffcn7ub5imcmm@t3wfqe5xmytv","threadId":"61395","inReplyTo":"ZjSKqaJxd55328Fn@tanuki","subject":"Re: [PATCH v2 3/5] ci: separate whitespace check script","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T15:27:20Z","receivedAt":"2024-05-03T15:28:38Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/03 08:56AM, Patrick Steinhardt wrote:\n> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:\n> > The `check-whitespace` CI job is only available as a GitHub action. To\n> > help enable this job with other CI providers, first separate the logic\n> > performing the whitespace check into its own script. In subsequent\n> > commits, this script is further generalized allowing its reuse.\n> > \n> > Helped-by: Patrick Steinhardt <ps@pks.im>\n> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> > ---\n> [snip]\n> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> > new file mode 100755\n> > index 0000000000..f57d1ff5f0\n> > --- /dev/null\n> > +++ b/ci/check-whitespace.sh\n> > @@ -0,0 +1,74 @@\n> > +#!/bin/bash\n> \n> This needs to be either \"/bin/sh\" or \"/usr/bin/env bash\".\n\nSince the script is using some shell specific features, I'll update this\nto \"/usr/bin/env bash\" in the next version.\n\n> \n> > +baseCommit=$1\n> > +outputFile=$2\n> \n> I think the script would be more flexible if we just didn't have an\n> output file in the first place and handle redirection of the output at\n> the caller's side. That for example also allows you to easily append to\n> a potential output file.\n> \n> Edit: I see you change this in the next patch, so this is okay.\n> \n> > +url=$3\n> \n> We should check that we got 3 arguments in the first place.\n> \n> Edit: I see that you add those checks in the next commit, but it does\n> leave the reader wondering at this point. Maybe we should have a strict\n> check here and then loosen it in the next commit where you make it\n> optional.\n\nFor this patch specifically, I was trying to really only factor out the\nwhitespace check into its own script and keep changes outside of that to\na minimum. The next patch focuses on all the actual script changes and I\nwas hoping it might be easier to review that way. :)\n\n-Justin\n\n"},{"id":"494008","messageId":"3xwa3bvda4ccctnaqerq45qsmrzq6hf3fqltzns2rcblyvnifb@ycoui7hsbkxr","threadId":"61395","inReplyTo":"ZjSKsGooraoQDU2I@tanuki","subject":"Re: [PATCH v2 4/5] ci: make the whitespace report optional","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T15:35:57Z","receivedAt":"2024-05-03T15:37:15Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/03 08:56AM, Patrick Steinhardt wrote:\n> On Thu, May 02, 2024 at 02:38:38PM -0500, Justin Tobler wrote:\n> > The `check-whitespace` CI job generates a formatted output file\n> > containing whitespace error information. As not all CI providers support\n> > rendering a formatted summary, make its generation optional.\n> > \n> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> > ---\n> >  ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------\n> >  1 file changed, 33 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> > index f57d1ff5f0..fabd6ecde5 100755\n> > --- a/ci/check-whitespace.sh\n> > +++ b/ci/check-whitespace.sh\n> > @@ -1,9 +1,20 @@\n> >  #!/bin/bash\n> > +#\n> > +# Check that commits after a specified point do not contain new or modified\n> > +# lines with whitespace errors. An optional formatted summary can be generated\n> > +# by providing an output file path and url as additional arguments.\n> > +#\n> >  \n> >  baseCommit=$1\n> >  outputFile=$2\n> >  url=$3\n> >  \n> > +if test \"$#\" -eq 0 || test \"$#\" -gt 3\n> \n> That check is wrong, isn't it? Based on the usage below you either\n> accept exactly 1 or exactly 3 arguments. But the condition here also\n> accepts 2 arguments just fine. The following may be a bit easier to\n> follow as it is more explicit:\n> \n>     if test \"$#\" -ne 2 && test \"$#\" -ne 3\n>     then\n>         ...\n>     fi\n\nYa, good point. We should only accept 1 or 3 arguments as valid options.\nI'll update this. Thanks!\n\n-Justin\n\n> > +then\n> > +\techo \"USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]\"\n> > +\texit 1\n> > +fi\n> \n> Ah, you make the output file optional here. Fair enough, then you can\n> scratch that comment from my preceding mail as it did serve a purpose.\n\n\n\n"},{"id":"494009","messageId":"otau2qghdji5cdzon57devetu4doqtzxsqhnttpdzde2lg2l5s@bfvjgeipdbhu","threadId":"61395","inReplyTo":"xmqqzft7c2gb.fsf@gitster.g","subject":"Re: [PATCH v2 0/5] Add GitLab CI to check for whitespace errors","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T15:39:19Z","receivedAt":"2024-05-03T15:40:37Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/02 02:45PM, Junio C Hamano wrote:\n> Justin Tobler <jltobler@gmail.com> writes:\n> \n> > This is the second version of my patch series to add a GitLab CI job to\n> > check for whitespace errors. The main differnece with this version is\n> > that it first generalizes the existing GitHub whitespace check CI job\n> > allowing the GitLab one to reuse it.\n> \n> Will queue.  Thanks.  The extraction and reuse of a common script is\n> excellent.\n\nThanks! I will send a V3 here shortly which makes some small suggested\nedits.\n\n-Justin\n"},{"id":"494017","messageId":"xmqq1q6ialiu.fsf@gitster.g","threadId":"61395","inReplyTo":"awbex2f6p4sqdneevwrsba4gayidhvxrdyqjqffcn7ub5imcmm@t3wfqe5xmytv","subject":"Re: [PATCH v2 3/5] ci: separate whitespace check script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T16:49:13Z","receivedAt":"2024-05-03T16:49:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> On 24/05/03 08:56AM, Patrick Steinhardt wrote:\n>> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:\n>> > The `check-whitespace` CI job is only available as a GitHub action. To\n>> > help enable this job with other CI providers, first separate the logic\n>> > performing the whitespace check into its own script. In subsequent\n>> > commits, this script is further generalized allowing its reuse.\n>> > \n>> > Helped-by: Patrick Steinhardt <ps@pks.im>\n>> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n>> > ---\n>> [snip]\n>> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n>> > new file mode 100755\n>> > index 0000000000..f57d1ff5f0\n>> > --- /dev/null\n>> > +++ b/ci/check-whitespace.sh\n>> > @@ -0,0 +1,74 @@\n>> > +#!/bin/bash\n>> \n>> This needs to be either \"/bin/sh\" or \"/usr/bin/env bash\".\n>\n> Since the script is using some shell specific features, I'll update this\n> to \"/usr/bin/env bash\" in the next version.\n\nThis is a question to Patrick, but what makes it bad to assume\n\"bash\" is in \"/bin\" when it is OK to assume that \"env\" is always in\n\"/usr/bin\"?\n\nAll other comments by Patrick I found very sensible.\n\nThanks, both of you.\n"},{"id":"494019","messageId":"20240503172110.181326-1-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240502193840.105355-1-jltobler@gmail.com","subject":"[PATCH v3 0/5] Add GitLab CI to check for whitespace errors","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:02Z","receivedAt":"2024-05-03T17:23:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Hello again,\n\nThis is the third version of my patch series to add a GitLab CI job to\ncheck for whitespace errors.\n\nChanges since V2:\n\n- In 35e293e6 (Merge branch 'ps/ci-test-with-jgit' into next,\n  2024-05-01), `install-docker-dependencies.sh` script was merged into \n  `install-dependencies.sh` and removed. Thus this patch series now \n  depends on that change and is updated to use the other script.\n- Changed the shebang to \"#!/usr/bin/env bash\".\n- Made the argument count validation more strict.\n- Fixed a typo.\n\nGitHub CI:\n\n- https://github.com/gitgitgadget/git/actions/runs/8941666020?pr=1727\n- https://github.com/gitgitgadget/git/actions/runs/8941721282?pr=1727\n\nGitLab CI:\n\n- https://gitlab.com/gitlab-org/git/-/jobs/6776032414\n- https://gitlab.com/gitlab-org/git/-/jobs/6776038234\n\nThanks for the reviews,\n-Justin\n\nJustin Tobler (5):\n  ci: pre-collapse GitLab CI sections\n  github-ci: fix link to whitespace error\n  ci: separate whitespace check script\n  ci: make the whitespace report optional\n  gitlab-ci: add whitespace error check\n\n .github/workflows/check-whitespace.yml | 67 ++----------------\n .gitlab-ci.yml                         |  9 +++\n ci/check-whitespace.sh                 | 95 ++++++++++++++++++++++++++\n ci/lib.sh                              |  2 +-\n 4 files changed, 109 insertions(+), 64 deletions(-)\n create mode 100755 ci/check-whitespace.sh\n\nRange-diff against v2:\n1:  924d3eb23c = 1:  924d3eb23c ci: pre-collapse GitLab CI sections\n2:  c8d8b444dc = 2:  c8d8b444dc github-ci: fix link to whitespace error\n3:  6b44b21dda ! 3:  933e0c33f9 ci: separate whitespace check script\n    @@ .github/workflows/check-whitespace.yml: jobs:\n     \n      ## ci/check-whitespace.sh (new) ##\n     @@\n    -+#!/bin/bash\n    ++#!/usr/bin/env bash\n     +\n     +baseCommit=$1\n     +outputFile=$2\n4:  87dfd1d5a9 ! 4:  374735c60f ci: make the whitespace report optional\n    @@ Commit message\n     \n      ## ci/check-whitespace.sh ##\n     @@\n    - #!/bin/bash\n    + #!/usr/bin/env bash\n     +#\n     +# Check that commits after a specified point do not contain new or modified\n     +# lines with whitespace errors. An optional formatted summary can be generated\n    @@ ci/check-whitespace.sh\n      outputFile=$2\n      url=$3\n      \n    -+if test \"$#\" -eq 0 || test \"$#\" -gt 3\n    ++if test \"$#\" -ne 1 && test \"$#\" -ne 3\n     +then\n     +\techo \"USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]\"\n     +\texit 1\n    @@ ci/check-whitespace.sh: then\n     +\techo \"Run the following command to resolve whitespace issues:\"\n     +\techo \"git rebase --whitespace=fix ${goodParent}\"\n     +\n    -+\t# If target output file is provided, write formatted ouput.\n    ++\t# If target output file is provided, write formatted output.\n     +\tif test -n \"$outputFile\"\n     +\tthen\n     +\t\techo \"🛑 Please review the Summary output for further information.\"\n5:  175b300e91 ! 5:  a7deaeadc7 gitlab-ci: add whitespace error check\n    @@ .gitlab-ci.yml: static-analysis:\n     +check-whitespace:\n     +  image: ubuntu:latest\n     +  before_script:\n    -+    - ./ci/install-docker-dependencies.sh\n    ++    - ./ci/install-dependencies.sh\n     +  script:\n     +    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n     +  rules:\n-- \n2.45.0\n\n"},{"id":"494020","messageId":"20240503172110.181326-2-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240503172110.181326-1-jltobler@gmail.com","subject":"[PATCH v3 1/5] ci: pre-collapse GitLab CI sections","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:03Z","receivedAt":"2024-05-03T17:23:17Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Sections of CI output defined by `begin_group()` and `end_group()` are\nexpanded in GitLab pipelines by default. This can make CI job output\nrather noisy and harder to navigate. Update the behavior for GitLab\npipelines to now collapse sections by default.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n ci/lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 0a73fc7bd1..02e5e058dd 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -18,7 +18,7 @@ elif test true = \"$GITLAB_CI\"\n then\n \tbegin_group () {\n \t\tneed_to_end_group=t\n-\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)\\r\\e[0K$1\\n\"\n+\t\tprintf \"\\e[0Ksection_start:$(date +%s):$(echo \"$1\" | tr ' ' _)[collapsed=true]\\r\\e[0K$1\\n\"\n \t\ttrap \"end_group '$1'\" EXIT\n \t\tset -x\n \t}\n-- \n2.45.0\n\n"},{"id":"494021","messageId":"20240503172110.181326-3-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240503172110.181326-1-jltobler@gmail.com","subject":"[PATCH v3 2/5] github-ci: fix link to whitespace error","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:04Z","receivedAt":"2024-05-03T17:23:17Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When the `check-whitespace` CI job detects whitespace errors, a\nformatted summary of the issue is generated. This summary contains links\nto the commits and blobs responsible for the whitespace errors. The\ngenerated links for blobs do not work and result in a 404.\n\nInstead of using the reference name in the link, use the commit ID\ndirectly. This fixes the broken link and also helps enable future\ngeneralization of the script for other CI providers by removing one of\nthe GitHub specific CI variables used.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .github/workflows/check-whitespace.yml | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex a241a63428..a3a6913ecc 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -31,14 +31,15 @@ jobs:\n         commit=\n         commitText=\n         commitTextmd=\n-        goodparent=\n+        goodParent=\n         while read dash sha etc\n         do\n           case \"${dash}\" in\n-          \"---\")\n-            if test -z \"${commit}\"\n+          \"---\") # Line contains commit information.\n+            if test -z \"${goodParent}\"\n             then\n-              goodparent=${sha}\n+              # Assume the commit has no whitespace errors until detected otherwise.\n+              goodParent=${sha}\n             fi\n             commit=\"${sha}\"\n             commitText=\"${sha} ${etc}\"\n@@ -46,18 +47,18 @@ jobs:\n             ;;\n           \"\")\n             ;;\n-          *)\n-            if test -n \"${commit}\"\n+          *) # Line contains whitespace error information for current commit.\n+            if test -n \"${goodParent}\"\n             then\n               problems+=(\"1) --- ${commitTextmd}\")\n               echo \"\"\n               echo \"--- ${commitText}\"\n-              commit=\n+              goodParent=\n             fi\n             case \"${dash}\" in\n             *:[1-9]*:) # contains file and line number information\n               dashend=${dash#*:}\n-              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${{github.event.pull_request.head.ref}}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n+              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n               ;;\n             *)\n               problems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n@@ -70,15 +71,15 @@ jobs:\n \n         if test ${#problems[*]} -gt 0\n         then\n-          if test -z \"${commit}\"\n+          if test -z \"${goodParent}\"\n           then\n-            goodparent=${baseSha: 0:7}\n+            goodParent=${baseSha: 0:7}\n           fi\n           echo \"🛑 Please review the Summary output for further information.\"\n           echo \"### :x: A whitespace issue was found in one or more of the commits.\" >$GITHUB_STEP_SUMMARY\n           echo \"\" >>$GITHUB_STEP_SUMMARY\n           echo \"Run these commands to correct the problem:\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git rebase --whitespace=fix ${goodparent}\\`\" >>$GITHUB_STEP_SUMMARY\n+          echo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>$GITHUB_STEP_SUMMARY\n           echo \"1. \\`git push --force\\`\" >>$GITHUB_STEP_SUMMARY\n           echo \" \" >>$GITHUB_STEP_SUMMARY\n           echo \"Errors:\" >>$GITHUB_STEP_SUMMARY\n-- \n2.45.0\n\n"},{"id":"494022","messageId":"20240503172110.181326-4-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240503172110.181326-1-jltobler@gmail.com","subject":"[PATCH v3 3/5] ci: separate whitespace check script","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:05Z","receivedAt":"2024-05-03T17:23:18Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The `check-whitespace` CI job is only available as a GitHub action. To\nhelp enable this job with other CI providers, first separate the logic\nperforming the whitespace check into its own script. In subsequent\ncommits, this script is further generalized allowing its reuse.\n\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .github/workflows/check-whitespace.yml | 68 ++---------------------\n ci/check-whitespace.sh                 | 74 ++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 64 deletions(-)\n create mode 100755 ci/check-whitespace.sh\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex a3a6913ecc..d0a78fc426 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -26,67 +26,7 @@ jobs:\n     - name: git log --check\n       id: check_out\n       run: |\n-        baseSha=${{github.event.pull_request.base.sha}}\n-        problems=()\n-        commit=\n-        commitText=\n-        commitTextmd=\n-        goodParent=\n-        while read dash sha etc\n-        do\n-          case \"${dash}\" in\n-          \"---\") # Line contains commit information.\n-            if test -z \"${goodParent}\"\n-            then\n-              # Assume the commit has no whitespace errors until detected otherwise.\n-              goodParent=${sha}\n-            fi\n-            commit=\"${sha}\"\n-            commitText=\"${sha} ${etc}\"\n-            commitTextmd=\"[${sha}](https://github.com/${{ github.repository }}/commit/${sha}) ${etc}\"\n-            ;;\n-          \"\")\n-            ;;\n-          *) # Line contains whitespace error information for current commit.\n-            if test -n \"${goodParent}\"\n-            then\n-              problems+=(\"1) --- ${commitTextmd}\")\n-              echo \"\"\n-              echo \"--- ${commitText}\"\n-              goodParent=\n-            fi\n-            case \"${dash}\" in\n-            *:[1-9]*:) # contains file and line number information\n-              dashend=${dash#*:}\n-              problems+=(\"[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n-              ;;\n-            *)\n-              problems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n-              ;;\n-            esac\n-            echo \"${dash} ${sha} ${etc}\"\n-            ;;\n-          esac\n-        done <<< $(git log --check --pretty=format:\"---% h% s\" ${baseSha}..)\n-\n-        if test ${#problems[*]} -gt 0\n-        then\n-          if test -z \"${goodParent}\"\n-          then\n-            goodParent=${baseSha: 0:7}\n-          fi\n-          echo \"🛑 Please review the Summary output for further information.\"\n-          echo \"### :x: A whitespace issue was found in one or more of the commits.\" >$GITHUB_STEP_SUMMARY\n-          echo \"\" >>$GITHUB_STEP_SUMMARY\n-          echo \"Run these commands to correct the problem:\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>$GITHUB_STEP_SUMMARY\n-          echo \"1. \\`git push --force\\`\" >>$GITHUB_STEP_SUMMARY\n-          echo \" \" >>$GITHUB_STEP_SUMMARY\n-          echo \"Errors:\" >>$GITHUB_STEP_SUMMARY\n-          for i in \"${problems[@]}\"\n-          do\n-            echo \"${i}\" >>$GITHUB_STEP_SUMMARY\n-          done\n-\n-          exit 2\n-        fi\n+        ./ci/check-whitespace.sh \\\n+          \"${{github.event.pull_request.base.sha}}\" \\\n+          \"$GITHUB_STEP_SUMMARY\" \\\n+          \"https://github.com/${{github.repository}}\"\ndiff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\nnew file mode 100755\nindex 0000000000..9cc496da40\n--- /dev/null\n+++ b/ci/check-whitespace.sh\n@@ -0,0 +1,74 @@\n+#!/usr/bin/env bash\n+\n+baseCommit=$1\n+outputFile=$2\n+url=$3\n+\n+problems=()\n+commit=\n+commitText=\n+commitTextmd=\n+goodParent=\n+\n+while read dash sha etc\n+do\n+\tcase \"${dash}\" in\n+\t\"---\") # Line contains commit information.\n+\t\tif test -z \"${goodParent}\"\n+\t\tthen\n+\t\t\t# Assume the commit has no whitespace errors until detected otherwise.\n+\t\t\tgoodParent=${sha}\n+\t\tfi\n+\n+\t\tcommit=\"${sha}\"\n+\t\tcommitText=\"${sha} ${etc}\"\n+\t\tcommitTextmd=\"[${sha}](${url}/commit/${sha}) ${etc}\"\n+\t\t;;\n+\t\"\")\n+\t\t;;\n+\t*) # Line contains whitespace error information for current commit.\n+\t\tif test -n \"${goodParent}\"\n+\t\tthen\n+\t\t\tproblems+=(\"1) --- ${commitTextmd}\")\n+\t\t\techo \"\"\n+\t\t\techo \"--- ${commitText}\"\n+\t\t\tgoodParent=\n+\t\tfi\n+\n+\t\tcase \"${dash}\" in\n+\t\t*:[1-9]*:) # contains file and line number information\n+\t\t\tdashend=${dash#*:}\n+\t\t\tproblems+=(\"[${dash}](${url}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}\")\n+\t\t\t;;\n+\t\t*)\n+\t\t\tproblems+=(\"\\`${dash} ${sha} ${etc}\\`\")\n+\t\t\t;;\n+\t\tesac\n+\t\techo \"${dash} ${sha} ${etc}\"\n+\t\t;;\n+\tesac\n+done <<< \"$(git log --check --pretty=format:\"---% h% s\" \"${baseCommit}\"..)\"\n+\n+if test ${#problems[*]} -gt 0\n+then\n+\tif test -z \"${goodParent}\"\n+\tthen\n+\t\tgoodParent=${baseCommit: 0:7}\n+\tfi\n+\n+\techo \"🛑 Please review the Summary output for further information.\"\n+\techo \"### :x: A whitespace issue was found in one or more of the commits.\" >\"$outputFile\"\n+\techo \"\" >>\"$outputFile\"\n+\techo \"Run these commands to correct the problem:\" >>\"$outputFile\"\n+\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>\"$outputFile\"\n+\techo \"1. \\`git push --force\\`\" >>\"$outputFile\"\n+\techo \" \" >>\"$outputFile\"\n+\techo \"Errors:\" >>\"$outputFile\"\n+\n+\tfor i in \"${problems[@]}\"\n+\tdo\n+\t\techo \"${i}\" >>\"$outputFile\"\n+\tdone\n+\n+\texit 2\n+fi\n-- \n2.45.0\n\n"},{"id":"494023","messageId":"20240503172110.181326-5-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240503172110.181326-1-jltobler@gmail.com","subject":"[PATCH v3 4/5] ci: make the whitespace report optional","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:06Z","receivedAt":"2024-05-03T17:23:19Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The `check-whitespace` CI job generates a formatted output file\ncontaining whitespace error information. As not all CI providers support\nrendering a formatted summary, make its generation optional.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n ci/check-whitespace.sh | 45 +++++++++++++++++++++++++++++++-----------\n 1 file changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\nindex 9cc496da40..db399097a5 100755\n--- a/ci/check-whitespace.sh\n+++ b/ci/check-whitespace.sh\n@@ -1,9 +1,20 @@\n #!/usr/bin/env bash\n+#\n+# Check that commits after a specified point do not contain new or modified\n+# lines with whitespace errors. An optional formatted summary can be generated\n+# by providing an output file path and url as additional arguments.\n+#\n \n baseCommit=$1\n outputFile=$2\n url=$3\n \n+if test \"$#\" -ne 1 && test \"$#\" -ne 3\n+then\n+\techo \"USAGE: $0 <BASE_COMMIT> [<OUTPUT_FILE> <URL>]\"\n+\texit 1\n+fi\n+\n problems=()\n commit=\n commitText=\n@@ -56,19 +67,29 @@ then\n \t\tgoodParent=${baseCommit: 0:7}\n \tfi\n \n-\techo \"🛑 Please review the Summary output for further information.\"\n-\techo \"### :x: A whitespace issue was found in one or more of the commits.\" >\"$outputFile\"\n-\techo \"\" >>\"$outputFile\"\n-\techo \"Run these commands to correct the problem:\" >>\"$outputFile\"\n-\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\" >>\"$outputFile\"\n-\techo \"1. \\`git push --force\\`\" >>\"$outputFile\"\n-\techo \" \" >>\"$outputFile\"\n-\techo \"Errors:\" >>\"$outputFile\"\n+\techo \"A whitespace issue was found in onen of more of the commits.\"\n+\techo \"Run the following command to resolve whitespace issues:\"\n+\techo \"git rebase --whitespace=fix ${goodParent}\"\n+\n+\t# If target output file is provided, write formatted output.\n+\tif test -n \"$outputFile\"\n+\tthen\n+\t\techo \"🛑 Please review the Summary output for further information.\"\n+\t\t(\n+\t\t\techo \"### :x: A whitespace issue was found in one or more of the commits.\"\n+\t\t\techo \"\"\n+\t\t\techo \"Run these commands to correct the problem:\"\n+\t\t\techo \"1. \\`git rebase --whitespace=fix ${goodParent}\\`\"\n+\t\t\techo \"1. \\`git push --force\\`\"\n+\t\t\techo \"\"\n+\t\t\techo \"Errors:\"\n \n-\tfor i in \"${problems[@]}\"\n-\tdo\n-\t\techo \"${i}\" >>\"$outputFile\"\n-\tdone\n+\t\t\tfor i in \"${problems[@]}\"\n+\t\t\tdo\n+\t\t\t\techo \"${i}\"\n+\t\t\tdone\n+\t\t) >\"$outputFile\"\n+\tfi\n \n \texit 2\n fi\n-- \n2.45.0\n\n"},{"id":"494024","messageId":"20240503172110.181326-6-jltobler@gmail.com","threadId":"61395","inReplyTo":"20240503172110.181326-1-jltobler@gmail.com","subject":"[PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-03T17:21:07Z","receivedAt":"2024-05-03T17:23:21Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"GitLab CI does not have a job to check for whitespace errors introduced\nby a set of changes. Reuse the existing generic `whitespace-check.sh` to\ncreate the job for GitLab pipelines.\n\nNote that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\navailable in GitLab merge request pipelines and therefore the CI job is\nconfigured to only run as part of those pipelines.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n .gitlab-ci.yml | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\nindex c0fa2fe90b..619bf729fa 100644\n--- a/.gitlab-ci.yml\n+++ b/.gitlab-ci.yml\n@@ -102,3 +102,12 @@ static-analysis:\n   script:\n     - ./ci/run-static-analysis.sh\n     - ./ci/check-directional-formatting.bash\n+\n+check-whitespace:\n+  image: ubuntu:latest\n+  before_script:\n+    - ./ci/install-dependencies.sh\n+  script:\n+    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n+  rules:\n+    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n-- \n2.45.0\n\n"},{"id":"494034","messageId":"ZjUmEuKZB1xwAPo7@ncase","threadId":"61395","inReplyTo":"xmqq1q6ialiu.fsf@gitster.g","subject":"Re: [PATCH v2 3/5] ci: separate whitespace check script","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-03T17:59:46Z","receivedAt":"2024-05-03T17:59:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote:\n> Justin Tobler <jltobler@gmail.com> writes:\n> \n> > On 24/05/03 08:56AM, Patrick Steinhardt wrote:\n> >> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote:\n> >> > The `check-whitespace` CI job is only available as a GitHub action. To\n> >> > help enable this job with other CI providers, first separate the logic\n> >> > performing the whitespace check into its own script. In subsequent\n> >> > commits, this script is further generalized allowing its reuse.\n> >> > \n> >> > Helped-by: Patrick Steinhardt <ps@pks.im>\n> >> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> >> > ---\n> >> [snip]\n> >> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh\n> >> > new file mode 100755\n> >> > index 0000000000..f57d1ff5f0\n> >> > --- /dev/null\n> >> > +++ b/ci/check-whitespace.sh\n> >> > @@ -0,0 +1,74 @@\n> >> > +#!/bin/bash\n> >> \n> >> This needs to be either \"/bin/sh\" or \"/usr/bin/env bash\".\n> >\n> > Since the script is using some shell specific features, I'll update this\n> > to \"/usr/bin/env bash\" in the next version.\n> \n> This is a question to Patrick, but what makes it bad to assume\n> \"bash\" is in \"/bin\" when it is OK to assume that \"env\" is always in\n> \"/usr/bin\"?\n\nMy own bias. I know of systems without \"/bin/bash\" (NixOS), but I don't\nknow of any without \"/usr/bin/env\". But you're right, \"/usr/bin/env\" is\nnot part of POSIX and thus not really portable, either.\n\nUltimately I don't think there is any way to write the shebang so that\nit is fully POSIX compliant. So I'd rather go with the option which is\nsupported on more systems, which is to the best of my knowlede env(1).\n\nPatrick\n"},{"id":"494075","messageId":"CAPx1GveuQMeKt0_ZuY4DN0_tKrLBjXpWLngxYTtbzV=iHYT-QQ@mail.gmail.com","threadId":"61395","inReplyTo":"ZjUmEuKZB1xwAPo7@ncase","subject":"Re: [PATCH v2 3/5] ci: separate whitespace check script","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-05-04T06:51:35Z","receivedAt":"2024-05-04T06:51:49Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Fri, May 3, 2024 at 11:27 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote:\n> > This is a question to Patrick, but what makes it bad to assume\n> > \"bash\" is in \"/bin\" when it is OK to assume that \"env\" is always in\n> > \"/usr/bin\"?\n>\n> My own bias. I know of systems without \"/bin/bash\" (NixOS), but I don't\n> know of any without \"/usr/bin/env\". But you're right, \"/usr/bin/env\" is\n> not part of POSIX and thus not really portable, either.\n>\n> Ultimately I don't think there is any way to write the shebang so that\n> it is fully POSIX compliant. So I'd rather go with the option which is\n> supported on more systems, which is to the best of my knowlede env(1).\n\nThe various BSDs mostly stick bash in /usr/local/bin; some versions\nof macOS did not have a /bin/bash either, as I recall, though my current\nmac-laptop does.\n\nIn any case, #! /usr/bin/env <program> is pretty darn common; it's found\nin a lot of Python scripts, for instance. It works well on old SunOS, on\nthe various BSDs, on macOS, and on Linux, provided of course that\nthe given <program> is installed at all.\n\nThe *most* portable method is generally to use only POSIX /bin/sh\nconstructs, of course. :-)\n\nChris\n"},{"id":"494118","messageId":"Zjh-3dCL2-MnV-PE@tanuki","threadId":"61395","inReplyTo":"20240503172110.181326-6-jltobler@gmail.com","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-06T06:55:25Z","receivedAt":"2024-05-06T07:02:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote:\n> GitLab CI does not have a job to check for whitespace errors introduced\n> by a set of changes. Reuse the existing generic `whitespace-check.sh` to\n> create the job for GitLab pipelines.\n> \n> Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n> available in GitLab merge request pipelines and therefore the CI job is\n> configured to only run as part of those pipelines.\n> \n> Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> ---\n>  .gitlab-ci.yml | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n> \n> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> index c0fa2fe90b..619bf729fa 100644\n> --- a/.gitlab-ci.yml\n> +++ b/.gitlab-ci.yml\n> @@ -102,3 +102,12 @@ static-analysis:\n>    script:\n>      - ./ci/run-static-analysis.sh\n>      - ./ci/check-directional-formatting.bash\n> +\n> +check-whitespace:\n> +  image: ubuntu:latest\n> +  before_script:\n> +    - ./ci/install-dependencies.sh\n\nDo we actually need to install dependencies? I imagine all that's needed\nwould be Git.\n\nOther than this question the patch series looks good to me, thanks!\n\nPatrick\n\n> +  script:\n> +    - ./ci/check-whitespace.sh \"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA\"\n> +  rules:\n> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'\n> -- \n> 2.45.0\n> \n> \n"},{"id":"494159","messageId":"6os32ofwwbxe3r4w7rnrpjkyz2jtob4rcucq75mjsy5gkioowf@euxp74pjq6xj","threadId":"61395","inReplyTo":"Zjh-3dCL2-MnV-PE@tanuki","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-06T13:59:58Z","receivedAt":"2024-05-06T14:01:22Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/06 08:55AM, Patrick Steinhardt wrote:\n> On Fri, May 03, 2024 at 12:21:07PM -0500, Justin Tobler wrote:\n> > GitLab CI does not have a job to check for whitespace errors introduced\n> > by a set of changes. Reuse the existing generic `whitespace-check.sh` to\n> > create the job for GitLab pipelines.\n> > \n> > Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only\n> > available in GitLab merge request pipelines and therefore the CI job is\n> > configured to only run as part of those pipelines.\n> > \n> > Signed-off-by: Justin Tobler <jltobler@gmail.com>\n> > ---\n> >  .gitlab-ci.yml | 9 +++++++++\n> >  1 file changed, 9 insertions(+)\n> > \n> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml\n> > index c0fa2fe90b..619bf729fa 100644\n> > --- a/.gitlab-ci.yml\n> > +++ b/.gitlab-ci.yml\n> > @@ -102,3 +102,12 @@ static-analysis:\n> >    script:\n> >      - ./ci/run-static-analysis.sh\n> >      - ./ci/check-directional-formatting.bash\n> > +\n> > +check-whitespace:\n> > +  image: ubuntu:latest\n> > +  before_script:\n> > +    - ./ci/install-dependencies.sh\n> \n> Do we actually need to install dependencies? I imagine all that's needed\n> would be Git.\n\nYou are correct. Git is really the only dependency we need. If we want\nto remove the need for the install dependencies script, we could fetch\nGit ourselves during the pre-script. Another option could be to use a\ndifferent container image that has Git baked in.\n\n-Justin\n"},{"id":"494168","messageId":"xmqqle4mzwp8.fsf@gitster.g","threadId":"61395","inReplyTo":"Zjh-3dCL2-MnV-PE@tanuki","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T17:17:39Z","receivedAt":"2024-05-06T17:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +check-whitespace:\n>> +  image: ubuntu:latest\n>> +  before_script:\n>> +    - ./ci/install-dependencies.sh\n>\n> Do we actually need to install dependencies? I imagine all that's needed\n> would be Git.\n>\n> Other than this question the patch series looks good to me, thanks!\n\nI am a bit puzzled.  Is the proposal to check our sources with a\npre-built Git (which by definition would be a bit older than what is\nbeing tested)?\n\nNot that I have serious trouble in that direction---I am just trying\nto make sure what is being proposed.\n\nThanks.\n"},{"id":"494183","messageId":"xjp247ji6zjyacniodws6jbdms7tr7726h6spsn7jvx3axqd4a@k6xhzf4v5akc","threadId":"61395","inReplyTo":"xmqqle4mzwp8.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-06T19:21:52Z","receivedAt":"2024-05-06T19:23:13Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/06 10:17AM, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> +check-whitespace:\n> >> +  image: ubuntu:latest\n> >> +  before_script:\n> >> +    - ./ci/install-dependencies.sh\n> >\n> > Do we actually need to install dependencies? I imagine all that's needed\n> > would be Git.\n> >\n> > Other than this question the patch series looks good to me, thanks!\n> \n> I am a bit puzzled.  Is the proposal to check our sources with a\n> pre-built Git (which by definition would be a bit older than what is\n> being tested)?\n\nThe GitLab `check-whitespace` CI job only needs Git to run and uses\n`ci/install-dependencies.sh` to download a pre-built Git package via\n`apt-get` since `ubuntu:latest` is the container image used. The \n`ci/install-dependencies.sh` script also fetches a bunch of other \ndependencies which are not needed. \n\nI think Patrick is proposing, to further simplify, we avoid using\n`ci/install-dependencies.sh` and only fetch Git. Patrick please correct\nme if I misunderstand :)\n\n-Justin\n"},{"id":"494218","messageId":"ZjmqEEB2gXU_nKHz@tanuki","threadId":"61395","inReplyTo":"xjp247ji6zjyacniodws6jbdms7tr7726h6spsn7jvx3axqd4a@k6xhzf4v5akc","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T04:12:00Z","receivedAt":"2024-05-07T04:12:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 06, 2024 at 02:21:52PM -0500, Justin Tobler wrote:\n> On 24/05/06 10:17AM, Junio C Hamano wrote:\n> > Patrick Steinhardt <ps@pks.im> writes:\n> > \n> > >> +check-whitespace:\n> > >> +  image: ubuntu:latest\n> > >> +  before_script:\n> > >> +    - ./ci/install-dependencies.sh\n> > >\n> > > Do we actually need to install dependencies? I imagine all that's needed\n> > > would be Git.\n> > >\n> > > Other than this question the patch series looks good to me, thanks!\n> > \n> > I am a bit puzzled.  Is the proposal to check our sources with a\n> > pre-built Git (which by definition would be a bit older than what is\n> > being tested)?\n> \n> The GitLab `check-whitespace` CI job only needs Git to run and uses\n> `ci/install-dependencies.sh` to download a pre-built Git package via\n> `apt-get` since `ubuntu:latest` is the container image used. The \n> `ci/install-dependencies.sh` script also fetches a bunch of other \n> dependencies which are not needed. \n> \n> I think Patrick is proposing, to further simplify, we avoid using\n> `ci/install-dependencies.sh` and only fetch Git. Patrick please correct\n> me if I misunderstand :)\n\nI just wondered how GitHub Workflows manages without installing any\ndependencies at all. Is Git already part of the default images? If so,\nthere is no need to install anything and we can just execute the script\ndirectly, which saves some time.\n\nIf there is a need to install Git we could either just manually install\nit in the `before_script` or leave it as-is. I don't mind it much either\nway.\n\nPatrick\n"},{"id":"494286","messageId":"lkd7hxcwnepjgyjhz5nl46h2igve2ttdoajtjgnene56qau6gh@dec6ticwjfqu","threadId":"61395","inReplyTo":"ZjmqEEB2gXU_nKHz@tanuki","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-05-07T18:06:04Z","receivedAt":"2024-05-07T18:07:26Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/05/07 06:12AM, Patrick Steinhardt wrote:\n \n> I just wondered how GitHub Workflows manages without installing any\n> dependencies at all. Is Git already part of the default images? If so,\n> there is no need to install anything and we can just execute the script\n> directly, which saves some time.\n\nGit is not bundled by default in the \"ubuntu:latest\" container image. We\nwould have to install it ourselves. As for why GitHub Workflows do not\nhave to install Git, it looks like each runner has a defined set of\nincluded software which happens to include Git.\n\nhttps://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md\n\n> If there is a need to install Git we could either just manually install\n> it in the `before_script` or leave it as-is. I don't mind it much either\n> way.\n\nI don't have strong opinions, but I think I would prefer to leave it\nas-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another\nversion unless there is addional feedback. Thanks\n\n-Justin\n"},{"id":"494287","messageId":"ZjpvE-6xQ-rf5xO_@ncase","threadId":"61395","inReplyTo":"lkd7hxcwnepjgyjhz5nl46h2igve2ttdoajtjgnene56qau6gh@dec6ticwjfqu","subject":"Re: [PATCH v3 5/5] gitlab-ci: add whitespace error check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-07T18:12:35Z","receivedAt":"2024-05-07T18:12:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 07, 2024 at 01:06:04PM -0500, Justin Tobler wrote:\n> On 24/05/07 06:12AM, Patrick Steinhardt wrote:\n>  \n> > I just wondered how GitHub Workflows manages without installing any\n> > dependencies at all. Is Git already part of the default images? If so,\n> > there is no need to install anything and we can just execute the script\n> > directly, which saves some time.\n> \n> Git is not bundled by default in the \"ubuntu:latest\" container image. We\n> would have to install it ourselves. As for why GitHub Workflows do not\n> have to install Git, it looks like each runner has a defined set of\n> included software which happens to include Git.\n\nOkay.\n\n> https://github.com/actions/runner-images/blob/ubuntu22/20240422.1/images/ubuntu/Ubuntu2204-Readme.md\n> \n> > If there is a need to install Git we could either just manually install\n> > it in the `before_script` or leave it as-is. I don't mind it much either\n> > way.\n> \n> I don't have strong opinions, but I think I would prefer to leave it\n> as-is and reuse `ci/install-dependencies.sh`. I'll forgo sending another\n> version unless there is addional feedback. Thanks\n\nFair enough. Let's not overthink this then and get this merged.\n\nPatrick\n"}]}