Fixed bugs found by the ShellCheck linter

[tdulcet]

Edit: This PR is now very outdated, but I would be happy to update it if there is ever interest in fixing these bugs.

• Fixed numerous bugs found by ShellCheck.
  • See many of the bugs here (lines 1374-2490).
  • Removed all the depreciated/legacy/antiquated Bash syntax (such as $[..] and `..`) and commands (such as tempfile).
• Outputs error messages to stderr. (Removed)
• Removed the use of many unnecessary commands.
  • Removed all the unnecessary cat and echo commands.
  • Replaced all the uses of the pwd command with Bash's $PWD variable.
• Fixed minor spelling/capitalization errors in script documentation. (Removed)

This pull request was originally part of #1435.

Closes #2342

[zatricky]

Very good work overall. Lots of minor shell ambiguities and spelling issues rectified.

[tdulcet]

@zatricky All your requested changes have been made in 93fe6bf.

[zatricky]

Good changes. I think the only nitpick left is the $* vs $@ in setup/functions.sh - which I defer to Josh.

[JoshData]

Hey.

So, this is still too big for me to review. I'm also hesitant to spend time making changes that aren't (actual) bugs or new functionality. Every change has the potential to create new problem....

[tdulcet]

There are plenty of "actual" bugs here. For example, I quoted all occurrences of the STORAGE_ROOT and STORAGE_USER variables, because the directories could contain a space, which would currently cause the script to fail. I removed all the depreciated/legacy/antiquated Bash syntax (such as $[..] and `..`) and Linux commands (such as tempfile), which could be removed at any time. I also removed many unnecessary cat and echo commands and made other changes, which simplify the scripts.

You can see the ShellCheck output here (lines 1374-2490), which describes the most of the bugs in more detail.

[bittorf]

We really should fix this:
Maybe most of these problems are harmless, but having
a chance to spot new problems is worth the effort.

For now it looks like this:

#!/bin/sh
cd mailinabox-git
LIST="$( { find . -type f -iname '*.sh' && find . -type f -exec grep -l '^#!/bin/bash\|^#!/usr/bin/env sh' {} \; ;} | sort -u )"
for FILE in $LIST; do shellcheck -x "$FILE"; done | sed -n 's/.* \(SC[0-9][0-9][0-9][0-9]\) .*/\1/p' | sort | uniq -c | sort -rn

    192 SC2086
     17 SC1091
     11 SC2046
      7 SC2148
      7 SC2002
      6 SC2236
      6 SC2001
      3 SC2166
      3 SC2034
      2 SC2006
      1 SC2164
      1 SC2162
      1 SC2140
      1 SC2091
      1 SC2044
      1 SC2007
      1 SC2005
      1 SC2003

for FILE in $LIST; do shellcheck -x "$FILE" | sed -n "s/.* \(SC[0-9][0-9][0-9][0-9]\) .*/$( basename -- "$FILE")/p"; done | uniq -c | sort -rn

     37 nextcloud.sh
     27 functions.sh
     17 start.sh
     15 web.sh
     15 ssl.sh
     15 questions.sh
     14 system.sh
     13 mail-postfix.sh
     11 webmail.sh
     11 mail-dovecot.sh
     11 bootstrap.sh
      9 owncloud-restore.sh
      8 munin.sh
      8 firstuser.sh
      7 mail-users.sh
      6 spamassassin.sh
      5 preflight.sh
      5 owncloud-unlockadmin.sh
      5 network-checks.sh
      5 management.sh
      5 dkim.sh
      4 dns.sh
      4 archive_conf_files.sh
      2 daily_tasks.sh
      1 zpush.sh
      1 web_update
      1 dns_update

[tdulcet]

We really should fix this:
Maybe most of these problems are harmless, but having
a chance to spot new problems is worth the effort.

Thanks, I agree. I created this PR over 5 years ago now, but I would be happy to rebase and update it if there is a chance it could ever be merged.

In addition to those issues in the Bash scripts, there are also many bugs in the Python code, which can be found with the Pylint and Ruff linters among others. I could create a separate PR for this, if there is any interest in fixing these bugs... For example, here are the counts of the potential issues identified by Ruff (almost half of these are autofixable):

