if then statment and for each bash script

Question

I'm attempting to write a bash script to check multiple files for a string, then if the string is found to remove it. Here is what I have, which I thought would work, but is only partly. The entries in the files are being removed, but running this a second time, I'm not getting the "nothing to do..." message.

#!/bin/bash
files=(
        '/etc/rsyslog.conf'
        '/etc/rsyslog.d/remote.conf'
        '/etc/rsyslog.d/01-remote.conf'
)

tmpcheck="for f in ${files[*]}; do  cat $f | grep blah | wc -l; done"
#for f in ${files[*]}; do  cat $f | grep collector.acuity.com | wc -l; done

if [[ "$tmpcheck" != 1 ]];then
for f in "${files[@]}";do
                echo -e "Removing blah from $f"
                sed -i "/blah/d" "$f"
        done
                echo -e "Restarting rsyslog service"
                systemctl restart rsyslog.service
else
        echo -e "Nothing to do, blah has been removed from $f"
fi

Any help would greatly help.

ctx · Answer 1 · 2020-09-05T21:21:13.340

I think you want to check for blah inside the for loop. There is no need for the tmpcheck variable. Instead you can use $restartrsyslog to restart rsyslog only once:

files=(
    '/etc/rsyslog.conf'
    '/etc/rsyslog.d/remote.conf'
    '/etc/rsyslog.d/01-remote.conf'
)

restartrsyslog=

for f in "${files[@]}";do
    if grep -q blah "$f"; then
        echo -e "Removing blah from $f"
        sed -i "/blah/d" "$f"
        restartrsyslog=yes
    else
        echo -e "Nothing to do, blah has been removed from $f"
    fi
done

if [[ -n $restartrsyslog ]] ; then
    echo -e "Restarting rsyslog service"
    systemctl restart rsyslog.service
fi

fra-san · Answer 2 · 2020-09-05T15:59:04.917

The main issue in your code (the assignment to tmpcheck) has been pointed out in other answers. Here is a slightly different approach, assuming that printing out the names of the modified/unmodified files is not mandatory:

if grep -q -- blah "${files[@]}"
then
    sed -i -e '/blah/d' -- "${files[@]}"
    systemctl restart rsyslog.service
else
    printf '%s\n' 'Nothing to do'
fi

The main ideas here are:

directly use a command's (grep) exit status in if compound commands instead of storing its output and testing it later, unless necessary;
calling utilities in a loop is less efficient; better to call them with several files as arguments whenever possible.

Also, while these are not issues in the code you show:

adding the end-of-options marker (--) protects against uncommon file names;
printf is safer than echo, especially when printing the result of expansions.

score 0 · Answer 3 · answered Aug 28 '20 at 16:25

Your tmpcheck variable is being set to a command not the output of that command, it will never equal 1 so that expression will always evaluate to true. You should use the $( ... ) construct to substitute the output of commands. You also don't need to loop over the files but if you do you should not use ${files[*]} but use ${files[@]} instead. Additionally there is no need to cat a single file and pipe into grep as grep can read files, and there is no need to pipe grep into wc -l as grep has a -c option to count the results. Finally why are you checking that tmpcheck doesn't equal 1? What if it equals 2 or more?

Does this work for you?

#!/bin/bash
files=(
    '/etc/rsyslog.conf'
    '/etc/rsyslog.d/remote.conf'
    '/etc/rsyslog.d/01-remote.conf'
)

tmpcheck=$(cat "${files[@]}" | grep -c blah)

if (($tmpcheck>=1)); then
    for f in "${files[@]}";do
        echo -e "Removing blah from $f"
        sed -i "/blah/d" "$f"
    done
    echo -e "Restarting rsyslog service"
    systemctl restart rsyslog.service
else
    echo -e "Nothing to do, blah has been removed from $f"
fi

if then statment and for each bash script

3 Answers3