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

From: Patrick Steinhardt <ps@pks.im>
Date: May 3, 2024, 06:56 UTC
Message-ID: <ZjSKsGooraoQDU2I@tanuki>
In-Reply-To: <20240502193840.105355-5-jltobler@gmail.com>

On Thu, May 02, 2024 at 02:38:38PM -0500, Justin Tobler wrote:

Show 26 quoted lines

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

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

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

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

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

Patrick