/mailinabox$ ruff --target-version py310 --select F,E4,E7,E9,W,I,UP,YTT,S,BLE,B,A,C4,T10,DJ,EM,EXE,ISC,ICN,G,PIE,PYI,Q003,Q004,RSE,RET,SLF,SLOT,SIM,TID,TCH,ARG,PGH,PL,TRY,FLY,PERF,FURB,LOG,RUF --preview --ignore W191,PLR09,PLR2004,RUF001,RUF002,RUF003 --line-length 320 . | head -n -2 | awk '{ print $2 }' | sort | uniq -c | sort -rn
error: Failed to parse tools/readable_bash.py:136:3: unindent does not match any outer indentation level
error: Failed to parse tools/mail.py:3:19: Unexpected token "$@"
     99 PLC0415
     85 E701
     47 UP031
     44 PLW1514
     37 I001
     33 W605
     32 PLR6201
     27 TRY003
     27 EM101
     26 PLC1901
     26 E401
     24 PLW2901
     22 UP032
     22 RET505
     20 FURB101
     19 UP015
     16 F401
     16 E722
     13 F841
     12 W293
     12 TRY200
     12 RUF005
     12 B904
     12 B007
     11 RSE102
     11 C401
     10 ARG001
      9 Q003
      8 SIM108
      8 E741
      8 BLE001
      7 TRY004
      7 TRY002
      6 SIM105
      6 SIM102
      6 S110
      6 RET504
      6 E711
      4 TRY300
      4 S603
      4 PERF401
      4 PERF203
      4 FURB113
      4 A002
      3 W291
      3 S607
      3 S324
      3 RET503
      3 PIE810
      2 UP030
      2 TRY301
      2 SIM110
      2 S310
      2 PLW0108
      2 ISC002
      2 G001
      2 F811
      2 E999
      2 E721
      2 E703
      1 W292
      1 UP041
      1 UP038
      1 UP034
      1 UP024
      1 SIM223
      1 SIM222
      1 SIM212
      1 SIM118
      1 SIM114
      1 S605
      1 S108
      1 RUF017
      1 RET507
      1 PLW0603
      1 PLW0120
      1 PLR6301
      1 PLR5501
      1 PLR1711
      1 PIE790
      1 ISC003
      1 G002
      1 F821
      1 EXE003
      1 EM103
      1 E713
      1 E712
      1 E402
      1 C414
      1 C405
      1 C404
      1 B012
      1 B006
      1 ARG005
      1 ARG002
      1 A001

[JoshData]

My comment from 2018 still holds. It's hard for me to dedicate time to things that have no impact on functionality. The way to get the changes merged is to create smaller commits with one type of change per commit so that it's easy for me to understand what the changes are.

Also if you call something a bug but then don't demonstrate how Mail-in-a-Box operates incorrectly, it's a show-stopper for review, because I'm not going to do the work to investigate a "bug" that turns out to be a code style preference.

[tdulcet]

Thanks Josh for your response. I can try to break this up into more commits, but note that many of the issues can now be fixed automatically by just running a single command like: shellcheck -f diff -s bash **/*.sh | git apply. Back in 2018 I had to make all of these fixes manually.

The term "bug" was used by ShellCheck to describe the type of issues that their linter detects, but I would be happy to use any alternative term that you prefer instead. For example, the term "error" is commonly used by other linters.

As I explained above back in 2018, I would consider almost all of the issues detected to be actual bugs, as either they address cases where the scripts would currently fail or they replace depreciated syntax/commands that could be removed and thus break at any time. Removing the depreciated syntax and commands should help significantly in preparation for the next Ubuntu LTS upgrade, as this most likely when it would otherwise break. A few of the changes removed unnecessary/useless commands to simply the script and improve readability/performance. If you consider this to be a "code style preference", I could remove these changes in the update/rebase. I also manually fixed a few typos I noticed in the script documentation, which I can remove as well, as they are obviously the least important of these fixes. I am not sure your definition of "code style preference", but I purposely did not include anything in this PR that I thought could be considered opinionated, although I would be happy to remove any of the individual changes if you feel differently. I would want to further hold up fixing the many critical issues this PR addresses.

I would highly encourage you to run ShellCheck yourself, as it explains each issue in more detail than I can in this PR. It also includes links to their Wiki, which explains each issue in even more detail, usually with examples and citations. For reference, here are the commands to run on Ubuntu and Debian:

sudo apt update
sudo apt install shellcheck
shopt -s globstar
shellcheck -s bash **/*.sh

(Note that this will miss a couple of scripts without the .sh extension. See @bittorf's find command above for an example of how to check those as well.)

[bittorf]

This is just a starter - the question is, if this format
(all fixes for one file) is OK and auditable and has
a proper commit message.

[tdulcet]

The way to get the changes merged is to create smaller commits with one type of change per commit so that it's easy for me to understand what the changes are.

@JoshData - As requested, I redid this PR, doing only a single type of fix per commit. Two of the fixes are each broken up into two separate commits, so there are a total of 17 fixes and 19 commits. Most of the commits only modify a few lines or less.

Please let me know if you have any questions or if there is anything else you need me to do for this to be merged.

[tdulcet]

@JoshData - I do not believe you meant to close this, as #2342 only fixed the ShellCheck issues in one of the 27 files this PR addresses.

[JoshData]

Ooops. Thanks. But this PR now has some conflicts with the main branch. Can you resolve those?

[tdulcet]

No problem. Yes, I will rebase this PR from the main branch.

[JoshData]

Thanks! I'm force-pushing to your branch to drop a few commits:

• Fixed SC2140: Word is of the form "A"B"C".: This commit was empty other than changing file permissions.
• Fixed SC2002: Useless cat.: This makes it less readable.

I also squashed a few while I was here.

After I push I'll merge. Thanks for your patience!

[bilogic]

May I also suggest standardizing on a code formatter?

And I propose using the standard one found in VSCode.