[pacman-dev] shellcheck conformity for makepkg

[Alad Wenter]

shellcheck is a static analysis tool to check common mistakes in shell 
scripting. Running it on makepkg yields 116 warnings; below I propose 
how to address some of them.

The idea would be that each warning, if applicable, is addressed in a 
separate patch.

* /usr/bin/makepkg:138:10: warning: Declare and assign separately to 
avoid masking return values. [SC2155]

makepkg does separate declare and assign statements _if_ it wants to 
exit on the failure of the assignment.

Keeping this separation consistent is both more clear and avoids later 
possible mistakes should further "|| exit" clauses be desired.

https://github.com/koalaman/shellcheck/wiki/SC2155

* /usr/bin/makepkg:149:16: error: Double quote array expansions to avoid 
re-splitting elements. [SC2068]

/usr/bin/makepkg:170:14: note: Double quote to prevent globbing and word 
splitting. [SC2086]

General quoting errors:

https://github.com/koalaman/shellcheck/wiki/SC2068

https://github.com/koalaman/shellcheck/wiki/SC2086

* /usr/bin/makepkg:291:32: warning: Use "$@" (with quotes) to prevent 
whitespace problems. [SC2048]

The line in question is:

deplist="$(set +E; check_deps $*)" || exit 1

I'm not sure why this needs to be a single, unquoted string; the 
check_deps function itself uses "$@".

* /usr/bin/makepkg:314:8: note: Use grep -q instead of comparing output 
with [ -n .. ]. [SC2143]

The line in question is:

     if [[ -n $(grep -xvFf <(printf '%s\n' "${current_pkglist[@]}") \
             <(printf '%s\n' "${original_pkglist[@]}")) ]]; then

Instead of an extra -n test, grep -q would do.

https://github.com/koalaman/shellcheck/wiki/SC2143

* /usr/bin/makepkg:323:11: warning: Expanding an array without an index 
only gives the first element. [SC2128]

For example:

                 mapfile -t filename < <(find "$pkgdir" -type f -name $p\*)
                 if [[ $filename ]]; then

It should always clear from the code that arrays are used; if you simply 
want to check if it is empty, ${filename[*]} can be used.

https://github.com/koalaman/shellcheck/wiki/SC2128

* /usr/bin/makepkg:404:1: warning: generate_checksums references 
arguments, but none are ever passed. [SC2120]

/usr/bin/makepkg:2181:2: note: Use generate_checksums "$@" if function's 
$1 should mean script's $1. [SC2119]

This function accounts for arguments, but seems only used without them 
in the GENINTEG if list.

* /usr/bin/makepkg:659:11: note: $/${} is unnecessary on arithmetic 
variables. [SC2004]

https://github.com/koalaman/shellcheck/wiki/SC2004

* /usr/bin/makepkg:666:13: warning: Don't use variables in the printf 
format string. Use printf "..%s.." "$foo". [SC2059]

https://github.com/koalaman/shellcheck/wiki/SC2059

* /usr/bin/makepkg:1138:23: warning: url is referenced but not assigned. 
[SC2154]

There's a few of these; not sure this is an issue since these are 
optional variables in a PKGBUILD and we don't have set -u.

* /usr/bin/makepkg:1224:22: note: Use ./*glob* or -- *glob* so names 
with dashes won't become options. [SC2035]

Though it seems unlikely to compress a file starting in a slash, ./ adds 
some safety; it doesn't impact the used nullglob:

     # when fileglobbing, we want * in an empty directory to expand to
     # the null string rather than itself
     shopt -s nullglob

     LANG=C bsdtar -czf .MTREE --format=mtree \
--options='!all,use-set,type,uid,gid,mode,time,size,md5,sha256,link' \
         "${comp_files[@]}" *

https://github.com/koalaman/shellcheck/wiki/SC2035

* /usr/bin/makepkg:1298:8: warning: Variable was used as an array but is 
now assigned a string. [SC2178]

This seems to be a false positive since filename is an array elsewhere 
in the script, but here it is a local.

* /usr/bin/makepkg:1666:27: error: Braces are required when expanding 
arrays, as in ${array[idx]}. [SC1087]

Shellcheck probably isn't aware of the eval construct.

* /usr/bin/makepkg:1700:11: warning: Remove space after = if trying to 
assign a value (for empty string, use var='' ... ). [SC1007]

The "local foo=" style is only used here, and "local foo" would have the 
same effect.

* /usr/bin/makepkg:1996:18: warning: Use single quotes, otherwise this 
expands now rather than when signalled. [SC2064]

There's a few nested quotes here, I guess it works as intended ...

* /usr/bin/makepkg:2068:10: warning: Exporting an expansion rather than 
a variable. [SC2163]

Probably as intended

* /usr/bin/makepkg:2350:11: warning: Use "${var:?}" to ensure this never 
expands to /* . [SC2115]

Well, makepkg can't run as root since 4.2, but good to add this anyway.

https://github.com/koalaman/shellcheck/wiki/SC2115

Alad