On Tue, Apr 13, 2010 at 12:03 AM, Cristian Ionescu-Idbohrn <[email protected]> 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 _______________________________________________ Debian-eeepc-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/debian-eeepc-devel
