[PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Maxin john]

Remove "bashism" and minor corrections for
contrib/thunderbird-patch-inline/appp.sh

Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>
---
diff --git a/contrib/thunderbird-patch-inline/appp.sh
b/contrib/thunderbird-patch-inline/appp.sh
index cc518f3..1d29f4b 100755
--- a/contrib/thunderbird-patch-inline/appp.sh
+++ b/contrib/thunderbird-patch-inline/appp.sh
@@ -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,6 +16,11 @@ else
        cd > /dev/null
 fi

+#check whether zenity is present
+if ! type zenity >/dev/null 2>&1 ; then
+       exit 1
+fi
+
 PATCH=$(zenity --file-selection)

 if [ "$?" != "0" ] ; then

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Ángel González]

Maxin john wrote:
> Remove "bashism" and minor corrections for
> contrib/thunderbird-patch-inline/appp.sh
> 
> Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>

This is wrong.

You are replacing bash with sh:
> -#!/bin/bash
> +#!/bin/sh

but the script still uses bash-specific syntax (aka. bashishms):
> +
>  PATCH=$(zenity --file-selection)
> 
>  if [ "$?" != "0" ] ; then

So with your change the script won't be able to run on systems which
don't have bash as /bin/sh

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Ángel González]

Maxin john wrote:
> > Remove "bashism" and minor corrections for
> > contrib/thunderbird-patch-inline/appp.sh
> >
> > Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>
This is wrong.

You are replacing bash with sh:
> > -#!/bin/bash
> > +#!/bin/sh

but the script still uses bash-specific syntax (aka. bashishms):
> > +
> >  PATCH=$(zenity --file-selection)
> >
> >  if [ "$?" != "0" ] ; then

So with your change the script won't be able to run on systems which
don't have bash as /bin/sh

The standard equivalent of $( ) are `backticks`.

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Junio C Hamano]

Maxin john <maxin@maxinbjohn.info> writes:

> Remove "bashism" and minor corrections for
> contrib/thunderbird-patch-inline/appp.sh

The script seems to only use standard POSIX shell features and nothing
particularly bash specific nor outside POSIX in general that we exclude
from our coding standards (e.g. "local", use of "function" noiseword,
substring expansion ${parmeter:offset:length}).

It is better to explain the patch as

  Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Maxin john]

Hi,

> It is better to explain the patch as
>
>  Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run

Yes. I do agree with you. I should have chosen a better Subject line
for this patch. Should I resend this patch with this modified Subject
line ?

Thanks and Regards,
Maxin B. John


On Tue, Mar 29, 2011 at 4:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Maxin john <maxin@maxinbjohn.info> writes:
>
>> Remove "bashism" and minor corrections for
>> contrib/thunderbird-patch-inline/appp.sh
>
> The script seems to only use standard POSIX shell features and nothing
> particularly bash specific nor outside POSIX in general that we exclude
> from our coding standards (e.g. "local", use of "function" noiseword,
> substring expansion ${parmeter:offset:length}).
>
> It is better to explain the patch as
>
>  Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run
>
>

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Maxin john]

Hi,

Thank you very much for the suggestions. However, I have tested this
script in Ubuntu which uses dash as /bin/sh

Eg: the following script runs successfully in Ubuntu 10.10

#!/bin/dash

PATCH=$(zenity --file-selection)

if [ "$?" != "0" ] ; then
 echo "zenity failed"
else
 echo "success"
fi

I haven't confirmed this in other shell implementations. Please let me
know your comments on this.

Best Regards,
Maxin B. John

2011/3/29 Ángel González <ingenit@zoho.com>:
> Maxin john wrote:
>> Remove "bashism" and minor corrections for
>> contrib/thunderbird-patch-inline/appp.sh
>>
>> Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>
>
> This is wrong.
>
> You are replacing bash with sh:
>> -#!/bin/bash
>> +#!/bin/sh
>
> but the script still uses bash-specific syntax (aka. bashishms):
>> +
>>  PATCH=$(zenity --file-selection)
>>
>>  if [ "$?" != "0" ] ; then
>
> So with your change the script won't be able to run on systems which
> don't have bash as /bin/sh
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Junio C Hamano]

