install.sh: Fix ShellCheck warnings by CyberShadow · Pull Request #239 · dlang/installer

CyberShadow · 2017-07-17T12:39:33Z

No description provided.

dlang-bot · 2017-07-17T12:39:34Z

Thanks for your pull request, @CyberShadow!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

CyberShadow · 2017-07-17T12:40:42Z

script/install.sh

    fi
    log "Removing $path/$1"
-    rm -rf "$path/$1"
+    rm -rf "${path:-.}/$1"


This one is particularly scary - it doesn't look like it can expand to rm -rf /, but it gets too close for comfort, e.g. with --path /.

PetarKirov

Overall LGTM. It would have been nice if each category of warnings fixed was in its own commit and the commit message included the warning text itself.

PetarKirov · 2017-07-17T14:09:11Z

script/install.sh

    check_tools egrep
    if [ -d "$path" ]; then
-        ls "$path" | egrep -v '^dub|^install\.sh|^d-keyring\.gpg'
+        find "$path" -maxdepth 1 -printf "%f\n" 1 | grep -Ev '^dub|^install\.sh|^d-keyring\.gpg'


Why the change from ls to find -maxdepth 1?

As I understand, ls's output is not meant to be parseable, it's meant for humans. The particular warning here is SC2012, but there are some other related warnings related to piping ls.

Note that this fix was incorrect and suboptimal; in the latest version it is all one find invocation, which also allows us to drop grep/egrep.

PetarKirov · 2017-07-17T14:14:58Z

script/install.sh

    fi
    log "Removing $path/$1"
-    rm -rf "$path/$1"
+    rm -rf "${path:-.}/$1"


"> is for string comparisons. Use -gt instead." https://github.com/koalaman/shellcheck/wiki/SC2071

"Quote this to prevent word splitting." https://github.com/koalaman/shellcheck/wiki/SC2046

"Use "${var:?}" to ensure this never expands to / ." https://github.com/koalaman/shellcheck/wiki/SC2115

"Declare and assign separately to avoid masking return values." https://github.com/koalaman/shellcheck/wiki/SC2155

"$/${} is unnecessary on arithmetic variables." https://github.com/koalaman/shellcheck/wiki/SC2004

CyberShadow · 2017-07-18T02:43:14Z

It would have been nice if each category of warnings fixed was in its own commit and the commit message included the warning text itself.

Good idea, done. I also improved some of the fixes slightly.

SC2012: "Use find instead of ls to better handle non-alphanumeric filenames." https://github.com/koalaman/shellcheck/wiki/SC2012 SC2196: "egrep is non-standard and deprecated. Use grep -E instead." https://github.com/koalaman/shellcheck/wiki/SC2196

"Note that A && B || C is not if-then-else. C may run when A is true." https://github.com/koalaman/shellcheck/wiki/SC2015

"Double quote to prevent globbing and word splitting." https://github.com/koalaman/shellcheck/wiki/SC2086

PetarKirov

Thanks @CyberShadow for splitting the PR into separate commits and linking to the respective Shell Check wiki entry, much appreciated!

By re-reviewing the commits one-by-one and reading the respective SC explanations, I feel much more comfortable signing off on this PR.

wilzbach

Thanks @CyberShadow for splitting the PR into separate commits and linking to the respective Shell Check wiki entry, much appreciated!

Same here :)

wilzbach · 2017-07-19T16:14:45Z

Btw how about adding a check to Travis, s.t. it future additions are checked as well?

-> https://github.com/koalaman/shellcheck/wiki/TravisCI

MartinNowak

Nice, thanks a lot.

MartinNowak · 2017-08-14T11:00:13Z

script/install.sh

    fi
    log "Removing $path/$1"
-    rm -rf "$path/$1"
+    rm -rf "${path:?}/$1"


Well that's why we're using set -u, but anyhow.

With set -u, $var will error only if it's unset, not empty. ${var:?} will always error if var is empty (or unset).

MartinNowak · 2017-08-14T11:01:24Z

script/install.sh

-    local path=$(mktemp "$TMP_ROOT/XXXXXX")
+    local url path
+    url=$1
+    path=$(mktemp "$TMP_ROOT/XXXXXX")


CyberShadow commented Jul 17, 2017

View reviewed changes

CyberShadow requested a review from MartinNowak July 17, 2017 12:42

PetarKirov approved these changes Jul 17, 2017

View reviewed changes

CyberShadow added 5 commits July 18, 2017 02:42

install.sh: Fix ShellCheck error SC2071

f713876

"> is for string comparisons. Use -gt instead." https://github.com/koalaman/shellcheck/wiki/SC2071

install.sh: Fix ShellCheck warning SC2046

00745ea

"Quote this to prevent word splitting." https://github.com/koalaman/shellcheck/wiki/SC2046

install.sh: Fix ShellCheck warning SC2115

79232f5

"Use "${var:?}" to ensure this never expands to / ." https://github.com/koalaman/shellcheck/wiki/SC2115

install.sh: Fix ShellCheck warning SC2155

23ea966

"Declare and assign separately to avoid masking return values." https://github.com/koalaman/shellcheck/wiki/SC2155

install.sh: Fix ShellCheck note SC2004

89d862f

"$/${} is unnecessary on arithmetic variables." https://github.com/koalaman/shellcheck/wiki/SC2004

CyberShadow force-pushed the pull-20170717-123612 branch from 595b514 to 9b5a2c7 Compare July 18, 2017 02:42

CyberShadow force-pushed the pull-20170717-123612 branch 2 times, most recently from 07ea81c to deb1eea Compare July 18, 2017 02:50

CyberShadow added 3 commits July 18, 2017 02:58

install.sh: Fix ShellCheck note SC2015

5793069

"Note that A && B || C is not if-then-else. C may run when A is true." https://github.com/koalaman/shellcheck/wiki/SC2015

install.sh: Fix ShellCheck note SC2086

c0763b8

"Double quote to prevent globbing and word splitting." https://github.com/koalaman/shellcheck/wiki/SC2086

CyberShadow force-pushed the pull-20170717-123612 branch from deb1eea to c0763b8 Compare July 18, 2017 02:58

PetarKirov approved these changes Jul 18, 2017

View reviewed changes

wilzbach approved these changes Jul 19, 2017

View reviewed changes

MartinNowak approved these changes Aug 14, 2017

View reviewed changes

run shellcheck in CI

9f549b9

MartinNowak force-pushed the pull-20170717-123612 branch from 1a0eccb to 9f549b9 Compare August 14, 2017 14:13

MartinNowak merged commit 2eabc00 into dlang:master Aug 14, 2017

John-Colvin mentioned this pull request Jul 31, 2018

fix uninitialised suffix on macOS #334

Merged

Uh oh!

Comments

Conversation

CyberShadow commented Jul 17, 2017

Uh oh!

dlang-bot commented Jul 17, 2017

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 19, 2017

Uh oh!

MartinNowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyberShadow commented Jul 18, 2017 •

edited

Loading