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

[Damyan Ivanov]

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

-=| 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. Having 
the patches extracted using 'git format-patch' is preferred as this 
would include the commit messages and authorship.

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

I'd say that changelog entries aren't obligatory as good commit 
messages can be used automaticaly to fill those.


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. Consider pressing and 
holding down a volume key. Events come several per second. According 
to the comments in the default file, dash is significantly slowed down 
by comments.

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

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

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

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

> > -	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 find the former 
a bit more readable, but otherwise consider it a mater of taste.

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/attachments/20100414/6393019f/attachment.pgp>