From: Junio C Hamano <hidden> Date: 2016-06-15 22:50:55
Ángel González [off-list ref] writes:
This is wrong.
Not really.
You are replacing bash with sh:
quoted
-#!/bin/bash
+#!/bin/sh
but the script still uses bash-specific syntax (aka. bashishms):
Do you mean some of the parts you quoted are bashism?
quoted
PATCH=$(zenity --file-selection)
Even though ancient shells I grew up with did not have $(), it is a way
backticks should have been written by Bourne from day one. Historically,
handling nesting and interraction between double-quotes and backticks
correctly was a nightmare to get right, and different implementations of
shells got them always wrong. If you use $(), the headaches go away.
These days, we don't know of any POSIX shell that is widely used and does
not understand $(). As such, the above construct is perfectly safe and
even preferred over ``. Welcome to the 21st century ;-)
quoted
if [ "$?" != "0" ] ; then
While I personally do not like this style (I am old fashioned) and would
probably write:
if test $? != 0
then
...
or make it even more readable by writing it together with the previous
statement, i.e.
PATCH=$(zenity --file-selection) ||
...
myself, it is definitely not bash-ism to use [] for conditionals. Some
people seem to find it more readable than traditional "test" (not me).
The only major platform that didn't have a reasonable shell was Solaris,
but we already have written its /bin/sh off as broken and unusable, and
suggest people to use xpg4 or xpg6 shell (see the Makefile).
but the script still uses bash-specific syntax (aka. bashishms):
Do you mean some of the parts you quoted are bashism?
I was pointing to the $( ) as a bashishm
quoted
quoted
PATCH=$(zenity --file-selection)
Even though ancient shells I grew up with did not have $(), it is a way
backticks should have been written by Bourne from day one. Historically,
handling nesting and interraction between double-quotes and backticks
correctly was a nightmare to get right, and different implementations of
shells got them always wrong. If you use $(), the headaches go away.
These days, we don't know of any POSIX shell that is widely used and does
not understand $(). As such, the above construct is perfectly safe and
even preferred over ``. Welcome to the 21st century ;-)
The only major platform that didn't have a reasonable shell was Solaris,
but we already have written its /bin/sh off as broken and unusable, and
suggest people to use xpg4 or xpg6 shell (see the Makefile).
I have to agree with you. $() is a much saner syntax. Still, the goal
was portability.
Reading your message, and considering the Solaris note, it might have
been fine as it was. I have also checked the "Shell Command Language"
section of IEEE Std 1003.1 and it does require $() use.
Albeit being a single line I would still change it, it is now a much
weaker position. Thanks for your insight.
From: Maxin john <hidden> Date: 2016-06-15 22:50:56
Hi,
Junio C Hamano wrote:
..
quoted
Even though ancient shells I grew up with did not have $(), it is a way
backticks should have been written by Bourne from day one. Historically,
handling nesting and interraction between double-quotes and backticks
correctly was a nightmare to get right, and different implementations of
shells got them always wrong. If you use $(), the headaches go away.
These days, we don't know of any POSIX shell that is widely used and does
not understand $(). As such, the above construct is perfectly safe and
even preferred over ``. Welcome to the 21st century ;-)
The only major platform that didn't have a reasonable shell was Solaris,
but we already have written its /bin/sh off as broken and unusable, and
suggest people to use xpg4 or xpg6 shell (see the Makefile).
Thank you very much for sharing this information. It was really really
informative.
Thanks to Ángel González and Victor Engmark for sharing their views.
Considering all the suggestions, I think, it is "not possible to
satisfy everyone" :)
So, I have modified the patch by incorporating most of the nice suggestions.
Please let me know your comments.
Signed-off-by: Maxin B. John <redacted>
---
@@ -1,8 +1,8 @@-#!/bin/bash+#!/bin/sh# Copyright 2008 Lukas Sandström <luksan@gmail.com>## AppendPatch - A script to be used together with ExternalEditor-# for Mozilla Thunderbird to properly include pathes inline i e-mails.+# for Mozilla Thunderbird to properly include patches inline in e-mails.# ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2
@@ -16,13 +16,12 @@ elsecd>/dev/nullfi-PATCH=$(zenity--file-selection)--if["$?"!="0"];then-#zenity --error --text "No patchfile given."-exit1+#check whether zenity is present+if!typezenity>/dev/null2>&1;then+exit1fi+PATCH=$(zenity--file-selection)||exit1cd->/dev/nullSUBJECT=`sed-n-e'/^Subject: /p'"${PATCH}"`
From: Junio C Hamano <hidden> Date: 2016-06-15 22:50:57
Just for the record, the patch at the bottom is what I queued.
-- >8 --
From: Maxin john <redacted>
Subject: [PATCH] contrib/thunderbird-patch-inline: do not require bash to run the script
The script does not have to be run under bash, but any POSIX compliant
shell would do, as it does not use any bash-isms.
It may be written under a different style than what is recommended in
Documentation/CodingGuidelines, but that is a different matter.
While at it, fix obvious typos in the comment.
Signed-off-by: Maxin B. John <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
contrib/thunderbird-patch-inline/appp.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
@@ -1,8 +1,8 @@-#!/bin/bash+#!/bin/sh# Copyright 2008 Lukas Sandström <luksan@gmail.com>## AppendPatch - A script to be used together with ExternalEditor-# for Mozilla Thunderbird to properly include pathes inline i e-mails.+# for Mozilla Thunderbird to properly include patches inline in e-mails.# ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2