| Message ID | 20240502193840.105355-4-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add GitLab CI to check for whitespace errors | expand |
On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote: > The `check-whitespace` CI job is only available as a GitHub action. To > help enable this job with other CI providers, first separate the logic > performing the whitespace check into its own script. In subsequent > commits, this script is further generalized allowing its reuse. > > Helped-by: Patrick Steinhardt <ps@pks.im> > Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- [snip] > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > new file mode 100755 > index 0000000000..f57d1ff5f0 > --- /dev/null > +++ b/ci/check-whitespace.sh > @@ -0,0 +1,74 @@ > +#!/bin/bash This needs to be either "/bin/sh" or "/usr/bin/env bash". > +baseCommit=$1 > +outputFile=$2 I think the script would be more flexible if we just didn't have an output file in the first place and handle redirection of the output at the caller's side. That for example also allows you to easily append to a potential output file. Edit: I see you change this in the next patch, so this is okay. > +url=$3 We should check that we got 3 arguments in the first place. Edit: I see that you add those checks in the next commit, but it does leave the reader wondering at this point. Maybe we should have a strict check here and then loosen it in the next commit where you make it optional. Patrick
On 24/05/03 08:56AM, Patrick Steinhardt wrote: > On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote: > > The `check-whitespace` CI job is only available as a GitHub action. To > > help enable this job with other CI providers, first separate the logic > > performing the whitespace check into its own script. In subsequent > > commits, this script is further generalized allowing its reuse. > > > > Helped-by: Patrick Steinhardt <ps@pks.im> > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > > --- > [snip] > > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > > new file mode 100755 > > index 0000000000..f57d1ff5f0 > > --- /dev/null > > +++ b/ci/check-whitespace.sh > > @@ -0,0 +1,74 @@ > > +#!/bin/bash > > This needs to be either "/bin/sh" or "/usr/bin/env bash". Since the script is using some shell specific features, I'll update this to "/usr/bin/env bash" in the next version. > > > +baseCommit=$1 > > +outputFile=$2 > > I think the script would be more flexible if we just didn't have an > output file in the first place and handle redirection of the output at > the caller's side. That for example also allows you to easily append to > a potential output file. > > Edit: I see you change this in the next patch, so this is okay. > > > +url=$3 > > We should check that we got 3 arguments in the first place. > > Edit: I see that you add those checks in the next commit, but it does > leave the reader wondering at this point. Maybe we should have a strict > check here and then loosen it in the next commit where you make it > optional. For this patch specifically, I was trying to really only factor out the whitespace check into its own script and keep changes outside of that to a minimum. The next patch focuses on all the actual script changes and I was hoping it might be easier to review that way. :) -Justin
Justin Tobler <jltobler@gmail.com> writes: > On 24/05/03 08:56AM, Patrick Steinhardt wrote: >> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote: >> > The `check-whitespace` CI job is only available as a GitHub action. To >> > help enable this job with other CI providers, first separate the logic >> > performing the whitespace check into its own script. In subsequent >> > commits, this script is further generalized allowing its reuse. >> > >> > Helped-by: Patrick Steinhardt <ps@pks.im> >> > Signed-off-by: Justin Tobler <jltobler@gmail.com> >> > --- >> [snip] >> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh >> > new file mode 100755 >> > index 0000000000..f57d1ff5f0 >> > --- /dev/null >> > +++ b/ci/check-whitespace.sh >> > @@ -0,0 +1,74 @@ >> > +#!/bin/bash >> >> This needs to be either "/bin/sh" or "/usr/bin/env bash". > > Since the script is using some shell specific features, I'll update this > to "/usr/bin/env bash" in the next version. This is a question to Patrick, but what makes it bad to assume "bash" is in "/bin" when it is OK to assume that "env" is always in "/usr/bin"? All other comments by Patrick I found very sensible. Thanks, both of you.
On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > On 24/05/03 08:56AM, Patrick Steinhardt wrote: > >> On Thu, May 02, 2024 at 02:38:37PM -0500, Justin Tobler wrote: > >> > The `check-whitespace` CI job is only available as a GitHub action. To > >> > help enable this job with other CI providers, first separate the logic > >> > performing the whitespace check into its own script. In subsequent > >> > commits, this script is further generalized allowing its reuse. > >> > > >> > Helped-by: Patrick Steinhardt <ps@pks.im> > >> > Signed-off-by: Justin Tobler <jltobler@gmail.com> > >> > --- > >> [snip] > >> > diff --git a/ci/check-whitespace.sh b/ci/check-whitespace.sh > >> > new file mode 100755 > >> > index 0000000000..f57d1ff5f0 > >> > --- /dev/null > >> > +++ b/ci/check-whitespace.sh > >> > @@ -0,0 +1,74 @@ > >> > +#!/bin/bash > >> > >> This needs to be either "/bin/sh" or "/usr/bin/env bash". > > > > Since the script is using some shell specific features, I'll update this > > to "/usr/bin/env bash" in the next version. > > This is a question to Patrick, but what makes it bad to assume > "bash" is in "/bin" when it is OK to assume that "env" is always in > "/usr/bin"? My own bias. I know of systems without "/bin/bash" (NixOS), but I don't know of any without "/usr/bin/env". But you're right, "/usr/bin/env" is not part of POSIX and thus not really portable, either. Ultimately I don't think there is any way to write the shebang so that it is fully POSIX compliant. So I'd rather go with the option which is supported on more systems, which is to the best of my knowlede env(1). Patrick
On Fri, May 3, 2024 at 11:27 PM Patrick Steinhardt <ps@pks.im> wrote: > > On Fri, May 03, 2024 at 09:49:13AM -0700, Junio C Hamano wrote: > > This is a question to Patrick, but what makes it bad to assume > > "bash" is in "/bin" when it is OK to assume that "env" is always in > > "/usr/bin"? > > My own bias. I know of systems without "/bin/bash" (NixOS), but I don't > know of any without "/usr/bin/env". But you're right, "/usr/bin/env" is > not part of POSIX and thus not really portable, either. > > Ultimately I don't think there is any way to write the shebang so that > it is fully POSIX compliant. So I'd rather go with the option which is > supported on more systems, which is to the best of my knowlede env(1). The various BSDs mostly stick bash in /usr/local/bin; some versions of macOS did not have a /bin/bash either, as I recall, though my current mac-laptop does. In any case, #! /usr/bin/env <program> is pretty darn common; it's found in a lot of Python scripts, for instance. It works well on old SunOS, on the various BSDs, on macOS, and on Linux, provided of course that the given <program> is installed at all. The *most* portable method is generally to use only POSIX /bin/sh constructs, of course. :-) Chris
diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml index a3a6913ecc..d0a78fc426 100644 --- a/.github/workflows/check-whitespace.yml +++ b/.github/workflows/check-whitespace.yml @@ -26,67 +26,7 @@ jobs: - name: git log --check id: check_out run: | - baseSha=${{github.event.pull_request.base.sha}} - problems=() - commit= - commitText= - commitTextmd= - goodParent= - while read dash sha etc - do - case "${dash}" in - "---") # Line contains commit information. - if test -z "${goodParent}" - then - # Assume the commit has no whitespace errors until detected otherwise. - goodParent=${sha} - fi - commit="${sha}" - commitText="${sha} ${etc}" - commitTextmd="[${sha}](https://github.com/${{ github.repository }}/commit/${sha}) ${etc}" - ;; - "") - ;; - *) # Line contains whitespace error information for current commit. - if test -n "${goodParent}" - then - problems+=("1) --- ${commitTextmd}") - echo "" - echo "--- ${commitText}" - goodParent= - fi - case "${dash}" in - *:[1-9]*:) # contains file and line number information - dashend=${dash#*:} - problems+=("[${dash}](https://github.com/${{ github.repository }}/blob/${commit}/${dash%%:*}#L${dashend%:}) ${sha} ${etc}") - ;; - *) - problems+=("\`${dash} ${sha} ${etc}\`") - ;; - esac - echo "${dash} ${sha} ${etc}" - ;; - esac - done <<< $(git log --check --pretty=format:"---% h% s" ${baseSha}..) - - if test ${#problems[*]} -gt 0 - then - if test -z "${goodParent}" - then - goodParent=${baseSha: 0:7} - fi - echo "
The `check-whitespace` CI job is only available as a GitHub action. To help enable this job with other CI providers, first separate the logic performing the whitespace check into its own script. In subsequent commits, this script is further generalized allowing its reuse. Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Justin Tobler <jltobler@gmail.com> --- .github/workflows/check-whitespace.yml | 68 ++--------------------- ci/check-whitespace.sh | 74 ++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 64 deletions(-) create mode 100755 ci/check-whitespace.sh