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@xxxxxx> > > Signed-off-by: Justin Tobler <jltobler@xxxxxxxxx> > > --- > [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
