* Stefano Lattarini wrote on Thu, Nov 11, 2010 at 09:10:39PM CET: > On Thursday 11 November 2010, Ralf Wildenhues wrote: > > * Stefano Lattarini wrote on Thu, Nov 11, 2010 at 02:52:06PM CET:
> > > @@ -228,11 +229,16 @@ do > > > (echo foo >> $priv_check_temp) >/dev/null 2>&1 > > > overwrite_status=$? > > > rm -f $priv_check_temp > > > - test $overwrite_status = 0 && exit 77 > > > + if test $overwrite_status = 0; then > > > > -eq seems more appropriate than = (more instances below). > OK. Sorry for making you fix bugs that were there before BTW. > > > + echo "$me: this test shouldn't be run as root" > > > > Please >&2, several instances. > I used stdout for "consistency" with messages like: > echo "$me: running $CC --version" > echo "$me: running python -V" But those aren't errors. I know it's a close call for the above messages, but for error messages, stderr is definitely right. > But I have no strong feelings on this matter, so I went along with > the ">&2" redirections throughout. Thanks. > > > perl-threads) > > > - # Skip with Devel::Cover: it cannot cope with threads. > > > - test "$WANT_NO_THREADS" = yes && exit 77 > > > + if test x"$WANT_NO_THREADS" = x"yes"; then > > > > no need to quote `yes', and in practice, no need for x prefixing either, > > I would guess. Do you think we need to take care of malicious users? > No, it's just for consistency. Because sometimes the idiom > test x"$foo" = x"bar" > is indeed required, I prefer to use it everywhere, instead of asking myself > at every turn "do I need to care for white spaces in $foo here?" or "do I > need to care for leading hypens in $foo here?". That's all. Well, the quotes around bar are never required (except in Libtool and there only because out testsuite is too dumb to not flag the false positives). > Do toy want me to use: > test $WANT_NO_THREADS = yes > anyway? No. You need to put double quotes around "$WANT_NO_THREADS", because it can be empty. The rest is not a big deal either way, but if you want to avoid asking yourself at every turn, then I just suggest not changing existing code at that turn unless you see a good reason. ;-) > > > tex) > > > # No all versions of Tex support `--version', so we use > > > # a configure check. > > > - test -n "$TEX" || exit 77 > > > + if test x"$TEX" = x; then > > > > test -n is portable here and more concise, $TEX will never start with > > a '-' character or be equal to '=' for any sane user. So let's please > > keep that. > Well, we should use `test -z' at least, since the semantic of the check > has been reversed. Also, `test -z' can sometimes be problematic too, and > I tend to avoid it altogheter for the same "consistency" resons stated > above. > > Do you want me to use `tezt -z' anyway here? Sure. > > > @@ -285,6 +298,37 @@ do > > > esac > > > done > > > > The rest of the patch from here on below seems to only transpose testing > > of $required and testing of some other variable. In essence for the > > default case it turns one case statement into three (thus a minor > > slowdown), little code into more code, and I fail to see the advantage > > of the new ordering. What is the intention here? > Giving precise resons about why the test was skipped. Ah; that's a good reason. Thanks. > > > - case $testsrcdir,$testbuilddir in > > > - *\ * | *\ *) exit 77;; > > > + *' libtool '*|*' libtoolize '*) > > > + if test x"$libtool_found" != x"yes"; then > > > > The old code was perfectly well quoted: in > > test $libtool_found = yes > > you would reliably and helpfully get a shell error if $libtool_found was > > erroneously unset. Also, we ensure that it could not start with '-'. > True, but see the "consistency" reasons stated above. Do you want me to > revert to the older (lack of) quoting anyway? Yes, please. Think of it as a check of your other changes in the code. ;-) Thanks, and please commit the patch when items are addressed, Ralf