Fix two very broken tests for null strings in archive_build.sh

Fix two very broken tests for null strings in archive_build.sh

Change-Id: I7c5d251ab1ae020c3ae8a10ca017c34a02eff893

BUG=None
TEST=./archive_build.sh --test_mod
TEST=./archive_build.sh --test_mod --from=../build/images/x86-generic
TEST=./archive_build.sh --board=x86-generic --gsutil_archive=/tmp --gsutil=echo
TEST=./archive_build.sh --board=x86-generic --gsutil_archive=/tmp --gsutil=echo --gsd_gen_index=echo

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=e1010d8

[jrbarnette, 2010-10-26 01:11:24]

I discovered this bug while (belatedly) reviewing this CL:
    http://codereview.chromium.org/3978005/show

There are two tests for null strings in the script that don't
quote the string to be tested.  It's wrong.  It's evil.  It must
be set right.

The reviewers selected are the perpetrators who failed to
notice this the first time around.

Ideally, someone will tell me how to test this, but I won't
stand on ceremony if no one steps forward...

[Nick Sanders, 2010-10-26 01:26:18]

lgtm

good catch! I looked right at it and didn't notice.

Possibly we could just remove --from in a future CL?

http://codereview.chromium.org/4153001/diff/1/2
File archive_build.sh (right):

http://codereview.chromium.org/4153001/diff/1/2#newcode70
archive_build.sh:70: if [ -z "$DEFAULT_USED" ]
You can test this by specifying --from=<something>

[jrbarnette, 2010-10-26 16:50:59]

On 2010/10/26 01:26:18, Nick Sanders wrote:
> lgtm
> 
> good catch! I looked right at it and didn't notice.
> 
> Possibly we could just remove --from in a future CL?
> 
> http://codereview.chromium.org/4153001/diff/1/2
> File archive_build.sh (right):
> 
> http://codereview.chromium.org/4153001/diff/1/2#newcode70
> archive_build.sh:70: if [ -z "$DEFAULT_USED" ]
> You can test this by specifying --from=<something>

I tested with and without the --from.  As a practical matter, the broken
test would always return the right answer - just not for the right reason.  :-)

I'm looking to see if I can exercise the second code path without actually
invoking gsutil; if I can't, I'll push, on strength of the obviousness of
this fix...

[David James, 2010-10-26 17:06:51]

http://codereview.chromium.org/4153001/diff/1/2
File archive_build.sh (right):

http://codereview.chromium.org/4153001/diff/1/2#newcode70
archive_build.sh:70: if [ -z "$DEFAULT_USED" ]
This code actually works fine as is, but it's probably taking advantage of a
bug. Interestingly, [ -z ] and [[ -z ]] return different results. So your update
is much better, thanks :)

http://codereview.chromium.org/4153001/diff/1/2#newcode247
archive_build.sh:247: if [ "$FLAGS_gsd_gen_index" != "" ]
The original code happens to work as is, but is nasty syntax and prints error
messages.

Your update is better. But how about, more idiomatically, [ -n
"$FLAGS_gsd_gen_index" ] ?

[jrbarnette, 2010-10-26 17:24:06]

On 2010/10/26 17:06:51, David James wrote:
> http://codereview.chromium.org/4153001/diff/1/2
> File archive_build.sh (right):
> 
> http://codereview.chromium.org/4153001/diff/1/2#newcode70
> archive_build.sh:70: if [ -z "$DEFAULT_USED" ]
> This code actually works fine as is, but it's probably taking advantage of a
> bug. Interestingly, [ -z ] and [[ -z ]] return different results. So your
update
> is much better, thanks :)
> 
It works in the sense that this code behaves the same with or without
the quotes.  However, that's not generally true of -z, and with -n it's
just wrong, so (to cite examples):
  [ -z $FUBAR ] is equivalent to [ -z "$FUBAR" ]
  [ -z $FUBAR -o $FUBAR != "A" ] causes a syntax error when FUBAR is empty
  [ -n $FUBAR ] can never be false(!)


> http://codereview.chromium.org/4153001/diff/1/2#newcode247
> archive_build.sh:247: if [ "$FLAGS_gsd_gen_index" != "" ]
> The original code happens to work as is, but is nasty syntax and prints error
> messages.
> 
I don't consider "prints a syntax error as a side effect" to be exactly
"working" :-)
But as long you say it's better, I won't quibble (any more).

> Your update is better. But how about, more idiomatically, [ -n
> "$FLAGS_gsd_gen_index" ] ?

An updated CL is on the way, after testing is complete.

[jrbarnette, 2010-10-26 17:26:55]

> just wrong, so (to cite examples):
>   [ -z $FUBAR ] is equivalent to [ -z "$FUBAR" ]
>
On reflection, I've realized this statement just ain't so:
[ -z $FUBAR ] and [ -z "$FUBAR" ] are different if FUBAR
is a nonempty string of white space characters (only).

[jrbarnette, 2010-10-26 17:48:19]

On 2010/10/26 16:50:59, jrbarnette wrote:
[ ... ]
> I'm looking to see if I can exercise the second code path without actually
> invoking gsutil; if I can't, I'll push, on strength of the obviousness of
> this fix...

Testing the gsutil code path turned out to be easy; the tests have passed,
and the description shows the various test command lines I used.  Both
changed code paths were exercised, both for true and false conditions.

At this point, I'm ready to go.  Y'all can have a half hour for any last minute
extra observations, after which I push.

Thanks!