Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison

David Jack Olrik [off-list ref] writes:

On 22/08/2007, at 15.23, Uwe Kleine-König wrote:

quoted

quoted

You'd then need to check against 2 instead of 1, which I find less
obvious as we are testing for a '/' at the begining of the string.

If I understood the problem right you only need to test for the exit
code, that is the program test is not required at all.

Ah, yes that's true. The following should make it more clear that we are
looking at the first character.

    if expr "$httpd_only" : "\/" >/dev/null

Then why not avoid the fork with something like:

	case "$httpd_only" in
        /*)
                ... begins with slash ...
	esac

By the way, I do not know if the use of "which" there is
portable.  Have Solaris folks tried this program ever?

How about doing it this way?

 * No need to use "cut"; just let IFS do its job.

 * No need to use "which"; we will need to do a discovery with
   custom paths anyway, so use the same logic for discovery from
   the normal $PATH as well.

 * The original code wanted to allow the httpd command that
   comes to the function to have arguments and preserve it but
   did so only for the case where the command was not specified
   with a full pathname.  Allow it for full-path case as well
   for consistency.

 * The original code returned without checking for error when
   httpd was specified without a full path, and checked and
   complained when it was.  Check the error exit for both cases.

---

 git-instaweb.sh |   42 ++++++++++++++++++++++++++----------------
 1 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index b79c6b6..a17c9b3 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh

@@ -29,32 +29,42 @@ test -z "$browser" && browser='firefox'
 test -z "$port" && port=1234
 
 start_httpd () {
-	httpd_only="`echo $httpd | cut -f1 -d' '`"
-	if test "`expr index $httpd_only /`" -eq '1' || \
-				which $httpd_only >/dev/null
-	then
-		$httpd $fqgitdir/gitweb/httpd.conf
-	else
+	set x $httpd
+	shift
+
+	httpd=
+	case "$1" in
+	/*)
+		: full path -- no discovery
+		httpd="$1"
+		;;
+	*)
 		# many httpds are installed in /usr/sbin or /usr/local/sbin
 		# these days and those are not in most users $PATHs
-		for i in /usr/local/sbin /usr/sbin
+		paths="$PATH:/usr/sbin:/usr/local/sbin"
+		oIFS="$IFS"
+		IFS=:
+		for p in $paths
 		do
-			if test -x "$i/$httpd_only"
+			if test -x "$p/$1"
 			then
-				# don't quote $httpd, there can be
-				# arguments to it (-f)
-				$i/$httpd "$fqgitdir/gitweb/httpd.conf"
-				return
+				httpd="$p/$1"
+				break
 			fi
 		done
-		echo "$httpd_only not found. Install $httpd_only or use" \
-		     "--httpd to specify another http daemon."
+		IFS="$oIFS"
+		;;
+	esac
+	if test -z "$httpd"
+	then
+		echo "$1 not found. Install $1 or use --httpd to specify another http daemon."
 		exit 1
 	fi
-	if test $? != 0; then
+	shift
+	"$httpd" "$@" "$fqgitdir/gitweb/httpd.conf" || {
 		echo "Could not execute http daemon $httpd."
 		exit 1
-	fi
+	}
 }
 
 stop_httpd () {

Hello Junio,

Junio C Hamano wrote:

By the way, I do not know if the use of "which" there is
portable.  Have Solaris folks tried this program ever?

I don't count myself to "Solaris folks", even though I still use it to
read and write my email.  But anyhow I know some of the pitfalls...

	login@~ > /bin/bash --version
	GNU bash, version 3.00.16(1)-release (sparc-sun-solaris2.10)
	Copyright (C) 2004 Free Software Foundation, Inc.

	login@~ > /bin/bash

	zeisberg@login ~$ which httpd && echo successful
	no httpd in /home/zeisberg/bin /home/zeisberg/usr/bin /opt/bin
	/usr/local/graphics/bin /usr/local/gnu/bin /usr/local/bin
	/usr/local/X11R6/bin /usr/xpg4/bin /usr/bin /usr/ccs/bin /usr/sbin
	/usr/ucb /usr/openwin/bin
	successful

That is, with httpd_only=httpd I get:

	$ if expr "z$httpd_only" : "z/" >/dev/null || \
		which $httpd_only >/dev/null
	then
		$httpd_only arg1 arg2
	fi
	bash: httpd: command not found

Best regards
Uwe

-- 
Uwe Kleine-König

If a lawyer and an IRS agent were both drowning, and you could only save
one of them, would you go to lunch or read the paper?