Ángel González <ingenit@zoho.com> writes:

> This is wrong.

Not really.

> You are replacing bash with sh:
>> -#!/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?

>>  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 ;-)

>>  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).

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Victor Engmark]

On 03/29/2011 09:09 AM, Junio C Hamano wrote:
> Ángel González <ingenit@zoho.com> writes:

>>>  if [ "$?" != "0" ] ; then
> 
> While I personally do not like this style (I am old fashioned) and would
> probably write:
> 
> 	if test $? != 0
>         then
>         	...

Nitpicking I suppose, but since `$?` is always an integer we should use
`-ne` (positive/negative integers) instead of `!=` (string comparison).

> 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).

Alternatively:

if ! PATCH=$(zenity --file-selection)
then
...

Yep, that works in dash - Both variable assignment and exit code checking.

-- 
Victor Engmark

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Ángel González]

Junio C Hamano wrote:
> Ángel González <ingenit@zoho.com> writes:
> 
>> This is wrong.
> 
> Not really.
> 
>> You are replacing bash with sh:
>>> -#!/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?

I was pointing to the $( ) as a bashishm

>>>  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.

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Maxin john]

Hi,

> Junio C Hamano wrote:
..
>> 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 <maxin@maxinbjohn.info>
---
diff --git a/contrib/thunderbird-patch-inline/appp.sh
b/contrib/thunderbird-patch-inline/appp.sh
index cc518f3..20dac9f 100755
--- a/contrib/thunderbird-patch-inline/appp.sh
+++ b/contrib/thunderbird-patch-inline/appp.sh
@@ -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 @@ else
        cd > /dev/null
 fi

-PATCH=$(zenity --file-selection)
-
-if [ "$?" != "0" ] ; then
-       #zenity --error --text "No patchfile given."
-       exit 1
+#check whether zenity is present
+if ! type zenity >/dev/null 2>&1 ; then
+       exit 1
 fi

+PATCH=$(zenity --file-selection) || exit 1
 cd - > /dev/null

 SUBJECT=`sed -n -e '/^Subject: /p' "${PATCH}"`

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Junio C Hamano]

Maxin john <maxin@maxinbjohn.info> writes:

> So, I have modified the patch by incorporating most of the nice suggestions.

I'd just replace /bin/bash with /bin/sh without any other fuss, perhaps
except for the typofix in the comment, and be done with the topic.

The script in the current version may look ugly to my eyes and others, but
there is nothing _wrong_ in it per-se.  Rewriting them in different styles
is not necessarily improvement, and this is only a contrib/ material after
all.

Thanks, I'll apply the early hunks from you.

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Maxin john]

Hi,

>
> I'd just replace /bin/bash with /bin/sh without any other fuss, perhaps
> except for the typofix in the comment, and be done with the topic.
>

I agree with this. The changes were mostly cosmetic and has nothing to
do with the functionality of the script.

>
> Thanks, I'll apply the early hunks from you.
>

Thank you very much.

Best Regards,
Maxin B. John

Re: [PATCH] Remove "bashism" from contrib/thunderbird-patch-inline/appp.sh

[Junio C Hamano]

Just for the record, the patch at the bottom is what I queued.

-- >8 --
From: Maxin john <maxin@maxinbjohn.info>
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 <maxin@maxinbjohn.info>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/thunderbird-patch-inline/appp.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh
index cc518f3..5eb4a51 100755
--- a/contrib/thunderbird-patch-inline/appp.sh
+++ b/contrib/thunderbird-patch-inline/appp.sh
@@ -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
 
-- 
1.7.4.2.422.g537d99