ShellCheck warning: "Iterating over ls output is fragile. Use globs. [SC2045]"

Question

I am getting a ShellCheck warning [SC2045] for the second line in the code below. Is it fine to ignore it as I am making sure the directory is not empty before I try the last ls?

 if [ "$(ls -A "$retryDir")" ]  ; then
    for thisRetryFile in $(ls "$retryDir"/*.tar.gz) ; do
        scp -o ConnectTimeout=30  "$thisRetryFile"  \             
              "$remoteUser@$remoteHost:$remotePath" >> "$BACKUPLOG"
    done
 fi

UPDATE: After reading the post comments. I have changed the line to:

for thisRetryFile in "$retryDir"/*.tar.gz ; do

This has removed the warnings.

See [BashPitfall #1](http://mywiki.wooledge.org/BashPitfalls#for_i_in_.24.28ls_.2A.mp3.29). — randomir, Dec 07 '17 at 19:35
Empty directories aren't the only problem. Filenames with spaces are also a problem. — Barmar, Dec 07 '17 at 19:55
[why not parse ls](https://unix.stackexchange.com/questions/128985/why-not-parse-ls) — Barmar, Dec 07 '17 at 19:57

janos · Accepted Answer · 2017-12-07T20:40:05.453

11

Use the loop with a glob, and also set nullglob to avoid executing scp in case the pattern doesn't match anything. And you don't need the outer if condition either, since the for with nullglob effectively takes care of that:

shopt -s nullglob

for thisRetryFile in "$retryDir"/*.tar.gz; do
    scp -o ConnectTimeout=30  "$thisRetryFile" \
          "$remoteUser@$remoteHost:$remotePath" >> "$BACKUPLOG"
done

If you want to catch the case when no file matches the pattern, you can write like this, without using shopt -s nullglob:

for thisRetryFile in "$retryDir"/*.tar.gz; do
    if [ -f "$thisRetryFile" ]; then
        scp -o ConnectTimeout=30  "$thisRetryFile" \
            "$remoteUser@$remoteHost:$remotePath" >> "$BACKUPLOG"
        break
    else
        echo "warn: no tar.gz file in dir: $retryDir"
    fi
done

edited Dec 07 '17 at 20:40

answered Dec 07 '17 at 19:50

janos

120,954
29
226
236

That's really a matter of opinion. The really safe alternative is to *handle* the case of no matches in every location where you use a wildcard. Then whether you use nullglob or something else will be case by case; some things are harder when your argument might disappear. – tripleee Dec 07 '17 at 20:33
@AndyM yes, like that – janos Dec 07 '17 at 23:00

score -2 · Answer 2 · answered Dec 07 '17 at 19:37

-2

This is safer. Try it.

 if [ "$(ls -A "$retryDir")" ]  ; then
    for thisRetryFile in ${retryDir}'/*.tar.gz' ; do
        scp -o ConnectTimeout=30  "$thisRetryFile"  "$remoteUser@$remoteHost:$remotePath" >> "$BACKUPLOG"
    done
 fi

Regards!

answered Dec 07 '17 at 19:37

Matias Barrios

4,674
3
22
49

1

No, this still violates [don't use `ls` in scripts](https://unix.stackexchange.com/questions/128985/why-not-parse-ls). – tripleee Dec 07 '17 at 20:31

ShellCheck warning: "Iterating over ls output is fragile. Use globs. [SC2045]"

2 Answers2