linting: add shellcheck for shell script lint

[L-series]

Add linting for shell scripts in the scipts directory as well as any
*.sh file accross the project using shellcheck.

[hanno-becker]

I am not a shell expert, but this looks wrong? Do we really want to pass the list of files as a single quoted argument to shfmt here?

[hanno-becker]

This fails locally for me. How come the CI does not catch this?

[L-series]

Hey Hanno, I just spent some time looking at this - it turns out that the command
shfmt -s -w -l -i 2 -ci -fn "$(shfmt -f "$(git grep -l '' :/)")"
has the following odd behavior:

1. It fails loudly (in the sense that it was printing an incorrect list of files which needed to be formatted to the console) - I missed this when rerunning my lint script.
2. In the context of checkerr, since we aren't calling the functions with the redirection of stderr to stdout, it receives an empty $out$ as shfmt was writing those logs to stderr.
3. Since $out was empty and the return value was 1, success would still remain true and it was appearing as if the linting had succeeded.

In general it would probably be wise to have checkerr fail if the return value is anything other than 0. I can implement this change if you'd like.

I've fixed the syntax and now CI should be passing.

[hanno-becker]

Thank you @L-series, in general I think linting the shell scripts is a great idea -- esp. seeing that at least I'm not a shell expert.

However, it looks like some of the quoting has broken scripts/lint, at least the Lint Shell command. Can you have a look, both at the issue itself and why the CI did not catch it?

[L-series]

@hanno-becker Hello, please see my comment above regarding what the issue was with the shfmt syntax. I've changed that syntax and I simplified the logic behind the shellcheck implementation.

I'm adding a variable which tracks the shells scripts which exist across the project and I pass that to both shfmt and shellcheck instead of recomputing it for shellcheck.

[hanno-becker]

Why have we lost -w?

[hanno-becker]

Will this work if scripts ever contain spaces in their file names?

[L-series]

Added back the missing -w and fixed the command to use xargs as per the lint script. I've checked and this will not work for files which contain spaces in the filename, however the original implementation also suffers from this issue. The output of renaming lint to lint new and calling shfmt on it:

shfmt -s -w -l -i 2 -ci -fn $(shfmt -f $(git grep -l '' :/))
lstat scripts/lint: no such file or directory
lstat new: no such file or directory

I'll spend some time tomorrow thinking on how to resolve this.

[L-series]

We could use a loop of the sort to bypass this issue:

git grep -l '' :/ | while IFS= read -r file; do
  shfmt -f "$file"
done | while IFS= read -r shellfile; do
  shfmt -s -w -l -i 2 -ci -fn "$shellfile"
done