[Ovirt-devel] [PATCH trivial] Create developer appliances under Ubuntu v2

[Jim Meyering]

"Jeff Schroeder" <jeffschroed at gmail.com> wrote:
> On Sat, Jun 28, 2008 at 1:34 AM, Jim Meyering <jim at meyering.net> wrote:
...
>>>  elif [ -e /etc/debian_version ]; then
>>>      # Works in Ubuntu 8.04. Still needs testing in Debian
>>> -    PACKAGES="libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer"
>>> -    CHECK=$(dpkg -l $PACKAGES &> /dev/null; echo $?)
>>> -    KVM_BINARY=/usr/bin/kvm
>>> +    PACKAGES='libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer'
>>> +    dpkg -l $PACKAGES > /dev/null 2>&1 && DEPS_MET=1
>>
>> The idiom I suggested, "; some_var=$?", guaranteed that
>> $some_var would be defined.
>> Since you've opted to use " && DEPS_MET=1",
>> it's your responsibility to define DEPS_MET when they're not met.
>> Otherwise, if DEPS_MET=1 happens to be set in the environment
>> (yeah, not likely), it will stay that way, even when the dependencies
>> are not met.
> The point on && was to set it to 1 if the dependencies are actually met.
> I misunderstood your previous email. It was just the subshell, not ; ... $?
> In doing it this way, "unset DEPS_MET" should have been first done.

DEPS_MET=0 is clearer and slightly more portable than using unset.
In addition, if you avoid using unset variables, you can skip
the double quotes and unsightly ${var:-...} syntax.
Plan for readability.

...
>>> -if [ $CHECK -ne 0 ]; then
>>> +if [ ${DEPS_MET:-0} -ne 1 ]; then
>>
>> Then you can use a cleaner comparison:
>>
>>  if [ $DEPS_MET = 0 ]; then
>>
>> By the way, please don't use "-ne" for arithmetic comparisons.
>> "=" works fine and its shorter and more readable.
>
> You mean this, right? If we set DEPS_MET=$? 0 is a true return and
> nonzero is false.

No, because 0 implies false, which is at odds with the semantics
of a name like DEPS_MET.  That would be misleading.  If you prefer
to use "; var_name=$?" as I originally suggested, you'll have to
choose a name with the opposite meaning.

> if [ $DEPS_MET != 0 ]; then
>
> -ne, -lt, -gt are all specifically for integer comparison. The purist
> in me is going to have to
> politely disagree on this one. != is more for string comparison but
> most shells use it for
> integer comparison also ontop of -ne.

In this case, there are two values you care about: 0 and 1.  Requiring
the shell to perform a string-to-integer conversion before comparing
is pointless, and in general counterproductive.  Do that only if you
require that comparisons like 00001 -eq 1 and '   42    ' -eq 042 be true.
In nearly every case I've seen in far too many years of shell programming,
using "=" is perfectly fine, and preferred because it's more readable.
Contrast the number of uses of -eq and -ne in autoconf-generated code
to the number of uses of "=".

> What is wrong with the brace expansion? Hating to pull out the
> [ ${DEPS_MET:-0} -ne ...

It's unnecessarily obtuse, and there is no benefit to using it,
once you've adjusted.