Excerpts from Marco Pesenti Gritti's message of 2011-12-07 18:01:05 +0100: > > Signed-off-by: Marco Pesenti Gritti <ma...@marcopg.org>
Thanks for the patch! It contains a few issues that prevent me for pushing it as-is. It would have been easy to fix them up prior to pushing, but I'd like to use this opportunity to point out some mistakes that happen rather often. Shell scripts are quite tricky to get right. [autogen.sh] > @@ -1,4 +1,13 @@ > #!/bin/sh > + > +test -n "$srcdir" || srcdir=`dirname "$0"` You miss one layer of quoting. Special characters in the directory name can cause you to do unexpected things, ranging from merely not working up to destroying user data (unlikely, but possible). Better: test -n "${srcdir}" || srcdir="$(dirname "$0")" Both $() and ${} are defined by POSIX [1,2]. $var works and is equivalent to ${var} in simple cases like the ones we're talking about. $(command) and `command` differ w.r.t. escaping and nesting. The differences shouldn't matter in this particular case, but it's a good habit to adopt. ${} and $() improve clarity and are significantly less tricky to use in more complex cases. > +test -n "$srcdir" || srcdir=. Shouldn't we assign "$(pwd)" in this case? > + > +olddir=`pwd` Again, we need to quote the result of the command: olddir="$(pwd)" And maybe builddir is a better name for olddir? An updated patch would be appreciated, but I can post it as well. Just tell me what you prefer. Sascha [1] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02 [2] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_03 -- http://sascha.silbe.org/ http://www.infra-silbe.de/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel