Re: string comparing and if-else character or so descriptive subject here (for reference)

[Eric Blake]

On 01/19/2012 01:08 AM, Parker.Du@alitech.com wrote:
> Description:
>         [String comapartion wrong , execute do not execute (else)'s 
> statements when (if)'s condition is (not true).  ]
> 
> Repeat-By:
>         [I have attach my .sh file, please check.]
> 

This is a poor quality bug report.  You did not technically attach
anything; and your inlined uuencoded tar file contains a directory but
no .sh file in that directory.  Plain text bug reports that focus in on
your problem, instead of sending 7 unrelated files in a
difficult-to-extract-format, along with giving us more details about
what you did, what happened, and why you think it was wrong, so that we
aren't playing guessing games, would be appreciated.  That said, I'll
still try to help.

In your compressed directory, you included a file warn/notify with this
snippet of code, which has several issues:

> for LOGIN in `cat warn_list | sed 's/^.*home.//'`

Useless use of cat.  Also, the regular expression ^.* is redundant; no
need to anchor a search if you are already grabbing as many leading
characters as possible.  Finally, the `` notation, while portable to
non-POSIX shells, is archaic; since you are specifically requesting bash
and are therefore not worried about portability, you might as well use
the POSIX notation of $().  You can get the same results with fewer
processes via:

for LOGIN in $(sed '/.*home.//' < warn_list)

> do
>         echo "LOGIN:" $LOGIN
>         if [ "wenx"==$LOGIN"x" ];

Your syntax is wrong here.  You are testing if the single string
"wenx==$LOGINx" is non-empty, which is always true.  You _meant_ to use:

if [ xwen = "x$LOGIN" ];

or the bash-ism:

if [[ wen == $LOGIN ]];

Then, in your file warn/notify_me, you have these questionable constructs:

> 
>         for PTS in `who | grep $USER | sed 's/2012.*$//' | sed 's/^\S*\s*//'`

No need to waste two processes; any time you see 'grep | sed', you can
almost always rewrite it to be just one sed; 'sed | sed' is not always
collapsible, but in this case it is.  The escapes \S and \s are not
portable, but I'm assuming you don't care about that, given that you are
already restricting yourself to bash.  Use:

for PTS in $(who | sed -n "/$USER/"' { s/2012.*//; s/^\S*\s*//; p; }')

>         do
>                 echo "Hello"  write $USER $PTS
>         done 
> }
> 
> cat `ls -1 ../usage-* | sort | tail -n 1` | sort -n | tail -n  `cat template 
> | wc -l` >top_usage

Useless use of cat, twice.  Useless use of ls and sort (globbing already
gives names in sorted order, and printf as a built-in is more efficient
than ls at listing one name per line).  Why not:

sort -n $(printf %s\\n ../usage-* | tail -n1) | \
  tail -n $(wc -l < template) > top_usage

> cat template top_usage | sort -n | tail -n `cat template | wc -l`|  >warn_list

Useless use of cat, twice.  And you probably didn't want that last |.
Why not:

sort -n template top_usage | tail -n $(wc -l < template) > warn_list

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature