Skip to content

🚨 shellcheck'ed #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thomasmerz
Copy link
Contributor

Before:

$ shellcheck dnstest.sh

In dnstest.sh line 8:
NAMESERVERS=`cat /etc/resolv.conf | grep ^nameserver | cut -d " " -f 2 | sed 's/\(.*\)/&#&/'`
            ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
                 ^--------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

Did you mean:
NAMESERVERS=$(cat /etc/resolv.conf | grep ^nameserver | cut -d " " -f 2 | sed 's/\(.*\)/&#&/')


In dnstest.sh line 46:
        ttime=`$dig +tries=1 +time=2 +stats @$pip $d |grep "Query time:" | cut -d : -f 2- | cut -d " " -f 2`
              ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
                                             ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                  ^-- SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        ttime=$($dig +tries=1 +time=2 +stats @"$pip" "$d" |grep "Query time:" | cut -d : -f 2- | cut -d " " -f 2)


In dnstest.sh line 50:
        elif [ "x$ttime" = "x0" ]; then
               ^-------^ SC2268 (style): Avoid x-prefix in comparisons as it no longer serves a purpose.

Did you mean:
        elif [ "$ttime" = "0" ]; then


In dnstest.sh line 57:
    avg=`bc -lq <<< "scale=2; $ftime/$totaldomains"`
        ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.

Did you mean:
    avg=$(bc -lq <<< "scale=2; $ftime/$totaldomains")

For more information:
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
  https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
  https://www.shellcheck.net/wiki/SC2006 -- Use $(...) notation instead of le...

After no complains anymore 👍🏻

@thomasmerz
Copy link
Contributor Author

@cleanbrowsing , have you seen my PR and can you please review and merge it? Thanks…

@thomasmerz
Copy link
Contributor Author

@cleanbrowsing , I rebased this PR and shellcheck'ed some more style-warnings that came from some other PRs:

SC2181 (style): Check exit code directly with e.g. 'if mycmd;', not indirectly with $?
SC2268 (style): Avoid x-prefix in comparisons as it no longer serves a purpose.

@thomasmerz
Copy link
Contributor Author

Testing dnstest.sh after I shellcheck'ed it:
image

@cleanbrowsing
Copy link
Owner

The issue with this syntax is that it breaks on openbsd's default shell (the standard sh shell - openbsd user here). For example, this is what I get when I run with the changes:

./dnstest.sh: syntax error: `< ' unexpected

That's why I kept the "x$1" syntax and the "if [ $?" format to test for the command results. Not sure what can be done to make it compatible with the standard sh shells and remove this format.

@cleanbrowsing
Copy link
Owner

cleanbrowsing commented Mar 17, 2022

Quick way to test:

$ ./test.sh 
./test.sh[5]: [: test: unexpected operator/operand

file:

#!/bin/sh

if [ $1 = "test" ]; then
    echo "cmd1 = test"
fi

@cleanbrowsing
Copy link
Owner

If I change to:

$ cat test.sh 
#!/bin/sh

if [ "x$1" = "xtest" ]; then
    echo "cmd1 = test"
fi

It works:

$ ./test.sh 
$ ./test.sh  test
cmd1 = test

Using OpenBSD 6.9

@thomasmerz
Copy link
Contributor Author

I added a commit to fix your problem with openbsd's default shell. But I wonder, if shebang says "use bash" why openbsd is ignoring this and uses default shell (that is not bash?) 🤔 …

@cleanbrowsing
Copy link
Owner

Thanks! Checking that now.

@thomasmerz thomasmerz mentioned this pull request Mar 18, 2022
@thomasmerz
Copy link
Contributor Author

Ping @cleanbrowsing

1 similar comment
@thomasmerz
Copy link
Contributor Author

Ping @cleanbrowsing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants