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

[Santi Béjar]

On Tue, Apr 13, 2010 at 12:03 AM, Cristian Ionescu-Idbohrn
<cristian.ionescu-idbohrn at axis.com> wrote:
> See attached.

Just some general comments (sorry if you had them in mind for the final patch)

Do one thing in each patch, so the reviewer/maintainer can apply/drop
them individually and it is easier to comment/review.

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

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)

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

And some comments about the changes.

This change:

-    CATEGORY=$1
-    MSG=$2

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

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

-    if [ "x$ENABLE_OSD" = "xno" ]; then
-        return
-    fi
+    [ "$ENABLE_OSD" != no ] || return

I don't like so many negative logic, I prefer:

[ "x$ENABLE_OSD" = "xno" ] && return

-	commands \
-		| su "$user" -c "$GOSDC -s --dbus"
+	commands |
+		su "$user" -c "$GOSDC -s --dbus"

Why did you change the continuation sequence?

HTH,
Santi