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

[Cristian Ionescu-Idbohrn]

On Wed, 14 Apr 2010, Damyan Ivanov wrote:

> Disclaimer: The following is my own opinion and I am not the only who
> can apply the patches.

The patches I sent are mostly intended as ideas and attempts to improve
the code.

> -=| Cristian Ionescu-Idbohrn, Wed, Apr 14, 2010 at 12:39:21AM +0200 |=-
> > On Tue, 13 Apr 2010, Santi Béjar wrote:
> > > 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?
>
> Ideally as an attachment, using 'inline' content-disposition.

I'll than have to hack my mail client, or use another ;)

> Having the patches extracted using 'git format-patch' is preferred as
> this would include the commit messages and authorship.

Right.  My git fu is limited.

> A note about comments and verification of function arguments. These
> are kept at minimum because some of the scripts are used in each ACPI
> event and need to be as quick as possible.

Yes.  Performance is important.  I do understand that.

> Maybe we can ship the scripts without comments in the binary package,
> as is done with the default file. That would make hacking of the
> installed scripts harder though :/

Right.  Maybe in a later phase.  Ironing out bugs and improving
performance at this stage is more important, IMO.

> > Another observation is the 'notify' function seems to require
> > 3 arguments, but never validates that.  Never rely user input is
> > correct.
>
> There is no user input. The only "user" are the other scripts in the
> package. Is some of them not supplying three arguments? Of course,
> validation can still be made, but I think performance will suffer. And
> it works now, doesn't it?

Seems to.  I don't remember all the details, but I think I found (and
commented on) questionable use of positional parameters $4 and $3.

> > > -    if [ "x$ENABLE_OSD" = "xno" ]; then
> > > -        return
> > > -    fi
> > > +    [ "$ENABLE_OSD" != no ] || return
> > >
> > > I don't like so many negative logic,
>
> I am used to that from makefiles, where it is inconvenient to split
> commands on multiple lines, and commands are required to return true.
> This is shell, though, so the readability of a multi-line if is
> something I value more.

That's ok, although parsing multiline conditions affects performance.
And then again, there are a lot of things that must be considered.
Finding the best balance isn't trivial.

> > 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?
>
> It is. Trimming your nails also helps you lose weight :)

Fair comparison :)

> > > -	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
>
> I'd like to see some numbers backing this claim.

I'll see if I find the time to do that at some point.

> I find the former a bit more readable, but otherwise consider it a mater
> of taste.

Fair enough.

> Personaly, I am much happier applying patches if they address problems
> somebody is experiencing. Is the code so "bloated" and "ugly" as to
> prevent contribution?

No.  But it's easier to work on non-bloated and less ugly code :)
I recall I pointed out two bugs and provided patch code already.


Cheers,

-- 
Cristian