Re: [lmi] [lmi-commits] master 29b03dc 4/5: Improve portability of shell scripts

[Greg Chicares]

On 2018-04-07 11:01, Vadim Zeitlin wrote:
> On Fri,  6 Apr 2018 21:41:37 -0400 (EDT) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit 29b03dc1a807155863cbcf923dcf43ec5c5628dd
> GC> Author: Gregory W. Chicares <address@hidden>
> GC> Commit: Gregory W. Chicares <address@hidden>
> GC> 
> GC>     Improve portability of shell scripts
> GC>     
> GC>     Ran 'shellcheck', at first with no '-e' option, and ultimately thus:
> GC>       for z in *.sh; do shellcheck -e2046,2166,2003,2129 $z; done
> GC>     and resolved most of the concerns it flagged.
> GC>     
> GC>     The 'install_*.sh' scripts in particular need careful testing.
> [...]
> GC> diff --git a/install_wx.sh b/install_wx.sh
> GC> index bf35ed2..2b2a5bf 100755
> GC> --- a/install_wx.sh
> GC> +++ b/install_wx.sh
> GC> @@ -48,16 +48,16 @@ then
> GC>      wx_dir_parent=${wx_dir%/*}
> GC>      [ -d $wx_dir_parent ] || mkdir -p $wx_dir_parent
> GC>      cd $wx_dir_parent
> GC> -    git clone $wx_git_url ${wx_dir##*/}
> GC> +    git clone "$wx_git_url" ${wx_dir##*/}
> 
>  Does this mean that all variables expansions now need to be inside quotes?

No. Especially not here:

../configure $config_options
             ^-- SC2086: Double quote to prevent globbing and word splitting.

as `$git show 180620b65` makes clear.

> We can be sure that wx_git_url (nor wx_commit_sha below) contains neither
> spaces nor any wildcard characters, so perhaps we could add 2086 to the
> list of disabled shellcheck warnings instead? The extra quotes really don't
> help the already not very readable shell syntax...

Let me know if you'd like
-    git clone $wx_git_url ${wx_dir##*/}
+    git clone "$wx_git_url" ${wx_dir##*/}
and any other specific changes reverted.

>  If not, I'd have to update the PR 80 as it contains some more unquoted
> variable expansions (all of which are still safe though).

I'm trying to ensure that you don't have to update any PR; thus:

f611bc98 Use relative URLs for wxWidgets submodules in install script
ee4adeaf Roll back some changes to avoid cherry-pick conflict

where f611bc98 is PR 80 without any modification.

My goal is to avoid creating needless extra work for you when I
cherry-pick PRs like that one, so please let me know if I'm not
achieving that goal.

> GC> -[ -n $mingw_bin_dir ] && export PATH="$mingw_bin_dir:${PATH}"
> GC> +[ -n "$mingw_bin_dir" ] && export PATH="$mingw_bin_dir:${PATH}"
> 
>  This one is a real bug, sorry for letting it slip in and thanks for
> finding it!

I'm just running 'shellcheck' and trying to separate the good suggestions
it makes from the bad ones. I believe I understand some of them fully:
'$()' vs. '``', for example. Replacing wildcard '*' with './*' seems like
a good idea even though I'm sure I'll never name a file '--recursive'.
But I really have no idea when double quotes are
  {necessary, legal-but-less-readable, harmful, ...}
especially the middle case, as above:
-    git clone $wx_git_url ${wx_dir##*/}
+    git clone "$wx_git_url" ${wx_dir##*/}
How can we be certain that a variable like $wx_git_url contains no
whitespace? On a corporate msw machine where we create a mirror at
  $HOME/MyMirror/
couldn't that expand to
  Corporate Employees/John Smith/MyMirror
?