[1/2] Make `check-whitespace` failures more helpful

Commit Message

Chris Webster Dec. 16, 2022, 8:31 a.m. UTC

From: "Chris. Webster" <[e-mail]>

Add the errors to the job summary along with suggested
commands to fix the problem.

Signed-off-by: Chris. Webster <[e-mail]>
---
 .github/workflows/check-whitespace.yml | 39 +++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Dec. 16, 2022, 10:06 a.m. UTC | #1

"Chris. Webster via GitGitGadget" <[e-mail]> writes:

> Subject: Re: [PATCH 1/2] Make `check-whitespace` failures more helpful

People usually make changes to the system to make it "more useful"
and/or "more helpful", and almost never to make it "less helpful".
Phrases you would use to explain why the failures become more
helpful with this change (compared to without) would help to promote
it in the "git shortlog --no-merges" output for the next release.
E.g. "make X failures stand out more", "make X failures gramatically
correct", "show X failures more concisely", etc.

> diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
> index ad3466ad16e..3a99073bc33 100644
> --- a/.github/workflows/check-whitespace.yml
> +++ b/.github/workflows/check-whitespace.yml
> @@ -13,38 +13,57 @@ jobs:
>    check-whitespace:
>      runs-on: ubuntu-latest
>      steps:
> -    - uses: actions/checkout@v2
> +    - uses: actions/checkout@v3

I think we saw changes to upgrade actions/checkout@ in another
topic, and it seems that we have missed this one even though we
should have upgraded it the same way as other files in the same
directory?  Shouldn't this hunk be a separate topic on its own,
or at least a separate patch on its own in the series?

>        with:
>          fetch-depth: 0
>  
>      - name: git log --check
>        id: check_out
>        run: |
> -        log=
> +        problems=()

Is it safe to assume we run Bash here, or can GitHub start using
other shells that lack the Bash-ism shell arrays and we should
protect against such future?

I suspect that we are already depend on <<< Bash-ism, so one more
dependency to Bash-ism is not a problem here?  I dunno.

Thanks.

Chris Webster Dec. 20, 2022, 12:30 a.m. UTC | #2

On Fri, Dec 16, 2022 at 2:06 AM Junio C Hamano <[e-mail]> wrote:
>
> People usually make changes to the system to make it "more useful"
> and/or "more helpful", and almost never to make it "less helpful".
> Phrases you would use to explain why the failures become more
> helpful with this change (compared to without) would help to promote
> it in the "git shortlog --no-merges" output for the next release.
> E.g. "make X failures stand out more", "make X failures gramatically
> correct", "show X failures more concisely", etc.

I will resend with a hopefully better explanation.

> directory?  Shouldn't this hunk be a separate topic on its own,
> or at least a separate patch on its own in the series?

It will now be a separate patch.

> Is it safe to assume we run Bash here, or can GitHub start using
> other shells that lack the Bash-ism shell arrays and we should
> protect against such future?
>
> I suspect that we are already depend on <<< Bash-ism, so one more
> dependency to Bash-ism is not a problem here?  I dunno.

While GitHub could probably allow other shells to be used, changing
the default would probably break a lot of things at this point.

Thanks for the feedback,
...chris.

Junio C Hamano Dec. 20, 2022, 1:36 a.m. UTC | #3

Chris Webster <[e-mail]> writes:

>> I suspect that we are already depend on <<< Bash-ism, so one more
>> dependency to Bash-ism is not a problem here?  I dunno.
>
> While GitHub could probably allow other shells to be used, changing
> the default would probably break a lot of things at this point.

Do we specifically ask for bash in our .github/ files?  That would
be perfectly acceptable.  Then we only have to worry about their
withdrawing support for bash which would never happen ;-)

Thanks.

Chris Webster Dec. 20, 2022, 5:50 a.m. UTC | #4

On Mon, Dec 19, 2022 at 5:36 PM Junio C Hamano <[e-mail]> wrote:
> Do we specifically ask for bash in our .github/ files?  That would
> be perfectly acceptable.  Then we only have to worry about their
> withdrawing support for bash which would never happen ;-)

We use the default but the default can be changed
https://docs.github.com/en/actions/using-jobs/setting-default-values-for-jobs.
IOW we can always use bash if the default changes.

...chris.

Patch

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index ad3466ad16e..3a99073bc33 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -13,38 +13,57 @@  jobs:
   check-whitespace:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v2
+    - uses: actions/checkout@v3
       with:
         fetch-depth: 0
 
     - name: git log --check
       id: check_out
       run: |
-        log=
+        problems=()
         commit=
-        while read dash etc
+        commitText=
+        lastcommit=
+        while read dash sha etc
         do
           case "${dash}" in
           "---")
-            commit="${etc}"
+            if test -z "${commit}"
+            then
+              lastcommit=${sha}
+            fi
+            commit="${sha}"
+            commitText="${sha} ${etc}"
             ;;
           "")
             ;;
           *)
             if test -n "${commit}"
             then
-              log="${log}\n${commit}"
+              problems+=("" "--- ${commitText}")
               echo ""
-              echo "--- ${commit}"
+              echo "--- ${commitText}"
+              commit=
             fi
-            commit=
-            log="${log}\n${dash} ${etc}"
-            echo "${dash} ${etc}"
+            problems+=("${dash} ${sha} ${etc}")
+            echo "${problems[-1]}"
             ;;
           esac
         done <<< $(git log --check --pretty=format:"---% h% s" ${{github.event.pull_request.base.sha}}..)
 
-        if test -n "${log}"
+        if test ${#problems[*]} -gt 0
         then
+          if test -z "${commit}"
+          then
+            lastcommit=${{github.event.pull_request.base.sha}}
+          fi
+          echo "A whitespace issue was found in one or more of the commits." >$GITHUB_STEP_SUMMARY
+          echo "" >>$GITHUB_STEP_SUMMARY
+          echo "Run \`git rebase --whitespace=fix ${lastcommit}\` and \`git push --force\` to correct the problem." >>$GITHUB_STEP_SUMMARY
+          for i in "${problems[@]}"
+          do
+            echo "${i}" >>$GITHUB_STEP_SUMMARY
+          done
+
           exit 2
         fi