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