* [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.
@ 2007-08-22 12:02 David Jack Olrik
2007-08-22 12:21 ` Uwe Kleine-König
2007-08-22 21:00 ` [PATCH] Fixed non portable use of expr, and " martin f krafft
0 siblings, 2 replies; 11+ messages in thread
From: David Jack Olrik @ 2007-08-22 12:02 UTC (permalink / raw)
To: git; +Cc: David Jack Olrik
Signed-off-by: David Jack Olrik <david@olrik.dk>
---
git-instaweb.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index b79c6b6..da8eb3f 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -30,7 +30,7 @@ test -z "$port" && port=1234
start_httpd () {
httpd_only="`echo $httpd | cut -f1 -d' '`"
- if test "`expr index $httpd_only /`" -eq '1' || \
+ if test "`echo $httpd_only | cut -c 1`" = '/' || \
which $httpd_only >/dev/null
then
$httpd $fqgitdir/gitweb/httpd.conf
--
1.5.3.rc6
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.
2007-08-22 12:02 [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison David Jack Olrik
@ 2007-08-22 12:21 ` Uwe Kleine-König
2007-08-22 12:30 ` David Jack Olrik
2007-08-22 21:00 ` [PATCH] Fixed non portable use of expr, and " martin f krafft
1 sibling, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2007-08-22 12:21 UTC (permalink / raw)
To: David Jack Olrik; +Cc: git
Hello,
David Jack Olrik wrote:
> - if test "`expr index $httpd_only /`" -eq '1' || \
> + if test "`echo $httpd_only | cut -c 1`" = '/' || \
> which $httpd_only >/dev/null
I wonder why not use:
if expr "z$httpd_only" : "z/" >/dev/null
Best regards
Uwe
--
Uwe Kleine-König
exit vi, lesson II:
: w q ! <CR>
NB: write the current file
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.
2007-08-22 12:21 ` Uwe Kleine-König
@ 2007-08-22 12:30 ` David Jack Olrik
2007-08-22 13:23 ` Uwe Kleine-König
0 siblings, 1 reply; 11+ messages in thread
From: David Jack Olrik @ 2007-08-22 12:30 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: git
On 22/08/2007, at 14.21, Uwe Kleine-König wrote:
> David Jack Olrik wrote:
>> - if test "`expr index $httpd_only /`" -eq '1' || \
>> + if test "`echo $httpd_only | cut -c 1`" = '/' || \
>> which $httpd_only >/dev/null
>
> I wonder why not use:
>
> if expr "z$httpd_only" : "z/" >/dev/null
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.
--
Best regards,
David Jack Olrik <david@olrik.dk> http://david.olrik.dk
GnuPG fingerprint C290 0A4A 0CCC CBA8 2B37 E18D 01D2 F6EF 2E61 9894
["The first rule of Perl club is You do not talk about Perl club"]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.
2007-08-22 12:30 ` David Jack Olrik
@ 2007-08-22 13:23 ` Uwe Kleine-König
2007-08-22 20:18 ` [PATCH] Fixed non portable use of expr and removed " David Jack Olrik
0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2007-08-22 13:23 UTC (permalink / raw)
To: David Jack Olrik; +Cc: git
David Jack Olrik wrote:
>
> On 22/08/2007, at 14.21, Uwe Kleine-König wrote:
>
> >David Jack Olrik wrote:
> >>- if test "`expr index $httpd_only /`" -eq '1' || \
> >>+ if test "`echo $httpd_only | cut -c 1`" = '/' || \
> >> which $httpd_only >/dev/null
> >
> >I wonder why not use:
> >
> > if expr "z$httpd_only" : "z/" >/dev/null
>
> 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.
Best regards
Uwe
--
Uwe Kleine-König
http://www.google.com/search?q=5%2B7
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-22 13:23 ` Uwe Kleine-König
@ 2007-08-22 20:18 ` David Jack Olrik
2007-08-22 21:56 ` Junio C Hamano
2007-08-23 8:58 ` Uwe Kleine-König
0 siblings, 2 replies; 11+ messages in thread
From: David Jack Olrik @ 2007-08-22 20:18 UTC (permalink / raw)
To: git; +Cc: ukleinek, David Jack Olrik
On 22/08/2007, at 15.23, Uwe Kleine-König wrote:
> > 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
Signed-off-by: David Jack Olrik <david@olrik.dk>
---
git-instaweb.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index b79c6b6..85646f1 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -30,7 +30,7 @@ test -z "$port" && port=1234
start_httpd () {
httpd_only="`echo $httpd | cut -f1 -d' '`"
- if test "`expr index $httpd_only /`" -eq '1' || \
+ if expr "$httpd_only" : "\/" >/dev/null || \
which $httpd_only >/dev/null
then
$httpd $fqgitdir/gitweb/httpd.conf
--
1.5.3.rc6
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-22 20:18 ` [PATCH] Fixed non portable use of expr and removed " David Jack Olrik
@ 2007-08-22 21:56 ` Junio C Hamano
2007-08-23 9:06 ` Uwe Kleine-König
2007-08-23 8:58 ` Uwe Kleine-König
1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2007-08-22 21:56 UTC (permalink / raw)
To: David Jack Olrik; +Cc: git, ukleinek
David Jack Olrik <david@olrik.dk> writes:
> On 22/08/2007, at 15.23, Uwe Kleine-König wrote:
>
>> > 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 () {
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-22 21:56 ` Junio C Hamano
@ 2007-08-23 9:06 ` Uwe Kleine-König
2007-08-23 9:25 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2007-08-23 9:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Jack Olrik, git
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-23 9:06 ` Uwe Kleine-König
@ 2007-08-23 9:25 ` Junio C Hamano
2007-08-23 10:02 ` Uwe Kleine-König
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2007-08-23 9:25 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: David Jack Olrik, git
Uwe Kleine-König <ukleinek@informatik.uni-freiburg.de> writes:
> 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
Thanks. Somebody else tried:
found=`which "$command"`
if test -n "$found"
then
... use $found as the full path to the command
fi
and got burned because "no httpd in ..." comes to the stdout!
I did not exactly recall if there was an issue with the exit
status, but your demonstration shows that the status is also
useless.
We _could_ do something ugly and pointless like:
test -f `which "$command"`
but I'd say I prefer the alternative I sent out at that point.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-23 9:25 ` Junio C Hamano
@ 2007-08-23 10:02 ` Uwe Kleine-König
0 siblings, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2007-08-23 10:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Jack Olrik, git
Hello Junio,
Junio C Hamano wrote:
> Uwe Kleine-König <ukleinek@informatik.uni-freiburg.de> writes:
>
> > 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
>
> Thanks. Somebody else tried:
>
> found=`which "$command"`
> if test -n "$found"
> then
> ... use $found as the full path to the command
> fi
>
> and got burned because "no httpd in ..." comes to the stdout!
That was someting I planed to mention in my email but obviously I
forgot.
> I did not exactly recall if there was an issue with the exit
> status, but your demonstration shows that the status is also
> useless.
>
> We _could_ do something ugly and pointless like:
>
> test -f `which "$command"`
>
> but I'd say I prefer the alternative I sent out at that point.
Ack.
--
Uwe Kleine-König
http://www.google.com/search?q=gravity+on+earth%3D
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison
2007-08-22 20:18 ` [PATCH] Fixed non portable use of expr and removed " David Jack Olrik
2007-08-22 21:56 ` Junio C Hamano
@ 2007-08-23 8:58 ` Uwe Kleine-König
1 sibling, 0 replies; 11+ messages in thread
From: Uwe Kleine-König @ 2007-08-23 8:58 UTC (permalink / raw)
To: David Jack Olrik; +Cc: git
Hello,
David Jack Olrik wrote:
> On 22/08/2007, at 15.23, Uwe Kleine-König wrote:
>
> > > 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
yet another note: I used expr "z$http_only" on purpose. Look what
happens here:
$ httpd_only=substr
$ expr "$httpd_only" : "\/"
expr: syntax error
Once more, Solaris is more exacting:
$ /usr/bin/expr "/" : "\/"
expr: syntax error
(This works fine with GNU expr.)
So better use "z$variable" because (up to now) there is no operator that
starts with a 'z'.
Best regards
Uwe
--
Uwe Kleine-König
fib where fib = 0 : 1 : zipWith (+) fib (tail fib)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.
2007-08-22 12:02 [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison David Jack Olrik
2007-08-22 12:21 ` Uwe Kleine-König
@ 2007-08-22 21:00 ` martin f krafft
1 sibling, 0 replies; 11+ messages in thread
From: martin f krafft @ 2007-08-22 21:00 UTC (permalink / raw)
To: David Jack Olrik; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
also sprach David Jack Olrik <david@olrik.dk> [2007.08.22.1402 +0200]:
> - if test "`expr index $httpd_only /`" -eq '1' || \
> + if test "`echo $httpd_only | cut -c 1`" = '/' || \
how about
if [ "$httpd_only" != "${httpd_only#/}" ]; then
that should do the same and does so without external processes.
> which $httpd_only >/dev/null
This is also not really portable. I suggest the use of
command -v $http_only
first, that's shell-internal as well, and second, it's
/more/ portable than which, but also not entirely.
http://www.debian.org/doc/developers-reference/ch-best-pkging-practices.en.html#s-bpp-debian-maint-scripts
--
martin; (greetings from the heart of the sun.)
\____ echo mailto: !#^."<*>"|tr "<*> mailto:" net@madduck
"of course the music is a great difficulty.
you see, if one plays good music, people don't listen,
and if one plays bad music people don't talk."
-- oscar wilde
spamtraps: madduck.bogus@madduck.net
[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-08-23 10:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-22 12:02 [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison David Jack Olrik
2007-08-22 12:21 ` Uwe Kleine-König
2007-08-22 12:30 ` David Jack Olrik
2007-08-22 13:23 ` Uwe Kleine-König
2007-08-22 20:18 ` [PATCH] Fixed non portable use of expr and removed " David Jack Olrik
2007-08-22 21:56 ` Junio C Hamano
2007-08-23 9:06 ` Uwe Kleine-König
2007-08-23 9:25 ` Junio C Hamano
2007-08-23 10:02 ` Uwe Kleine-König
2007-08-23 8:58 ` Uwe Kleine-König
2007-08-22 21:00 ` [PATCH] Fixed non portable use of expr, and " martin f krafft
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).