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

From: Patrick Steinhardt <[email-masked]>
Date: Fri, 3 May 2024 08:56:41 +0200

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 <[email-masked]>
> Signed-off-by: Justin Tobler <[email-masked]>
> ---
[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