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

[Santi Béjar]

On Wed, Apr 14, 2010 at 12:39 AM, Cristian Ionescu-Idbohrn
<cristian.ionescu-idbohrn at axis.com> wrote:
> 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.

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.

>
>> 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.

But you need a changelog nevertheless.

>> And some comments about the changes.
>>
>> This change:
>>
>> -    CATEGORY=$1
>> -    MSG=$2
>
> You might have misses these comments:

No I didn't miss them.

>
> +    # $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.

It is not marginally, it is 10 lines out of 25 removed lines.

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

Yes, you did and I saw it. I see it could be better but I don't see
big benefits.

>
>> -    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" ...

> 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.

I was not talking about the "x$var" syntax. But tradition?

>  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.

>  In other
> words, isn't this readable and less bloaty?
>
> [ "$ENABLE_OSD" = no ] && return

Yes.

>
> 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 :(

There is more commands after this line, so the status code does not matter.
But you have a point, it is better to catch all error conditions.

>
>> -     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.

OK, thanks for the explanation.

You can add it to the commit message so other can learn it too and
think about it in the future.

>
> 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.
>

Thanks, again, for the explanation.

Santi