Re: [lmi] [lmi-commits] master 0d8f53e 02/18: Resolve shellcheck "SC1117" warning

[Vadim Zeitlin]

On Wed, 29 May 2019 18:49:06 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 0d8f53ef1d66cbedf3935e19b8af954b2e37a80a
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Resolve shellcheck "SC1117" warning

 Oops, it looks like this one is relatively new and while I see it with
shellcheck 0.5.0 from Debian Sid, it didn't appear with shellcheck 0.3.4
from Jessie, which explains why we hadn't seen it.

 Now that I do see it, however, I think it's just plain wrong because we
really want to pass literal string "\n", consisting of 2 characters with
ASCII codes 0x5c and 0x6e, to dpkg-query and I have no idea why should
shellcheck complain about it.

 And please note that https://github.com/koalaman/shellcheck/wiki/SC1117
mentions that this warning was removed because it was more annoying than
useful (which, to be honest, applies to many other shellcheck warnings too
IMHO), after 0.5 (where it presumably was added, although I was too lazy to
confirm this).

GC>     Replaced double quotes with single quotes. Tested thus:
GC>     
GC>     zsh: unrecognized modifier
GC>     1583

 Sorry, what does this part mean?

GC>     This manpages.debian.org example:
GC>       dpkg-query -W -f='${binary:Package} ${Version}\t${Maintainer}\n' dpkg
GC>     uses single quotes.

 I think both double and single quotes work just fine here because
dpkg-query "-f" option also performs its own escape sequences substitution.

 However the change as written:

GC> ---
GC>  install_msw.sh | 2 +-
GC>  1 file changed, 1 insertion(+), 1 deletion(-)
GC> 
GC> diff --git a/install_msw.sh b/install_msw.sh
GC> index 4f4777f..0c15c89 100755
GC> --- a/install_msw.sh
GC> +++ b/install_msw.sh
GC> @@ -146,7 +146,7 @@ then
GC>      # Disable shellcheck warning about the need to double quote 
$packages_list:
GC>      # it can't be done here and we really want word splitting to happen 
here.
GC>      # shellcheck disable=SC2086
GC> -    missing_packages_count=$(dpkg-query -W -f="\${Status}\n" 
$packages_list 2>&1 | \
GC> +    missing_packages_count=$(dpkg-query -W -f='\${Status}\n' 
$packages_list 2>&1 | \

is a bit strange because it uses the format string starting with literal
dollar sign. Somehow "Status" still gets expanded by dpkg-query, but I'm
not sure if it's really intended to work like this. So, just to be on the
safe side, and if you really want to fix this SC1117, I'd use

        missing_packages_count=$(dpkg-query -W -f='${Status}\n' $packages_list 
2>&1 | \

line here.

 Sorry for missing this warning,
VZ

pgpSCztYWQF1_.pgp
Description: PGP signature