make test framework pass shellcheck

[kurahaupo]

This includes

1. uniformly assume Bash version 3;
   • the references to #!/bin/sh and .sh were wrong and misleading;
   • change var=$((var+1)) to simply ((var++))
2. updates so that all scripts pass ShellCheck (version 0.3.3) without any warnings:
   • add quotes around $var and $( cmd ) expansions whereever they may be subject to wordsplitting or globbing;
   • use arrays for lists, rather than word-split strings;
   • include # shellcheck disable= in a few cases;
3. additional logic so that reports can be written to stdout instead of a named file; the framework can at some future time be adjusted to use redirection rather than naming files;
4. stylistic changes to avoid confusing ${} and $() as they are almost indistinguishable in some fonts:
   • prefer $var over ${var} wherever possible.
   • include whitespace inside $( cmd )
5. prefer (( expr )) for numeric operations including assignments and tests;
   • avoid $var inside (( ... ))
6. prefer [[ $x = *glob* ]] over [[ $x =~ .*regex.* ]];

Note; I don't have access to test on all platforms, so I recommend re-running the test suite on all dialects when accepting these changes.

[masatake]

Thank you, but ... the change is too large for reviewing.
If possible, I would like you to split the change into smaller parts like....this pull request. As your comment that enumerates six aspects of your change, I would like you to split the change into six commits.

You wrote what you did at the initial comment of this pull request.
However, I would like you to write it to the commit log.

    if
        bash "$x" "$lsof" "$report" "$d" "$dialect"
    then

I prefer to single line if ~ then like:

if  bash "$x" "$lsof" "$report" "$d" "$dialect";  then

Did you run the test cases? With your change, the test cases are never run because the test harness gets an error. About CI platforms, you can ignore fresbsd1[1-3]. However, we cannot ignore Travis-CI where GNU/Linux runs.

change var=$((var+1)) to simply ((var++))

This change breaks the harness.
Quoted from info of bash:

'((...))'
          (( EXPRESSION ))

     The arithmetic EXPRESSION is evaluated according to the rules
     described below (*note Shell Arithmetic::).  If the value of the
     expression is non-zero, the return status is 0; otherwise the
     return status is 1.

if $var is 0, the EXPRESSION in ((var++)) is 0 though $var is 1.
Because of "set -e" at the head of the test harness (check.bash), ((var++)) evaluated as 0 stops the script.
Therefore, ((++var)) is suitable rewriting for var=$((var+1)).
I'm afraid that we cannot remember this mysterious tip when extending the test harness in the future.

[kurahaupo]

This whole thing got much bigger than I'd intended; I started by looking at one file, and ended up auditing more than 20. I've already far exceeded the amount of time I wanted to spend on it, so it may be some time before I come back to it.

I couldn't figure out how to run the integration checks in my branch; I expected that such a large change would probably break something, which is why I expressly asked to have it tested.

The use of set -e (or equivalently, shopt -os errexit) is controvertial; some regard it as an essential error-catcher, others regard it as something that's so unreliable that it gives a false sense of security, and should be excised from production code, and because it breaks basic idioms like arithmetic. I'm in the latter camp, so I never use it code I write, and often forget to test against it.

That said, the reliable fix is to preface ! on any command whose exit status should be ignored by errexit, thus:

! ((var++))

Because this is commonly cited as a reason for preferring var=$((...)) over ((var=...)), I will ask Vidal about added this hint to Shellcheck, and consider incorporating it into my normal coding style.

Many people prefer the "single line if", because it matches how they think of "if" in other languages.
I use a multiline "if" precisely to remind me that it's not the same as other languages, but I don't expect you to be convinced by that, so I'll change it in this case.

[masatake]

I couldn't figure out how to run the integration checks in my branch.

This is just a question.
Do you mean you have never run the test locally after making the change?
(I don't talk about CI platforms.)

[kurahaupo]

I did try to run what I could but obviously I missed that somehow.

[masatake]

I did try to run what I could but obviously I missed that somehow.

I see.

[masatake]

I would like you to split this pull request to smaller commits when you find time.
The change is too large for me to review. My skill of reviewing is limited. However, I must maintain lsof. So I would like to
keep the code understandable for me.

It is better to make new requests for each commits. I will merge them incrementally. NOTE: I will reject some of them if the change doesn't match my preference.

I would like you to run the test cases locally before making a pull request. Let's me know if you don't know how to do it.