patchRemove "bashism" from contrib/thunderbird-patch-inline/appp.sh

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

Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>
---

Maxin john wrote:

This is wrong.

You are replacing bash with sh:

but the script still uses bash-specific syntax (aka. bashishms):

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

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

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

Not really.

Do you mean some of the parts you quoted are bashism?

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

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

Junio C Hamano wrote:

I was pointing to the $( ) as a bashishm

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.

Hi,

..

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

Maxin john <maxin@maxinbjohn.info> writes:

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.

Hi,

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

Thank you very much.

Best Regards, Maxin B. John

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

Maxin john wrote:

This is wrong.

You are replacing bash with sh:

but the script still uses bash-specific syntax (aka. bashishms):

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

Maxin john <maxin@maxinbjohn.info> writes:

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

Hi,

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:

On 03/29/2011 09:09 AM, Junio C Hamano wrote:

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

Alternatively:

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

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