[Debian-eeepc-devel] [patch] proposal: /etc/acpi/lib/notify.sh

[Cristian Ionescu-Idbohrn]

On Wed, 14 Apr 2010, Santi Béjar wrote:

> On Wed, Apr 14, 2010 at 12:39 AM, Cristian Ionescu-Idbohrn <cristian.ionescu-idbohrn at axis.com> wrote:
> >
> > Iterations like that tend to take longer.
>
> I agree if the patch is small, but this is not:
>
> $ diffstat notify.sh.1.patch
>  notify.sh |   53 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
>
> > It might look much more than it really is, but changes are quite
> > trivial.
>
> A lot of trivial changes is not a trivial change.

Gotcha.

> > True.  But, as I said, changes are trivial.
>
> But you need a changelog nevertheless.

Alright.

> >> is making the whole patch bigger (and mixed) and I don't see the
> >> benefits, apart from using $1, $2,....

Benefits, performancewise are the shell does not have to allocate space
for new variables and uselessly copy around values.

> > Marginally bigger, yes.
>
> It is not marginally, it is 10 lines out of 25 removed lines.

You keep banging on the patch size, not on what it does.

> >> -    if [ -n "$4" -o \( -n "$3" -a "$3" != 'fast' \) ]; then
> >> +    if [ "$4" -o \( "$3" -a "$3" != fast \) ]; then
> >>
> >> are not equivalent, if $3 or $4 are -n you get a syntax error.
> >
> > Can't see that happen.  Could you provide a simple example that shows the
> > syntax error you mention?  Tested with bash and dash.  Please check this
> > yourself.  The positional parameters _are_ quoted.  Did you miss that?
>
> Yes, I missed that, but
>
> > Consider this example:
> >
> > ,----
> > | #!/bin/dash
> > |
> > | set -e
> > | set -x
> > |
> > | [ "$2" -o \( "$1" -a "$1" != fast \) ]
> > `----
>
> ./script -n -n
> + [ -n -o ( -n -a -n != fast ) ]
> [: 1: -o: unexpected operator
>
> and works with [ -n "$2" ...

Ouch...

Thanks.  I think I know better now.
I never use that construct and I advise against using it.
This is what I usually use:

[ "$2" ] || { [ "$1" ] && [ "$1" != fast ]; }

as it's easily read (clearly separated logic) and less error prone.

> >> I prefer:
> >>
> >> [ "x$ENABLE_OSD" = "xno" ] && return
> >
> > Why?  All modern shells do the right thing without that ancient 'x'.
> > That's bloat.
>
> I was not talking about the "x$var" syntax. But tradition?

So you're a traditionalist? ;)

> >  And why do you need to quote something (non-null) that is
> > not going to expand to anything different than it already is?
>
> If it is agreed to fix it, OK, but I don't see the point in fixing this.

It's nothing wrong with it, it's not a fix, it's cleanup.
Do things you must do and don't do what you don't need to.


Cheers,

-- 
Cristian