{"thread":{"id":"9609","subject":"[PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","startedAt":"2007-08-22T12:02:43Z","lastAt":"2007-08-23T10:02:02Z","messageCount":11,"participants":["David Jack Olrik","Uwe Kleine-König","martin f krafft","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"51242","messageId":"11877841633473-git-send-email-david@olrik.dk","threadId":"9609","inReplyTo":null,"subject":"[PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","fromName":"David Jack Olrik","fromEmail":"david@olrik.dk","sentAt":"2007-08-22T12:02:43Z","receivedAt":"2007-08-22T12:02:43Z","isPatch":true,"sender":{"key":"david@olrik.dk","avatar":"https://gravatar.com/avatar/57c45d6c7ec26ec586b2d5f4d7b77d02f5a544bd43fa3345c518b54736d2d5ba?d=mp&s=160"},"body":"\nSigned-off-by: David Jack Olrik \n---\n git-instaweb.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex b79c6b6..da8eb3f 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -30,7 +30,7 @@ test -z \"$port\" && port=1234\n \n start_httpd () {\n \thttpd_only=\"`echo $httpd | cut -f1 -d' '`\"\n-\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n+\tif test \"`echo $httpd_only | cut -c 1`\" = '/' || \\\n \t\t\t\twhich $httpd_only >/dev/null\n \tthen\n \t\t$httpd $fqgitdir/gitweb/httpd.conf\n-- \n1.5.3.rc6\n"},{"id":"51244","messageId":"20070822122140.GB11926@informatik.uni-freiburg.de","threadId":"9609","inReplyTo":"11877841633473-git-send-email-david@olrik.dk","subject":"Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","fromName":"Uwe Kleine-König","fromEmail":"ukleinek@informatik.uni-freiburg.de","sentAt":"2007-08-22T12:21:40Z","receivedAt":"2007-08-22T12:21:40Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nDavid Jack Olrik wrote:\n> -\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n> +\tif test \"`echo $httpd_only | cut -c 1`\" = '/' || \\\n> \t\t\t\twhich $httpd_only >/dev/null\n\nI wonder why not use:\n\n\tif expr \"z$httpd_only\" : \"z/\" >/dev/null\n\nBest regards\nUwe\n\n-- \nUwe Kleine-König\n\nexit vi, lesson II:\n: w q ! \n\nNB: write the current file\n"},{"id":"51245","messageId":"9E40DAA3-69E5-417C-8041-E1980E42DB60@olrik.dk","threadId":"9609","inReplyTo":"20070822122140.GB11926@informatik.uni-freiburg.de","subject":"Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","fromName":"David Jack Olrik","fromEmail":"david@olrik.dk","sentAt":"2007-08-22T12:30:03Z","receivedAt":"2007-08-22T12:30:03Z","isPatch":true,"sender":{"key":"david@olrik.dk","avatar":"https://gravatar.com/avatar/57c45d6c7ec26ec586b2d5f4d7b77d02f5a544bd43fa3345c518b54736d2d5ba?d=mp&s=160"},"body":"\nOn 22/08/2007, at 14.21, Uwe Kleine-König wrote:\n\n> David Jack Olrik wrote:\n>> -\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n>> +\tif test \"`echo $httpd_only | cut -c 1`\" = '/' || \\\n>> \t\t\t\twhich $httpd_only >/dev/null\n>\n> I wonder why not use:\n>\n> \tif expr \"z$httpd_only\" : \"z/\" >/dev/null\n\nYou'd then need to check against 2 instead of 1, which I find less\nobvious as we are testing for a '/' at the begining of the string.\n\n-- \nBest regards,\nDavid Jack Olrik http://david.olrik.dk\nGnuPG fingerprint C290 0A4A 0CCC CBA8 2B37 E18D 01D2 F6EF 2E61 9894\n[\"The first rule of Perl club is You do not talk about Perl club\"]\n"},{"id":"51249","messageId":"20070822132359.GA13750@informatik.uni-freiburg.de","threadId":"9609","inReplyTo":"9E40DAA3-69E5-417C-8041-E1980E42DB60@olrik.dk","subject":"Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","fromName":"Uwe Kleine-König","fromEmail":"ukleinek@informatik.uni-freiburg.de","sentAt":"2007-08-22T13:23:59Z","receivedAt":"2007-08-22T13:23:59Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"David Jack Olrik wrote:\n> \n> On 22/08/2007, at 14.21, Uwe Kleine-König wrote:\n> \n> >David Jack Olrik wrote:\n> >>-\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n> >>+\tif test \"`echo $httpd_only | cut -c 1`\" = '/' || \\\n> >> \t\t\t\twhich $httpd_only >/dev/null\n> >\n> >I wonder why not use:\n> >\n> >\tif expr \"z$httpd_only\" : \"z/\" >/dev/null\n> \n> You'd then need to check against 2 instead of 1, which I find less\n> obvious as we are testing for a '/' at the begining of the string.\nIf I understood the problem right you only need to test for the exit\ncode, that is the program test is not required at all.\n\nBest regards\nUwe\n\n-- \nUwe Kleine-König\n\nhttp://www.google.com/search?q=5%2B7\n"},{"id":"51287","messageId":"11878139102715-git-send-email-david@olrik.dk","threadId":"9609","inReplyTo":"20070822132359.GA13750@informatik.uni-freiburg.de","subject":"[PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"David Jack Olrik","fromEmail":"david@olrik.dk","sentAt":"2007-08-22T20:18:30Z","receivedAt":"2007-08-22T20:18:30Z","isPatch":true,"sender":{"key":"david@olrik.dk","avatar":"https://gravatar.com/avatar/57c45d6c7ec26ec586b2d5f4d7b77d02f5a544bd43fa3345c518b54736d2d5ba?d=mp&s=160"},"body":"On 22/08/2007, at 15.23, Uwe Kleine-König wrote:\n\n> > You'd then need to check against 2 instead of 1, which I find less\n> > obvious as we are testing for a '/' at the begining of the string.\n> If I understood the problem right you only need to test for the exit\n> code, that is the program test is not required at all.\n\nAh, yes that's true. The following should make it more clear that we are\nlooking at the first character.\n\n if expr \"$httpd_only\" : \"\\/\" >/dev/null\n\n\nSigned-off-by: David Jack Olrik \n---\n git-instaweb.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex b79c6b6..85646f1 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -30,7 +30,7 @@ test -z \"$port\" && port=1234\n \n start_httpd () {\n \thttpd_only=\"`echo $httpd | cut -f1 -d' '`\"\n-\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n+\tif expr \"$httpd_only\" : \"\\/\" >/dev/null || \\\n \t\t\t\twhich $httpd_only >/dev/null\n \tthen\n \t\t$httpd $fqgitdir/gitweb/httpd.conf\n-- \n1.5.3.rc6\n"},{"id":"51290","messageId":"20070822210001.GA7585@piper.oerlikon.madduck.net","threadId":"9609","inReplyTo":"11877841633473-git-send-email-david@olrik.dk","subject":"Re: [PATCH] Fixed non portable use of expr, and incorrect use of test -eq for string comparison.","fromName":"martin f krafft","fromEmail":"madduck@madduck.net","sentAt":"2007-08-22T21:00:01Z","receivedAt":"2007-08-22T21:00:01Z","isPatch":true,"sender":{"key":"madduck@madduck.net","avatar":null},"body":"also sprach David Jack Olrik [2007.08.22.1402 +0200]:\n> -\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n> +\tif test \"`echo $httpd_only | cut -c 1`\" = '/' || \\\n\nhow about\n\n if [ \"$httpd_only\" != \"${httpd_only#/}\" ]; then\n\nthat should do the same and does so without external processes.\n\n> \t\t\t\twhich $httpd_only >/dev/null\n\nThis is also not really portable. I suggest the use of\n\n command -v $http_only\n\nfirst, that's shell-internal as well, and second, it's\n/more/ portable than which, but also not entirely.\n\nhttp://www.debian.org/doc/developers-reference/ch-best-pkging-practices.en.html#s-bpp-debian-maint-scripts\n\n-- \nmartin; (greetings from the heart of the sun.)\n \\____ echo mailto: !#^.\"<*>\"|tr \"<*> mailto:\" net@madduck\n \n\"of course the music is a great difficulty.\n you see, if one plays good music, people don't listen,\n and if one plays bad music people don't talk.\"\n -- oscar wilde\n \nspamtraps: madduck.bogus@madduck.net\n"},{"id":"51298","messageId":"7vejhvi67x.fsf@gitster.siamese.dyndns.org","threadId":"9609","inReplyTo":"11878139102715-git-send-email-david@olrik.dk","subject":"Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-22T21:56:18Z","receivedAt":"2007-08-22T21:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Jack Olrik writes:\n\n> On 22/08/2007, at 15.23, Uwe Kleine-König wrote:\n>\n>> > You'd then need to check against 2 instead of 1, which I find less\n>> > obvious as we are testing for a '/' at the begining of the string.\n>> If I understood the problem right you only need to test for the exit\n>> code, that is the program test is not required at all.\n>\n> Ah, yes that's true. The following should make it more clear that we are\n> looking at the first character.\n>\n> if expr \"$httpd_only\" : \"\\/\" >/dev/null\n\nThen why not avoid the fork with something like:\n\n\tcase \"$httpd_only\" in\n /*)\n ... begins with slash ...\n\tesac\n\nBy the way, I do not know if the use of \"which\" there is\nportable. Have Solaris folks tried this program ever?\n\nHow about doing it this way?\n\n * No need to use \"cut\"; just let IFS do its job.\n\n * No need to use \"which\"; we will need to do a discovery with\n custom paths anyway, so use the same logic for discovery from\n the normal $PATH as well.\n\n * The original code wanted to allow the httpd command that\n comes to the function to have arguments and preserve it but\n did so only for the case where the command was not specified\n with a full pathname. Allow it for full-path case as well\n for consistency.\n\n * The original code returned without checking for error when\n httpd was specified without a full path, and checked and\n complained when it was. Check the error exit for both cases.\n\n---\n\n git-instaweb.sh | 42 ++++++++++++++++++++++++++----------------\n 1 files changed, 26 insertions(+), 16 deletions(-)\n\ndiff --git a/git-instaweb.sh b/git-instaweb.sh\nindex b79c6b6..a17c9b3 100755\n--- a/git-instaweb.sh\n+++ b/git-instaweb.sh\n@@ -29,32 +29,42 @@ test -z \"$browser\" && browser='firefox'\n test -z \"$port\" && port=1234\n \n start_httpd () {\n-\thttpd_only=\"`echo $httpd | cut -f1 -d' '`\"\n-\tif test \"`expr index $httpd_only /`\" -eq '1' || \\\n-\t\t\t\twhich $httpd_only >/dev/null\n-\tthen\n-\t\t$httpd $fqgitdir/gitweb/httpd.conf\n-\telse\n+\tset x $httpd\n+\tshift\n+\n+\thttpd=\n+\tcase \"$1\" in\n+\t/*)\n+\t\t: full path -- no discovery\n+\t\thttpd=\"$1\"\n+\t\t;;\n+\t*)\n \t\t# many httpds are installed in /usr/sbin or /usr/local/sbin\n \t\t# these days and those are not in most users $PATHs\n-\t\tfor i in /usr/local/sbin /usr/sbin\n+\t\tpaths=\"$PATH:/usr/sbin:/usr/local/sbin\"\n+\t\toIFS=\"$IFS\"\n+\t\tIFS=:\n+\t\tfor p in $paths\n \t\tdo\n-\t\t\tif test -x \"$i/$httpd_only\"\n+\t\t\tif test -x \"$p/$1\"\n \t\t\tthen\n-\t\t\t\t# don't quote $httpd, there can be\n-\t\t\t\t# arguments to it (-f)\n-\t\t\t\t$i/$httpd \"$fqgitdir/gitweb/httpd.conf\"\n-\t\t\t\treturn\n+\t\t\t\thttpd=\"$p/$1\"\n+\t\t\t\tbreak\n \t\t\tfi\n \t\tdone\n-\t\techo \"$httpd_only not found. Install $httpd_only or use\" \\\n-\t\t \"--httpd to specify another http daemon.\"\n+\t\tIFS=\"$oIFS\"\n+\t\t;;\n+\tesac\n+\tif test -z \"$httpd\"\n+\tthen\n+\t\techo \"$1 not found. Install $1 or use --httpd to specify another http daemon.\"\n \t\texit 1\n \tfi\n-\tif test $? != 0; then\n+\tshift\n+\t\"$httpd\" \"$@\" \"$fqgitdir/gitweb/httpd.conf\" || {\n \t\techo \"Could not execute http daemon $httpd.\"\n \t\texit 1\n-\tfi\n+\t}\n }\n \n stop_httpd () {\n"},{"id":"51348","messageId":"20070823085808.GA6573@informatik.uni-freiburg.de","threadId":"9609","inReplyTo":"11878139102715-git-send-email-david@olrik.dk","subject":"Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"Uwe Kleine-König","fromEmail":"ukleinek@informatik.uni-freiburg.de","sentAt":"2007-08-23T08:58:08Z","receivedAt":"2007-08-23T08:58:08Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nDavid Jack Olrik wrote:\n> On 22/08/2007, at 15.23, Uwe Kleine-König wrote:\n> \n> > > You'd then need to check against 2 instead of 1, which I find less\n> > > obvious as we are testing for a '/' at the begining of the string.\n> > If I understood the problem right you only need to test for the exit\n> > code, that is the program test is not required at all.\n> \n> Ah, yes that's true. The following should make it more clear that we are\n> looking at the first character.\n> \n> if expr \"$httpd_only\" : \"\\/\" >/dev/null\nyet another note: I used expr \"z$http_only\" on purpose. Look what\nhappens here:\n\n\t$ httpd_only=substr\n\t$ expr \"$httpd_only\" : \"\\/\"\n\texpr: syntax error\n\nOnce more, Solaris is more exacting:\n\n\t$ /usr/bin/expr \"/\" : \"\\/\"\n\texpr: syntax error\n\n(This works fine with GNU expr.)\n\nSo better use \"z$variable\" because (up to now) there is no operator that\nstarts with a 'z'.\n\nBest regards\nUwe\n\n-- \nUwe Kleine-König\n\nfib where fib = 0 : 1 : zipWith (+) fib (tail fib)\n"},{"id":"51349","messageId":"20070823090600.GB6573@informatik.uni-freiburg.de","threadId":"9609","inReplyTo":"7vejhvi67x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"Uwe Kleine-König","fromEmail":"ukleinek@informatik.uni-freiburg.de","sentAt":"2007-08-23T09:06:00Z","receivedAt":"2007-08-23T09:06:00Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Junio,\n\nJunio C Hamano wrote:\n> By the way, I do not know if the use of \"which\" there is\n> portable. Have Solaris folks tried this program ever?\nI don't count myself to \"Solaris folks\", even though I still use it to\nread and write my email. But anyhow I know some of the pitfalls...\n\n\tlogin@~ > /bin/bash --version\n\tGNU bash, version 3.00.16(1)-release (sparc-sun-solaris2.10)\n\tCopyright (C) 2004 Free Software Foundation, Inc.\n\n\tlogin@~ > /bin/bash\n\n\tzeisberg@login ~$ which httpd && echo successful\n\tno httpd in /home/zeisberg/bin /home/zeisberg/usr/bin /opt/bin\n\t/usr/local/graphics/bin /usr/local/gnu/bin /usr/local/bin\n\t/usr/local/X11R6/bin /usr/xpg4/bin /usr/bin /usr/ccs/bin /usr/sbin\n\t/usr/ucb /usr/openwin/bin\n\tsuccessful\n\nThat is, with httpd_only=httpd I get:\n\n\t$ if expr \"z$httpd_only\" : \"z/\" >/dev/null || \\\n\t\twhich $httpd_only >/dev/null\n\tthen\n\t\t$httpd_only arg1 arg2\n\tfi\n\tbash: httpd: command not found\n\nBest regards\nUwe\n\n-- \nUwe Kleine-König\n\nIf a lawyer and an IRS agent were both drowning, and you could only save\none of them, would you go to lunch or read the paper?\n"},{"id":"51353","messageId":"7vveb6txfc.fsf@gitster.siamese.dyndns.org","threadId":"9609","inReplyTo":"20070823090600.GB6573@informatik.uni-freiburg.de","subject":"Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-23T09:25:27Z","receivedAt":"2007-08-23T09:25:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König writes:\n\n> Junio C Hamano wrote:\n>> By the way, I do not know if the use of \"which\" there is\n>> portable. Have Solaris folks tried this program ever?\n> I don't count myself to \"Solaris folks\", even though I still use it to\n> read and write my email. But anyhow I know some of the pitfalls...\n>\n> \tlogin@~ > /bin/bash --version\n> \tGNU bash, version 3.00.16(1)-release (sparc-sun-solaris2.10)\n> \tCopyright (C) 2004 Free Software Foundation, Inc.\n>\n> \tlogin@~ > /bin/bash\n>\n> \tzeisberg@login ~$ which httpd && echo successful\n> \tno httpd in /home/zeisberg/bin /home/zeisberg/usr/bin /opt/bin\n> \t/usr/local/graphics/bin /usr/local/gnu/bin /usr/local/bin\n> \t/usr/local/X11R6/bin /usr/xpg4/bin /usr/bin /usr/ccs/bin /usr/sbin\n> \t/usr/ucb /usr/openwin/bin\n> \tsuccessful\n\nThanks. Somebody else tried:\n\n\tfound=`which \"$command\"`\n if test -n \"$found\"\n then\n \t... use $found as the full path to the command\n\tfi\n\nand got burned because \"no httpd in ...\" comes to the stdout!\nI did not exactly recall if there was an issue with the exit\nstatus, but your demonstration shows that the status is also\nuseless.\n\nWe _could_ do something ugly and pointless like:\n\n\ttest -f `which \"$command\"`\n\nbut I'd say I prefer the alternative I sent out at that point.\n"},{"id":"51358","messageId":"20070823100202.GE6573@informatik.uni-freiburg.de","threadId":"9609","inReplyTo":"7vveb6txfc.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fixed non portable use of expr and removed incorrect use of test -eq for string comparison","fromName":"Uwe Kleine-König","fromEmail":"ukleinek@informatik.uni-freiburg.de","sentAt":"2007-08-23T10:02:02Z","receivedAt":"2007-08-23T10:02:02Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello Junio,\n\nJunio C Hamano wrote:\n> Uwe Kleine-König writes:\n> \n> > Junio C Hamano wrote:\n> >> By the way, I do not know if the use of \"which\" there is\n> >> portable. Have Solaris folks tried this program ever?\n> > I don't count myself to \"Solaris folks\", even though I still use it to\n> > read and write my email. But anyhow I know some of the pitfalls...\n> >\n> > \tlogin@~ > /bin/bash --version\n> > \tGNU bash, version 3.00.16(1)-release (sparc-sun-solaris2.10)\n> > \tCopyright (C) 2004 Free Software Foundation, Inc.\n> >\n> > \tlogin@~ > /bin/bash\n> >\n> > \tzeisberg@login ~$ which httpd && echo successful\n> > \tno httpd in /home/zeisberg/bin /home/zeisberg/usr/bin /opt/bin\n> > \t/usr/local/graphics/bin /usr/local/gnu/bin /usr/local/bin\n> > \t/usr/local/X11R6/bin /usr/xpg4/bin /usr/bin /usr/ccs/bin /usr/sbin\n> > \t/usr/ucb /usr/openwin/bin\n> > \tsuccessful\n> \n> Thanks. Somebody else tried:\n> \n> \tfound=`which \"$command\"`\n> if test -n \"$found\"\n> then\n> \t... use $found as the full path to the command\n> \tfi\n> \n> and got burned because \"no httpd in ...\" comes to the stdout!\nThat was someting I planed to mention in my email but obviously I\nforgot.\n\n> I did not exactly recall if there was an issue with the exit\n> status, but your demonstration shows that the status is also\n> useless.\n> \n> We _could_ do something ugly and pointless like:\n> \n> \ttest -f `which \"$command\"`\n> \n> but I'd say I prefer the alternative I sent out at that point.\nAck.\n\n-- \nUwe Kleine-König\n\nhttp://www.google.com/search?q=gravity+on+earth%3D\n"}]}