Hi, On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> wrote: > > +# Debian and derivatives use x-www-browser or www-browser to set > +# the default browser for the system > +if test -z "$browser" ; then > + wwwbrowser="/usr/bin/www-browser" > + if test -n "$DISPLAY"; then > + wwwbrowser="/usr/bin/x-www-browser $wwwbrowser" > + fi > + for i in $wwwbrowser; do > + if test -x $i ; then > + verstring="$($i --version 2> /dev/null | head -n 1)" > + browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)" Stupid questions: Did you check that all the browser we support accept the --version option? What if we add support for a new one that doesn't ? Shouldn't we add something like : test -z "$browser" && browser="$(echo $i | cut -f1 -d' ' | tr A-Z a-z)" And are you sure that when they support --version, the first word of the output is better than "$(echo $i | cut -f1 -d' ' | tr A-Z a-z)"? What if we add support for a new one? > + case "$browser" in > + mozilla) > + browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)" > + ;; > + google) > + browser="google-chrome" > + ;; > + esac > + if valid_tool "$browser" ; then valid_tool "$browser" is called once here... > + browser_path="$i" > + break > + fi > + fi > + done > +fi > + > if test -z "$browser" ; then > if test -n "$DISPLAY"; then > browser_candidates="firefox iceweasel google-chrome chrome chromium konqueror opera seamonkey iceape w3m elinks links lynx dillo" > @@ -129,7 +156,7 @@ if test -z "$browser" ; then > fi > done > test -z "$browser" && die "No known browser available." > -else > +else if test -z "$browser_path"; then > valid_tool "$browser" || die "Unknown browser '$browser'." ...valid_tool "$browser" is called again here if it failed above, and here we die, so isn't it clearer to die as soon as we call it and it fails? Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html