{"thread":{"id":"26880","subject":"[PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","startedAt":"2011-03-27T14:37:04Z","lastAt":"2011-03-31T21:56:45Z","messageCount":13,"participants":["Maxin john","Ángel González","Junio C Hamano","Victor Engmark"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"164389","messageId":"AANLkTin-USDnTxeKT_KOZW5kgC0vFXYbMNP9ct6fzbUC@mail.gmail.com","threadId":"26880","inReplyTo":null,"subject":"[PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Maxin john","fromEmail":"maxin@maxinbjohn.info","sentAt":"2011-03-27T14:37:04Z","receivedAt":"2011-03-27T14:37:04Z","isPatch":true,"sender":{"key":"maxin@maxinbjohn.info","avatar":null},"body":"Remove \"bashism\" and minor corrections for\ncontrib/thunderbird-patch-inline/appp.sh\n\nSigned-off-by: Maxin B. John \n---\ndiff --git a/contrib/thunderbird-patch-inline/appp.sh\nb/contrib/thunderbird-patch-inline/appp.sh\nindex cc518f3..1d29f4b 100755\n--- a/contrib/thunderbird-patch-inline/appp.sh\n+++ b/contrib/thunderbird-patch-inline/appp.sh\n@@ -1,8 +1,8 @@\n-#!/bin/bash\n+#!/bin/sh\n # Copyright 2008 Lukas Sandström \n #\n # AppendPatch - A script to be used together with ExternalEditor\n-# for Mozilla Thunderbird to properly include pathes inline i e-mails.\n+# for Mozilla Thunderbird to properly include patches inline in e-mails.\n\n # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2\n\n@@ -16,6 +16,11 @@ else\n cd > /dev/null\n fi\n\n+#check whether zenity is present\n+if ! type zenity >/dev/null 2>&1 ; then\n+ exit 1\n+fi\n+\n PATCH=$(zenity --file-selection)\n\n if [ \"$?\" != \"0\" ] ; then\n"},{"id":"164517","messageId":"4D9103D3.5010403@zoho.com","threadId":"26880","inReplyTo":"AANLkTin-USDnTxeKT_KOZW5kgC0vFXYbMNP9ct6fzbUC@mail.gmail.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Ángel González","fromEmail":"ingenit@zoho.com","sentAt":"2011-03-28T21:55:31Z","receivedAt":"2011-03-28T21:55:31Z","isPatch":true,"sender":{"key":"ingenit@zoho.com","avatar":null},"body":"Maxin john wrote:\n> Remove \"bashism\" and minor corrections for\n> contrib/thunderbird-patch-inline/appp.sh\n> \n> Signed-off-by: Maxin B. John \n\nThis is wrong.\n\nYou are replacing bash with sh:\n> -#!/bin/bash\n> +#!/bin/sh\n\nbut the script still uses bash-specific syntax (aka. bashishms):\n> +\n> PATCH=$(zenity --file-selection)\n> \n> if [ \"$?\" != \"0\" ] ; then\n\nSo with your change the script won't be able to run on systems which\ndon't have bash as /bin/sh\n"},{"id":"164513","messageId":"4D9124C0.8070003@zoho.com","threadId":"26880","inReplyTo":"AANLkTin-USDnTxeKT_KOZW5kgC0vFXYbMNP9ct6fzbUC@mail.gmail.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Ángel González","fromEmail":"ingenit@zoho.com","sentAt":"2011-03-29T00:16:00Z","receivedAt":"2011-03-29T00:16:00Z","isPatch":true,"sender":{"key":"ingenit@zoho.com","avatar":null},"body":"Maxin john wrote:\n> > Remove \"bashism\" and minor corrections for\n> > contrib/thunderbird-patch-inline/appp.sh\n> >\n> > Signed-off-by: Maxin B. John \nThis is wrong.\n\nYou are replacing bash with sh:\n> > -#!/bin/bash\n> > +#!/bin/sh\n\nbut the script still uses bash-specific syntax (aka. bashishms):\n> > +\n> > PATCH=$(zenity --file-selection)\n> >\n> > if [ \"$?\" != \"0\" ] ; then\n\nSo with your change the script won't be able to run on systems which\ndon't have bash as /bin/sh\n\nThe standard equivalent of $( ) are `backticks`.\n"},{"id":"164516","messageId":"7v7hbiviwv.fsf@alter.siamese.dyndns.org","threadId":"26880","inReplyTo":"AANLkTin-USDnTxeKT_KOZW5kgC0vFXYbMNP9ct6fzbUC@mail.gmail.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-29T01:01:52Z","receivedAt":"2011-03-29T01:01:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maxin john writes:\n\n> Remove \"bashism\" and minor corrections for\n> contrib/thunderbird-patch-inline/appp.sh\n\nThe script seems to only use standard POSIX shell features and nothing\nparticularly bash specific nor outside POSIX in general that we exclude\nfrom our coding standards (e.g. \"local\", use of \"function\" noiseword,\nsubstring expansion ${parmeter:offset:length}).\n\nIt is better to explain the patch as\n\n Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run\n"},{"id":"164534","messageId":"AANLkTi=Nd897+GrfmPS38aQo_Vh-J9cJr9bB-UzN_CqQ@mail.gmail.com","threadId":"26880","inReplyTo":"7v7hbiviwv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Maxin john","fromEmail":"maxin@maxinbjohn.info","sentAt":"2011-03-29T06:47:41Z","receivedAt":"2011-03-29T06:47:41Z","isPatch":true,"sender":{"key":"maxin@maxinbjohn.info","avatar":null},"body":"Hi,\n\n> It is better to explain the patch as\n>\n> Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run\n\nYes. I do agree with you. I should have chosen a better Subject line\nfor this patch. Should I resend this patch with this modified Subject\nline ?\n\nThanks and Regards,\nMaxin B. John\n\n\nOn Tue, Mar 29, 2011 at 4:01 AM, Junio C Hamano wrote:\n> Maxin john writes:\n>\n>> Remove \"bashism\" and minor corrections for\n>> contrib/thunderbird-patch-inline/appp.sh\n>\n> The script seems to only use standard POSIX shell features and nothing\n> particularly bash specific nor outside POSIX in general that we exclude\n> from our coding standards (e.g. \"local\", use of \"function\" noiseword,\n> substring expansion ${parmeter:offset:length}).\n>\n> It is better to explain the patch as\n>\n>  Subject: contrib/thunderbird-patch-inline: do not require /bin/bash to run\n>\n>\n"},{"id":"164537","messageId":"AANLkTikaZA=7EFMVf1hwEoQJd6hChha0cCL7ZRMEZXyS@mail.gmail.com","threadId":"26880","inReplyTo":"4D9103D3.5010403@zoho.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Maxin john","fromEmail":"maxin@maxinbjohn.info","sentAt":"2011-03-29T06:54:37Z","receivedAt":"2011-03-29T06:54:37Z","isPatch":true,"sender":{"key":"maxin@maxinbjohn.info","avatar":null},"body":"Hi,\n\nThank you very much for the suggestions. However, I have tested this\nscript in Ubuntu which uses dash as /bin/sh\n\nEg: the following script runs successfully in Ubuntu 10.10\n\n#!/bin/dash\n\nPATCH=$(zenity --file-selection)\n\nif [ \"$?\" != \"0\" ] ; then\n echo \"zenity failed\"\nelse\n echo \"success\"\nfi\n\nI haven't confirmed this in other shell implementations. Please let me\nknow your comments on this.\n\nBest Regards,\nMaxin B. John\n\n2011/3/29 Ángel González :\n> Maxin john wrote:\n>> Remove \"bashism\" and minor corrections for\n>> contrib/thunderbird-patch-inline/appp.sh\n>>\n>> Signed-off-by: Maxin B. John \n>\n> This is wrong.\n>\n> You are replacing bash with sh:\n>> -#!/bin/bash\n>> +#!/bin/sh\n>\n> but the script still uses bash-specific syntax (aka. bashishms):\n>> +\n>>  PATCH=$(zenity --file-selection)\n>>\n>>  if [ \"$?\" != \"0\" ] ; then\n>\n> So with your change the script won't be able to run on systems which\n> don't have bash as /bin/sh\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"164541","messageId":"7vei5qtnc5.fsf@alter.siamese.dyndns.org","threadId":"26880","inReplyTo":"4D9103D3.5010403@zoho.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-29T07:09:14Z","receivedAt":"2011-03-29T07:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ángel González writes:\n\n> This is wrong.\n\nNot really.\n\n> You are replacing bash with sh:\n>> -#!/bin/bash\n>> +#!/bin/sh\n>\n> but the script still uses bash-specific syntax (aka. bashishms):\n\nDo you mean some of the parts you quoted are bashism?\n\n>> PATCH=$(zenity --file-selection)\n\nEven though ancient shells I grew up with did not have $(), it is a way\nbackticks should have been written by Bourne from day one. Historically,\nhandling nesting and interraction between double-quotes and backticks\ncorrectly was a nightmare to get right, and different implementations of\nshells got them always wrong. If you use $(), the headaches go away.\n\nThese days, we don't know of any POSIX shell that is widely used and does\nnot understand $(). As such, the above construct is perfectly safe and\neven preferred over ``. Welcome to the 21st century ;-)\n\n>> if [ \"$?\" != \"0\" ] ; then\n\nWhile I personally do not like this style (I am old fashioned) and would\nprobably write:\n\n\tif test $? != 0\n then\n \t...\n\nor make it even more readable by writing it together with the previous\nstatement, i.e.\n\n\tPATCH=$(zenity --file-selection) ||\n ...\n\nmyself, it is definitely not bash-ism to use [] for conditionals. Some\npeople seem to find it more readable than traditional \"test\" (not me).\n\nThe only major platform that didn't have a reasonable shell was Solaris,\nbut we already have written its /bin/sh off as broken and unusable, and\nsuggest people to use xpg4 or xpg6 shell (see the Makefile).\n"},{"id":"164574","messageId":"4D91E6AE.9040208@terreactive.ch","threadId":"26880","inReplyTo":"8721039.4955.1301382568626.JavaMail.trustmail@mail1.terreactive.ch","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Victor Engmark","fromEmail":"victor.engmark@terreactive.ch","sentAt":"2011-03-29T14:03:26Z","receivedAt":"2011-03-29T14:03:26Z","isPatch":true,"sender":{"key":"victor.engmark@terreactive.ch","avatar":null},"body":"On 03/29/2011 09:09 AM, Junio C Hamano wrote:\n> Ángel González writes:\n\n>>> if [ \"$?\" != \"0\" ] ; then\n> \n> While I personally do not like this style (I am old fashioned) and would\n> probably write:\n> \n> \tif test $? != 0\n> then\n> \t...\n\nNitpicking I suppose, but since `$?` is always an integer we should use\n`-ne` (positive/negative integers) instead of `!=` (string comparison).\n\n> or make it even more readable by writing it together with the previous\n> statement, i.e.\n> \n> \tPATCH=$(zenity --file-selection) ||\n> ...\n> \n> myself, it is definitely not bash-ism to use [] for conditionals. Some\n> people seem to find it more readable than traditional \"test\" (not me).\n\nAlternatively:\n\nif ! PATCH=$(zenity --file-selection)\nthen\n...\n\nYep, that works in dash - Both variable assignment and exit code checking.\n\n-- \nVictor Engmark\n"},{"id":"164640","messageId":"4D9261AE.5070103@zoho.com","threadId":"26880","inReplyTo":"7vei5qtnc5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Ángel González","fromEmail":"ingenit@zoho.com","sentAt":"2011-03-29T22:48:14Z","receivedAt":"2011-03-29T22:48:14Z","isPatch":true,"sender":{"key":"ingenit@zoho.com","avatar":null},"body":"Junio C Hamano wrote:\n> Ángel González writes:\n> \n>> This is wrong.\n> \n> Not really.\n> \n>> You are replacing bash with sh:\n>>> -#!/bin/bash\n>>> +#!/bin/sh\n>>\n>> but the script still uses bash-specific syntax (aka. bashishms):\n> \n> Do you mean some of the parts you quoted are bashism?\n\nI was pointing to the $( ) as a bashishm\n\n>>> PATCH=$(zenity --file-selection)\n> \n> Even though ancient shells I grew up with did not have $(), it is a way\n> backticks should have been written by Bourne from day one. Historically,\n> handling nesting and interraction between double-quotes and backticks\n> correctly was a nightmare to get right, and different implementations of\n> shells got them always wrong. If you use $(), the headaches go away.\n\n> These days, we don't know of any POSIX shell that is widely used and does\n> not understand $(). As such, the above construct is perfectly safe and\n> even preferred over ``. Welcome to the 21st century ;-)\n>\n> The only major platform that didn't have a reasonable shell was Solaris,\n> but we already have written its /bin/sh off as broken and unusable, and\n> suggest people to use xpg4 or xpg6 shell (see the Makefile).\n\nI have to agree with you. $() is a much saner syntax. Still, the goal\nwas portability.\nReading your message, and considering the Solaris note, it might have\nbeen fine as it was. I have also checked the \"Shell Command Language\"\nsection of IEEE Std 1003.1 and it does require $() use.\n\nAlbeit being a single line I would still change it, it is now a much\nweaker position. Thanks for your insight.\n"},{"id":"164672","messageId":"AANLkTim+0gxGKZT=vfmX7v0QZrApjRwAzW3PiLePL-iQ@mail.gmail.com","threadId":"26880","inReplyTo":"4D9261AE.5070103@zoho.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Maxin john","fromEmail":"maxin@maxinbjohn.info","sentAt":"2011-03-30T08:52:23Z","receivedAt":"2011-03-30T08:52:23Z","isPatch":true,"sender":{"key":"maxin@maxinbjohn.info","avatar":null},"body":"Hi,\n\n> Junio C Hamano wrote:\n..\n>> Even though ancient shells I grew up with did not have $(), it is a way\n>> backticks should have been written by Bourne from day one.  Historically,\n>> handling nesting and interraction between double-quotes and backticks\n>> correctly was a nightmare to get right, and different implementations of\n>> shells got them always wrong.  If you use $(), the headaches go away.\n>> These days, we don't know of any POSIX shell that is widely used and does\n>> not understand $().  As such, the above construct is perfectly safe and\n>> even preferred over ``.  Welcome to the 21st century ;-)\n>>\n>> The only major platform that didn't have a reasonable shell was Solaris,\n>> but we already have written its /bin/sh off as broken and unusable, and\n>> suggest people to use xpg4 or xpg6 shell (see the Makefile).\n\nThank you very much for sharing this information. It was really really\ninformative.\nThanks to Ángel González and Victor Engmark for sharing their views.\n\nConsidering all the suggestions, I think, it is \"not possible to\nsatisfy everyone\" :)\nSo, I have modified the patch by incorporating most of the nice suggestions.\n\nPlease let me know your comments.\n\nSigned-off-by: Maxin B. John \n---\ndiff --git a/contrib/thunderbird-patch-inline/appp.sh\nb/contrib/thunderbird-patch-inline/appp.sh\nindex cc518f3..20dac9f 100755\n--- a/contrib/thunderbird-patch-inline/appp.sh\n+++ b/contrib/thunderbird-patch-inline/appp.sh\n@@ -1,8 +1,8 @@\n-#!/bin/bash\n+#!/bin/sh\n # Copyright 2008 Lukas Sandström \n #\n # AppendPatch - A script to be used together with ExternalEditor\n-# for Mozilla Thunderbird to properly include pathes inline i e-mails.\n+# for Mozilla Thunderbird to properly include patches inline in e-mails.\n\n # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2\n\n@@ -16,13 +16,12 @@ else\n cd > /dev/null\n fi\n\n-PATCH=$(zenity --file-selection)\n-\n-if [ \"$?\" != \"0\" ] ; then\n- #zenity --error --text \"No patchfile given.\"\n- exit 1\n+#check whether zenity is present\n+if ! type zenity >/dev/null 2>&1 ; then\n+ exit 1\n fi\n\n+PATCH=$(zenity --file-selection) || exit 1\n cd - > /dev/null\n\n SUBJECT=`sed -n -e '/^Subject: /p' \"${PATCH}\"`\n"},{"id":"164701","messageId":"7vmxkco5jg.fsf@alter.siamese.dyndns.org","threadId":"26880","inReplyTo":"AANLkTim+0gxGKZT=vfmX7v0QZrApjRwAzW3PiLePL-iQ@mail.gmail.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-30T17:57:07Z","receivedAt":"2011-03-30T17:57:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maxin john writes:\n\n> So, I have modified the patch by incorporating most of the nice suggestions.\n\nI'd just replace /bin/bash with /bin/sh without any other fuss, perhaps\nexcept for the typofix in the comment, and be done with the topic.\n\nThe script in the current version may look ugly to my eyes and others, but\nthere is nothing _wrong_ in it per-se. Rewriting them in different styles\nis not necessarily improvement, and this is only a contrib/ material after\nall.\n\nThanks, I'll apply the early hunks from you.\n"},{"id":"164706","messageId":"AANLkTinrfswqETPVjDEuKon8ntcgUpkizxit84b4imno@mail.gmail.com","threadId":"26880","inReplyTo":"7vmxkco5jg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Maxin john","fromEmail":"maxin@maxinbjohn.info","sentAt":"2011-03-30T18:51:25Z","receivedAt":"2011-03-30T18:51:25Z","isPatch":true,"sender":{"key":"maxin@maxinbjohn.info","avatar":null},"body":"Hi,\n\n>\n> I'd just replace /bin/bash with /bin/sh without any other fuss, perhaps\n> except for the typofix in the comment, and be done with the topic.\n>\n\nI agree with this. The changes were mostly cosmetic and has nothing to\ndo with the functionality of the script.\n\n>\n> Thanks, I'll apply the early hunks from you.\n>\n\nThank you very much.\n\nBest Regards,\nMaxin B. John\n"},{"id":"164837","messageId":"7vvcyzhs2q.fsf@alter.siamese.dyndns.org","threadId":"26880","inReplyTo":"AANLkTim+0gxGKZT=vfmX7v0QZrApjRwAzW3PiLePL-iQ@mail.gmail.com","subject":"Re: [PATCH] Remove \"bashism\" from contrib/thunderbird-patch-inline/appp.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-31T21:56:45Z","receivedAt":"2011-03-31T21:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just for the record, the patch at the bottom is what I queued.\n\n-- >8 --\nFrom: Maxin john \nSubject: [PATCH] contrib/thunderbird-patch-inline: do not require bash to run the script\n\nThe script does not have to be run under bash, but any POSIX compliant\nshell would do, as it does not use any bash-isms.\n\nIt may be written under a different style than what is recommended in\nDocumentation/CodingGuidelines, but that is a different matter.\n\nWhile at it, fix obvious typos in the comment.\n\nSigned-off-by: Maxin B. John \nSigned-off-by: Junio C Hamano \n---\n contrib/thunderbird-patch-inline/appp.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/thunderbird-patch-inline/appp.sh b/contrib/thunderbird-patch-inline/appp.sh\nindex cc518f3..5eb4a51 100755\n--- a/contrib/thunderbird-patch-inline/appp.sh\n+++ b/contrib/thunderbird-patch-inline/appp.sh\n@@ -1,8 +1,8 @@\n-#!/bin/bash\n+#!/bin/sh\n # Copyright 2008 Lukas Sandström \n #\n # AppendPatch - A script to be used together with ExternalEditor\n-# for Mozilla Thunderbird to properly include pathes inline i e-mails.\n+# for Mozilla Thunderbird to properly include patches inline in e-mails.\n \n # ExternalEditor can be downloaded at http://globs.org/articles.php?lng=en&pg=2\n \n-- \n1.7.4.2.422.g537d99\n"}]}