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

[Greg Chicares]

On 2019-05-29 23:04, Vadim Zeitlin wrote:
> On Wed, 29 May 2019 18:49:06 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC> commit 0d8f53ef1d66cbedf3935e19b8af954b2e37a80a
[...]
> GC>     Resolve shellcheck "SC1117" warning
[...]
>  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.

Until now, I've been running shellcheck manually and sporadically,
each time trying to pick out the warnings I should heed while
mentally filtering out those that seemed like mere nuisances.
That took considerable effort each time, so I ran it less often;
and I missed some important things.

Now, instead, I've decided to run it all the time, automatically.
In the rare cases where its advice is wrong, I disable the
particular warning, with an explanation, e.g.:

  # 'configure' options must not be double-quoted
  # shellcheck disable=SC2086
  "$proxy_wx_dir"/configure $config_options

In many cases (frequently, for SC1117), there are various ways of
writing a command, and shellcheck prefers one while warning about
another. I could disable such warnings globally, but that would
inhibit diagnostics that might identify actual errors. Or I could
repeatedly write:

  # This warning is just a nuisance.
  # shellcheck disable=SC1117
  printf "\nHello\n"

but it's easier just to make a change like:

  printf '\nHello\n'

and to adopt a habit of writing all such statements the same way.

In exchange for restricting myself to the subset of 'sh' code that
shellcheck accepts without warning, I get notified automatically
of some mistakes, before I even commit them. For me, that's a good
tradeoff.

>  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).

$ shellcheck --version
version: 0.5.0

He says it might reappear later, subject to a 'pedantic' flag.
I would probably enable "pedantic" warnings for shellcheck, as I
do for gcc.

> GC>     Replaced double quotes with single quotes. Tested thus:
> GC>     
> GC>     zsh: unrecognized modifier
> GC>     1583
> 
>  Sorry, what does this part mean?

lmi commit 16034b8239 is necessary to prevent git from quietly doing
something that's astonishing and wrong. The commit message I entered
for 0d8f53ef included:

# dpkg-query -f "${binary:Package}\n" -W | wc -l
zsh: unrecognized modifier
# dpkg-query -f '${binary:Package}\n' -W | wc -l
1583

which seems to make a point in favor of single quotes here.

> 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.

The commands above (which git stripped from the commit message)
seem to show otherwise.

>  However the change as written:
[...]
> 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.

Committed. Now it's written the same way manpages.debian.org writes it.