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

[Cristian Ionescu-Idbohrn]

On Tue, 13 Apr 2010, Santi Béjar wrote:

> On Tue, Apr 13, 2010 at 12:03 AM, Cristian Ionescu-Idbohrn
> <cristian.ionescu-idbohrn at axis.com> wrote:
> > See attached.
>
> Do one thing in each patch, so the reviewer/maintainer can apply/drop
> them individually and it is easier to comment/review.

Yes.  That's one of the more popular advices.  But there's a back side to
it.  Iterations like that tend to take longer.  It might look much more
than it really is, but changes are quite trivial.

> Send it in-line so we can easily comment your changes.

Sure, I could do that and then find out others prefer attachments, because
their mail servers/clients mangle the contents (wrap lines and damage
whitespace).  What do the maintainers prefer?

> Provide a changelog (and changes to debian/changelog). I see it is
> just a proposal, but still it is better if you explain why this
> changes are good (even if you already wrote them in another mail)

True.  But, as I said, changes are trivial.

> Hint: if you have to explain more than one thing in the changelog you
> probably need to split the patch.

See above.

> And some comments about the changes.
>
> This change:
>
> -    CATEGORY=$1
> -    MSG=$2

You might have misses these comments:

+    # $1 - category
+    # $2 - message
+    # $3 - ???
+    # $4.. - ???

specially this one:

+    # positional parameters never change during the course of this function

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

Marginally bigger, yes.  But those variables are not needed.  And then, I
documented the arguments, didn't I?

> -    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?
Consider this example:

,----
| #!/bin/dash
|
| set -e
| set -x
|
| [ "$2" -o \( "$1" -a "$1" != fast \) ]
`----

Another observation is the 'notify' function seems to require 3 arguments,
but never validates that.  Never rely user input is correct.

> -    if [ "x$ENABLE_OSD" = "xno" ]; then
> -        return
> -    fi
> +    [ "$ENABLE_OSD" != no ] || return
>
> I don't like so many negative logic,

I think I know what you mean.  Still, the original is bloat.

> I prefer:
>
> [ "x$ENABLE_OSD" = "xno" ] && return

Why?  All modern shells do the right thing without that ancient 'x'.
That's bloat.  And why do you need to quote something (non-null) that is
not going to expand to anything different than it already is?  In other
words, isn't this readable and less bloaty?

[ "$ENABLE_OSD" = no ] && return

You still have a small problem there.  If $ENABLE_OSD is anything else
than 'no', the 'return' command will not be executed and the status code
'$?' result of the condition will be '-ne 0'.  This:

[ "$ENABLE_OSD" != no ] || return

says "it's ok with $ENABLE_OS being different from 'no' and do nothing
($? -eq 0), but if it is 'no', execute the 'return' command.  The '||'
operator may even be followed by a command list, and the status code
comming out of that is usually more interesting than the one resulting
from a condition we don't care much about.

In general, what you should try to do is to ensure status code '$?' is
success before executing the next command.  Error handling is often
neglected in shell scripts, unfortunatelly.  And that generates bugs,
harder to track down and reproduce.  Everything seems to be fine until you
stick in the 'errexit' (-e; special rules; see man page), either on the
command line, shebang line or inside the script:

	set -e

A can of worms opens up :(

> -	commands \
> -		| su "$user" -c "$GOSDC -s --dbus"
> +	commands |
> +		su "$user" -c "$GOSDC -s --dbus"
>
> Why did you change the continuation sequence?

Because shell is an iterpreted language.
Because parsing shell commands in real time is expensive.  This:

	foo \
		| bar

is subtilely more expensive than this:

	foo |
		bar

The shell parsed the 'foo' command (in later form) and it's done with it
(does not need to parse any further, looking for arguments), as '|' is a
natural command separator (as ';' is) and now knows about the pipe too.
In the earlier form, the shell needs to read another line to find out the
command already ended.

Anyway, the rest of the patch proposal removes bloaty quotes, declares
some local variables and documents the global '$user' variable ( that
'detect_x_display' behaviour needs to be changed, IMO; 'detect_x_display'
sets another global variable, 'home', too).  Not too controversial patch,
I would have hoped.

There are other things (missing error handling in various places) that
need some more careful thought and need to be addressed.  But the package
does not provide a consistant script error handling framework, yet.


Cheers,

-- 
Cristian