{"thread":{"id":"28303","subject":"[PATCH] shell portability: Use sed instead of non-portable variable expansion","startedAt":"2011-09-05T05:11:47Z","lastAt":"2011-09-06T20:30:33Z","messageCount":19,"participants":["Naohiro Aota","Johannes Sixt","Junio C Hamano","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174845","messageId":"8762l73758.fsf@elisp.net","threadId":"28303","inReplyTo":null,"subject":"[PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Naohiro Aota","fromEmail":"naota@elisp.net","sentAt":"2011-09-05T05:11:47Z","receivedAt":"2011-09-05T05:11:47Z","isPatch":true,"sender":{"key":"naota@elisp.net","avatar":null},"body":"Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\nshells like FreeBSD sh and they made the test to fail. This patch\nreplace such variable expansions with sed.\n\nSigned-off-by: Naohiro Aota \n---\n\nTesting on FreeBSD failed because of this \"bash-ism\". After applying\nthis patch, I've verified the test to pass on FreeBSD. (and it worked\nwell also with GNU sed)\n\n t/t5560-http-backend-noserver.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 0ad7ce0..c8237ef 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -9,8 +9,8 @@ test_have_prereq MINGW && export GREP_OPTIONS=-U\n \n run_backend() {\n \techo \"$2\" |\n-\tQUERY_STRING=\"${1#*\\?}\" \\\n-\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n+\tQUERY_STRING=$(echo \"$1\"|sed -e 's/^[^?]*?\\(.*\\)$/\\1/') \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/$(echo \"$1\"|sed -e 's/^\\([^?]*\\)?.*$/\\1/')\" \\\n \tgit http-backend >act.out 2>act.err\n }\n \n-- \n1.7.6\n"},{"id":"174847","messageId":"4E647442.9000005@viscovery.net","threadId":"28303","inReplyTo":"8762l73758.fsf@elisp.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-05T07:03:30Z","receivedAt":"2011-09-05T07:03:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/5/2011 7:11, schrieb Naohiro Aota:\n> Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\n> shells like FreeBSD sh and they made the test to fail. This patch\n> replace such variable expansions with sed.\n> \n> Signed-off-by: Naohiro Aota \n> ---\n> \n> Testing on FreeBSD failed because of this \"bash-ism\".\n\nThese are not bashism, but features require by POSIX.\n\nI'd rather suspect that the failures are not because FreeBSD sh does not\nhave ${%} or ${#}, but rather that it interprets the meaning of the\nbackslash in this case in a way different from other shells.\n\n> run_backend() {\n> \techo \"$2\" |\n> -\tQUERY_STRING=\"${1#*\\?}\" \\\n> -\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n\nWhat happens if you write these as\n\n\tQUERY_STRING=${1#*\\?} \\\n\tPATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*} \\\n\ni.e., drop the double-quotes?\n\n-- Hannes\n"},{"id":"174848","messageId":"7vbouzxy7g.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"8762l73758.fsf@elisp.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-05T07:09:07Z","receivedAt":"2011-09-05T07:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Naohiro Aota writes:\n\n> Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\n> shells like FreeBSD sh and they made the test to fail.\n\nSorry, I do appreciate the effort, but a patch like this takes us in the\nwrong direction.\n\nWhile we do not allow blatant bashisms like ${parameter:offset:length}\n(substring expansion), ${parameter/pattern/string} (pattern substitution),\n\"local\" variables, \"function\" noiseword, and shell arrays in our shell\nscripts, the two kinds of substitution you quoted above are purely POSIX,\nand our coding guideline does allow them to be used in the scripts.\n\nEven though you may be able to rewrite trivial cases easily in some\nscripts (either tests or Porcelain), some Porcelain scripts we ship\n(e.g. \"git bisect\", \"git stash\", \"git pull\", etc.) do use these POSIX\nconstructs, and we do not want to butcher them with extra forks and\nreduced readability.\n\nPlease use $SHELL_PATH and point to a POSIX compliant shell on your\nplatform instead. \"make test\" should pick it up and pass it down to\nt/Makefile to be used when it runs these test scripts.\n\nBesides, even inside t/ directory, there are many other instances of these\nprefix/postfix substitution, not just 5560. Do the following tests pass on\nyour box without a similar patch?\n\n$ git grep -n -e '\\${[^}]*[#%]' -- t/\\*.sh\nt/t1410-reflog.sh:33:\taa=${1%??????????????????????????????????????} zz=${1#??}\nt/t1410-reflog.sh:38:\taa=${1%??????????????????????????????????????} zz=${1#??}\nt/t2030-unresolve-info.sh:125:\trerere_id=${rerere_id%/postimage} &&\nt/t2030-unresolve-info.sh:151:\trerere_id=${rerere_id%/postimage} &&\nt/t5560-http-backend-noserver.sh:12:\tQUERY_STRING=\"${1#*\\?}\" \\\nt/t5560-http-backend-noserver.sh:13:\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\nt/t6050-replace.sh:124: aa=${HASH2%??????????????????????????????????????} &&\nt/t9010-svn-fe.sh:17:\t\tprintf \"%s\\n\" \"K ${#property}\" &&\nt/t9010-svn-fe.sh:19:\t\tprintf \"%s\\n\" \"V ${#value}\" &&\nt/t9010-svn-fe.sh:30:\tprintf \"%s\\n\" \"Text-content-length: ${#text}\" &&\nt/t9010-svn-fe.sh:31:\tprintf \"%s\\n\" \"Content-length: $((${#text} + 10))\" &&\nt/test-lib.sh:838:\t\ttest_results_path=\"$test_results_dir/${0%.sh}-$$.counts\"\nt/test-lib.sh:1047:this_test=${0##*/}\nt/test-lib.sh:1048:this_test=${this_test%%-*}\nt/valgrind/analyze.sh:98:\t\t\t\ttest $output = ${output%.message} &&\n\nLooking at the above output, I suspect that it _might_ be that your shell\nis almost POSIX but does not handle the backslash-quoted question mark\ncorrectly or something silly like that, in which case a stupid patch like\nthe attached might be an acceptable compromise, until the shell is fixed.\n\nBy the way, t9010 uses ${#parameter} (strlen) which is bashism we forbid,\nand it needs to be rewritten (David CC'ed).\n\nThanks.\n\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 0ad7ce0..c8bbacc 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -9,8 +9,8 @@ test_have_prereq MINGW && export GREP_OPTIONS=-U\n \n run_backend() {\n \techo \"$2\" |\n-\tQUERY_STRING=\"${1#*\\?}\" \\\n-\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n+\tQUERY_STRING=\"${1#*[?]}\" \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n \tgit http-backend >act.out 2>act.err\n }\n \n"},{"id":"174849","messageId":"7v7h5nxxwf.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"4E647442.9000005@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-05T07:15:44Z","receivedAt":"2011-09-05T07:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> run_backend() {\n>> \techo \"$2\" |\n>> -\tQUERY_STRING=\"${1#*\\?}\" \\\n>> -\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n>\n> What happens if you write these as\n>\n> \tQUERY_STRING=${1#*\\?} \\\n> \tPATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*} \\\n>\n> i.e., drop the double-quotes?\n\nInteresting. Your conjecture is that the shell may be dropping the\nbackslash inside dq context when it does not understand what follows the\nbackslash, i.e. \"\\?\" -> \"?\", losing the quote. I find it very plausible.\n\nIf that is the case, either the above or my [?] would work it around, I\nwould think.\n\nThanks.\n"},{"id":"174850","messageId":"4E647BD5.8060609@viscovery.net","threadId":"28303","inReplyTo":"7v7h5nxxwf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-05T07:35:49Z","receivedAt":"2011-09-05T07:35:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/5/2011 9:15, schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>>> run_backend() {\n>>> \techo \"$2\" |\n>>> -\tQUERY_STRING=\"${1#*\\?}\" \\\n>>> -\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n>>\n>> What happens if you write these as\n>>\n>> \tQUERY_STRING=${1#*\\?} \\\n>> \tPATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*} \\\n>>\n>> i.e., drop the double-quotes?\n> \n> Interesting. Your conjecture is that the shell may be dropping the\n> backslash inside dq context when it does not understand what follows the\n> backslash, i.e. \"\\?\" -> \"?\", losing the quote. I find it very plausible.\n\nActually, it's the opposite: Within double-quotes, a backslash is only\nremoved when the next character has a special meaning (essentially $, `,\n\", \\), otherwise, it remains and loses its quoting ability. This means,\nthat the backslash would remain as a literal character in our patterns on\nthe right of % or #, and they would not work anymore as intended.\n\nOther shells seem to parse the pattern following % and # in a different\nmode, which keeps the quoting ability of the backslash even inside\ndouble-quotes... (And to me it looks like those shells are wrong.)\n\nWithout double-quotes, backslashes (that are not themselves quoted) are\nalways removed and give the subsequent character its literal meaning.\nHence, in my version, the question mark would unambiguously (I think) act\nas a literal rather than a wildcard.\n\n> If that is the case, either the above or my [?] would work it around, I\n> would think.\n\n[?] instead of \\? is certainly also worth a try.\n\n-- Hannes\n"},{"id":"174851","messageId":"7v39gbxwi6.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"4E647BD5.8060609@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-05T07:45:53Z","receivedAt":"2011-09-05T07:45:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Actually, it's the opposite: Within double-quotes, a backslash is only\n> removed when the next character has a special meaning (essentially $, `,\n> \", \\), otherwise, it remains and loses its quoting ability. This means,\n> that the backslash would remain as a literal character in our patterns on\n> the right of % or #, and they would not work anymore as intended.\n\nThat's strange...\n\nI thought that VAR= would behave\nidentically to VAR=\"\". You seem to be saying\nthat they should act differently.\n\n>> If that is the case, either the above or my [?] would work it around, I\n>> would think.\n>\n> [?] instead of \\? is certainly also worth a try.\n\nI obviously agree. Besides, [?] would sidestep the tricky backslash vs\ndouble quote issue entirely, so it would be a more robust solution to\nleave it around than \"sometimes you need to avoid double-quote and some\nother times you would need double-quote\" for other people to mimic writing\ntests later.\n"},{"id":"174852","messageId":"4E648031.6050607@viscovery.net","threadId":"28303","inReplyTo":"7vbouzxy7g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-05T07:54:25Z","receivedAt":"2011-09-05T07:54:25Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/5/2011 9:09, schrieb Junio C Hamano:\n> By the way, t9010 uses ${#parameter} (strlen) which is bashism we forbid,\n> and it needs to be rewritten (David CC'ed).\n\nActually, no. It is perfectly valid POSIX. So we would need this patch.\n\n--- 8< ---\nFrom: Johannes Sixt \nSubject: [PATCH] CodingGuidelines: ${#parameter} is POSIX and should be allowed\n\nSee http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02.\n\nSigned-off-by: Johannes Sixt \n---\n Documentation/CodingGuidelines | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex fe1c1e5..df0b620 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -52,8 +52,6 @@ For shell scripts specifically (not exhaustive):\n \n - No shell arrays.\n \n- - No strlen ${#parameter}.\n-\n - No pattern replacement ${parameter/pattern/string}.\n \n - We use Arithmetic Expansion $(( ... )).\n-- \n1.7.7.rc0.211.g5ac7e\n"},{"id":"174853","messageId":"87fwkbzama.fsf@elisp.net","threadId":"28303","inReplyTo":"4E647442.9000005@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Naohiro Aota","fromEmail":"naota@elisp.net","sentAt":"2011-09-05T07:55:41Z","receivedAt":"2011-09-05T07:55:41Z","isPatch":true,"sender":{"key":"naota@elisp.net","avatar":null},"body":"Ah, seems I was misundertanding much of things :(\n\nJohannes Sixt writes:\n\n> What happens if you write these as\n>\n> \tQUERY_STRING=${1#*\\?} \\\n> \tPATH_TRANSLATED=$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*} \\\n>\n> i.e., drop the double-quotes?\n\nnot worked, even increased the number of failure...\n\nJunio C Hamano writes:\n\n> Naohiro Aota writes:\n>\n>> Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\n>> shells like FreeBSD sh and they made the test to fail.\n>\n> Sorry, I do appreciate the effort, but a patch like this takes us in the\n> wrong direction.\n>\n> While we do not allow blatant bashisms like ${parameter:offset:length}\n> (substring expansion), ${parameter/pattern/string} (pattern substitution),\n> \"local\" variables, \"function\" noiseword, and shell arrays in our shell\n> scripts, the two kinds of substitution you quoted above are purely POSIX,\n> and our coding guideline does allow them to be used in the scripts.\n>\n> Even though you may be able to rewrite trivial cases easily in some\n> scripts (either tests or Porcelain), some Porcelain scripts we ship\n> (e.g. \"git bisect\", \"git stash\", \"git pull\", etc.) do use these POSIX\n> constructs, and we do not want to butcher them with extra forks and\n> reduced readability.\n>\n> Please use $SHELL_PATH and point to a POSIX compliant shell on your\n> platform instead. \"make test\" should pick it up and pass it down to\n> t/Makefile to be used when it runs these test scripts.\n\nThanks, I'll try this.\n\n> Besides, even inside t/ directory, there are many other instances of these\n> prefix/postfix substitution, not just 5560. Do the following tests pass on\n> your box without a similar patch?\n>\n> $ git grep -n -e '\\${[^}]*[#%]' -- t/\\*.sh\n> t/t1410-reflog.sh:33:\taa=${1%??????????????????????????????????????} zz=${1#??}\n> t/t1410-reflog.sh:38:\taa=${1%??????????????????????????????????????} zz=${1#??}\n> t/t2030-unresolve-info.sh:125:\trerere_id=${rerere_id%/postimage} &&\n> t/t2030-unresolve-info.sh:151:\trerere_id=${rerere_id%/postimage} &&\n> t/t5560-http-backend-noserver.sh:12:\tQUERY_STRING=\"${1#*\\?}\" \\\n> t/t5560-http-backend-noserver.sh:13:\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n> t/t6050-replace.sh:124: aa=${HASH2%??????????????????????????????????????} &&\n> t/t9010-svn-fe.sh:17:\t\tprintf \"%s\\n\" \"K ${#property}\" &&\n> t/t9010-svn-fe.sh:19:\t\tprintf \"%s\\n\" \"V ${#value}\" &&\n> t/t9010-svn-fe.sh:30:\tprintf \"%s\\n\" \"Text-content-length: ${#text}\" &&\n> t/t9010-svn-fe.sh:31:\tprintf \"%s\\n\" \"Content-length: $((${#text} + 10))\" &&\n> t/test-lib.sh:838:\t\ttest_results_path=\"$test_results_dir/${0%.sh}-$$.counts\"\n> t/test-lib.sh:1047:this_test=${0##*/}\n> t/test-lib.sh:1048:this_test=${this_test%%-*}\n> t/valgrind/analyze.sh:98:\t\t\t\ttest $output = ${output%.message} &&\n\nI've tried t[0-9]{4}-*.sh and all of them passed. (t9010 had some known\nbreakages) yeah, my patch was taking wrong way.\n\n> Looking at the above output, I suspect that it _might_ be that your shell\n> is almost POSIX but does not handle the backslash-quoted question mark\n> correctly or something silly like that, in which case a stupid patch like\n> the attached might be an acceptable compromise, until the shell is fixed.\n>\n> diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\n> index 0ad7ce0..c8bbacc 100755\n> --- a/t/t5560-http-backend-noserver.sh\n> +++ b/t/t5560-http-backend-noserver.sh\n> @@ -9,8 +9,8 @@ test_have_prereq MINGW && export GREP_OPTIONS=-U\n> \n> run_backend() {\n> \techo \"$2\" |\n> -\tQUERY_STRING=\"${1#*\\?}\" \\\n> -\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n> +\tQUERY_STRING=\"${1#*[?]}\" \\\n> +\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n> \tgit http-backend >act.out 2>act.err\n> }\n\nThis worked on my box. hm, then the problem should be in /bin/sh\n"},{"id":"174854","messageId":"7vehzvcst5.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"4E648031.6050607@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-05T08:11:18Z","receivedAt":"2011-09-05T08:11:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 9/5/2011 9:09, schrieb Junio C Hamano:\n>> By the way, t9010 uses ${#parameter} (strlen) which is bashism we forbid,\n>> and it needs to be rewritten (David CC'ed).\n>\n> Actually, no. It is perfectly valid POSIX. So we would need this patch.\n\nI know it is in POSIX, but not in the subset we allowed so far. I do not\nrecall the details offhand, but we must have seen some shell that lacked\nit or something.\n"},{"id":"174855","messageId":"4E648546.8060303@viscovery.net","threadId":"28303","inReplyTo":"7v39gbxwi6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-05T08:16:06Z","receivedAt":"2011-09-05T08:16:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/5/2011 9:45, schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> Actually, it's the opposite: Within double-quotes, a backslash is only\n>> removed when the next character has a special meaning (essentially $, `,\n>> \", \\), otherwise, it remains and loses its quoting ability. This means,\n>> that the backslash would remain as a literal character in our patterns on\n>> the right of % or #, and they would not work anymore as intended.\n> \n> That's strange...\n> \n> I thought that VAR= would behave\n> identically to VAR=\"\". You seem to be saying\n> that they should act differently.\n\nThey are not the same.\n\nFirst of all, the value of $IFS is irrelevant whether or not you need\ndouble-quotes on the RHS of an assignment, because it is purely a\nsyntactic matter; $IFS plays no role during syntax analysis. It is only\nthe presence of white-space that sometimes[*] requires quoting of some form.\n\nThe most visible difference is a backslash that is followed by a character\nthat is not special:\n\n$ foo=\"a\\xb\" env | grep foo; foo=a\\xb env | grep foo\nfoo=a\\xb\nfoo=axb\n\nBut it is the same elsewhere in a command:\n\n$ echo \"a\\xb\"; echo a\\xb\na\\xb\naxb\n\nThe reason is that a backslash inside double-quotes remains as a literal\ncharacter when it is not followed by a special character, whereas outside\ndouble-quotes an unquoted backslash is always removed.\n\n[*] No quoting is required in cases like this: VAR=$(echo foo)\n\n>> [?] instead of \\? is certainly also worth a try.\n> \n> I obviously agree. Besides, [?] would sidestep the tricky backslash vs\n> double quote issue entirely, so it would be a more robust solution to\n> leave it around than \"sometimes you need to avoid double-quote and some\n> other times you would need double-quote\" for other people to mimic writing\n> tests later.\n\nGood point, and I shall prefer this solution as well.\n\n-- Hannes\n"},{"id":"174856","messageId":"4E64869D.8060601@viscovery.net","threadId":"28303","inReplyTo":"4E648546.8060303@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-09-05T08:21:49Z","receivedAt":"2011-09-05T08:21:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 9/5/2011 10:16, schrieb Johannes Sixt:\n> The most visible difference is a backslash that is followed by a character\n> that is not special:\n\nI should have said: \"The difference that I can see immediately is...\". I\nsuspect there are other subtle differences that are not so obvious to me.\n\n-- Hannes\n"},{"id":"174857","messageId":"7vaaajcsb7.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"4E648031.6050607@viscovery.net","subject":"Re: [PATCH] shell portability: Use sed instead of non-portable variable expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-05T08:22:04Z","receivedAt":"2011-09-05T08:22:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 9/5/2011 9:09, schrieb Junio C Hamano:\n>> By the way, t9010 uses ${#parameter} (strlen) which is bashism we forbid,\n>> and it needs to be rewritten (David CC'ed).\n>\n> Actually, no. It is perfectly valid POSIX. So we would need this patch.\n>\n> --- 8< ---\n> From: Johannes Sixt \n> Subject: [PATCH] CodingGuidelines: ${#parameter} is POSIX and should be allowed\n>\n> See http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n\nI would prefer to play it safe at least for now, especially before 1.7.7\nships.\n\n Documentation/CodingGuidelines | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex fe1c1e5..594fb76 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -52,7 +52,7 @@ For shell scripts specifically (not exhaustive):\n \n - No shell arrays.\n \n- - No strlen ${#parameter}.\n+ - No strlen ${#parameter} (even though it is in POSIX).\n \n - No pattern replacement ${parameter/pattern/string}.\n \n"},{"id":"174947","messageId":"rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil","threadId":"28303","inReplyTo":"7vbouzxy7g.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-09-06T19:09:43Z","receivedAt":"2011-09-06T19:09:43Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\nAdd an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target\nwhich tests whether the shell supports ${parameter%word} expansion. I\nassume this one test is enough to indicate whether the shell supports the\nentire family of prefix and suffix removal syntax:\n\n ${parameter%word}\n ${parameter%%word}\n ${parameter#word}\n ${parameter##word}\n\nFreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but\nnot the above prefix/suffix removal notation.\n---\n\nOn 09/05/2011 02:09 AM, Junio C Hamano wrote:\n> Naohiro Aota writes:\n> \n>> Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\n>> shells like FreeBSD sh and they made the test to fail.\n> \n> Sorry, I do appreciate the effort, but a patch like this takes us in the\n> wrong direction.\n> \n> While we do not allow blatant bashisms like ${parameter:offset:length}\n> (substring expansion), ${parameter/pattern/string} (pattern substitution),\n> \"local\" variables, \"function\" noiseword, and shell arrays in our shell\n> scripts, the two kinds of substitution you quoted above are purely POSIX,\n> and our coding guideline does allow them to be used in the scripts.\n\nPerhaps we should add a test for this shell feature.\n\n-Brandon\n\n Makefile | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 8d6d451..46d9c5d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1738,6 +1738,7 @@ endif\n \n please_set_SHELL_PATH_to_a_more_modern_shell:\n \t@$$(:)\n+\t@foo=bar_suffix && test bar = \"$${foo%_*}\"\n \n shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n \n-- \n1.7.6.1\n"},{"id":"174949","messageId":"2i2CfjMHrXZ7dV7ciebqx3PjO-cpw8QIplKjdcx_bGmGt8jgFr3efDXeMJMcn_I9ZH6X71aBdaO7vGiRBQuhbukGEWFJZQuvWtq079u0KYQ@cipher.nrlssc.navy.mil","threadId":"28303","inReplyTo":"rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2011-09-06T19:32:45Z","receivedAt":"2011-09-06T19:32:45Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"\nFYI:\nIt should be possible to test this patch on a modern system by doing\nsomething like:\n\n make SHELL_PATH=/bin/false\n\nand you should see something like this:\n\n make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1\n\nBut beware, GNU make 3.81 seems to have a bug which sends it into an\ninfinite loop.\n\nmake 3.80 produces the desired results, as does 3.77 which I have\ninstalled on an old machine. GNU make 3.82 seems to be the latest but\nI don't have access to it. If anyone does, I'd appreciate if you\ncould test.\n\n-Brandon\n\n\nOn 09/06/2011 02:09 PM, Brandon Casey wrote:\n> From: Brandon Casey \n> \n> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target\n> which tests whether the shell supports ${parameter%word} expansion. I\n> assume this one test is enough to indicate whether the shell supports the\n> entire family of prefix and suffix removal syntax:\n> \n> ${parameter%word}\n> ${parameter%%word}\n> ${parameter#word}\n> ${parameter##word}\n> \n> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but\n> not the above prefix/suffix removal notation.\n> ---\n> \n> On 09/05/2011 02:09 AM, Junio C Hamano wrote:\n>> Naohiro Aota writes:\n>>\n>>> Variable expansions like \"${foo#bar}\" or \"${foo%bar}\" doesn't work on\n>>> shells like FreeBSD sh and they made the test to fail.\n>>\n>> Sorry, I do appreciate the effort, but a patch like this takes us in the\n>> wrong direction.\n>>\n>> While we do not allow blatant bashisms like ${parameter:offset:length}\n>> (substring expansion), ${parameter/pattern/string} (pattern substitution),\n>> \"local\" variables, \"function\" noiseword, and shell arrays in our shell\n>> scripts, the two kinds of substitution you quoted above are purely POSIX,\n>> and our coding guideline does allow them to be used in the scripts.\n> \n> Perhaps we should add a test for this shell feature.\n> \n> -Brandon\n> \n> Makefile | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 8d6d451..46d9c5d 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1738,6 +1738,7 @@ endif\n> \n> please_set_SHELL_PATH_to_a_more_modern_shell:\n> \t@$$(:)\n> +\t@foo=bar_suffix && test bar = \"$${foo%_*}\"\n> \n> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n> \n"},{"id":"174952","messageId":"7v62l58mp2.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T20:01:29Z","receivedAt":"2011-09-06T20:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> From: Brandon Casey \n>\n> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target\n> which tests whether the shell supports ${parameter%word} expansion. I\n> assume this one test is enough to indicate whether the shell supports the\n> entire family of prefix and suffix removal syntax:\n>\n> ${parameter%word}\n> ${parameter%%word}\n> ${parameter#word}\n> ${parameter##word}\n>\n> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but\n> not the above prefix/suffix removal notation.\n\nMy reading of the later part of the thread you are basing the above is\nsomewhat different from your diagnosis. The funny seems to happen only\nwhen there is a backslash-quoted glob special inside double-quotes\n(e.g. \"${parameter%\\?*}\") and the same shell does not seem to be choking\non many prefix/suffix expansion used in other test scripts.\n"},{"id":"174953","messageId":"7v1uvt8ml6.fsf@alter.siamese.dyndns.org","threadId":"28303","inReplyTo":"rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T20:03:49Z","receivedAt":"2011-09-06T20:03:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> diff --git a/Makefile b/Makefile\n> index 8d6d451..46d9c5d 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1738,6 +1738,7 @@ endif\n> \n> please_set_SHELL_PATH_to_a_more_modern_shell:\n> \t@$$(:)\n> +\t@foo=bar_suffix && test bar = \"$${foo%_*}\"\n> \n> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n\nPerhaps\n\n\t@foo='bar?suffix' && test bar = \"$${foo%\\?*}\"\n\ninstead?\n"},{"id":"174954","messageId":"urATHUDMxTPsK81dgL16m7pg7qY-SQUV-kFZY47c-P0m2twVUC6nD2h-wYpNL6rcveoTNZdxojcfp9X0SeQjMuV97FzxI40RxrDw6pGEAFg@cipher.nrlssc.navy.mil","threadId":"28303","inReplyTo":"7v62l58mp2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-09-06T20:09:55Z","receivedAt":"2011-09-06T20:09:55Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On 09/06/2011 03:01 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> From: Brandon Casey \n>>\n>> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target\n>> which tests whether the shell supports ${parameter%word} expansion. I\n>> assume this one test is enough to indicate whether the shell supports the\n>> entire family of prefix and suffix removal syntax:\n>>\n>> ${parameter%word}\n>> ${parameter%%word}\n>> ${parameter#word}\n>> ${parameter##word}\n>>\n>> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but\n>> not the above prefix/suffix removal notation.\n> \n> My reading of the later part of the thread you are basing the above is\n> somewhat different from your diagnosis. The funny seems to happen only\n> when there is a backslash-quoted glob special inside double-quotes\n> (e.g. \"${parameter%\\?*}\") and the same shell does not seem to be choking\n> on many prefix/suffix expansion used in other test scripts.\n\nAh, I didn't read through closely enough to notice that the above\nsyntax was not also an issue, as was mentioned in the original email.\n\nSorry for the noise.\n\n-Brandon\n"},{"id":"174955","messageId":"P8L-gIYkRp9VP2hZA6C9Q0Ka-vFda8JLDmCkZvI-HruWKjjPYWAg-PmIrbpKE20ORu1qtoL33A2B9_m2ojl0754hk4q4quHgQXx3-S5StEk@cipher.nrlssc.navy.mil","threadId":"28303","inReplyTo":"7v1uvt8ml6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-09-06T20:20:53Z","receivedAt":"2011-09-06T20:20:53Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On 09/06/2011 03:03 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> diff --git a/Makefile b/Makefile\n>> index 8d6d451..46d9c5d 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -1738,6 +1738,7 @@ endif\n>> \n>> please_set_SHELL_PATH_to_a_more_modern_shell:\n>> \t@$$(:)\n>> +\t@foo=bar_suffix && test bar = \"$${foo%_*}\"\n>> \n>> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell\n> \n> Perhaps\n> \n> \t@foo='bar?suffix' && test bar = \"$${foo%\\?*}\"\n> \n> instead?\n\nLooks right.\n\nNaohiro, can you test? Or someone else with FreeBSD?\n\nmake should produce an error message like this:\n\n gmake: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1\n\n-Brandon\n"},{"id":"174958","messageId":"4KpnoijSRGBLoF4pZj7c1eShQRupu7h-gkSjM2Ej6nnefH-n7qWuAkoY4CEocEsJb6XCaqhrtHT3uQL2W3DKu0yJ1rAh-UxeXocbOTvMhBw@cipher.nrlssc.navy.mil","threadId":"28303","inReplyTo":"2i2CfjMHrXZ7dV7ciebqx3PjO-cpw8QIplKjdcx_bGmGt8jgFr3efDXeMJMcn_I9ZH6X71aBdaO7vGiRBQuhbukGEWFJZQuvWtq079u0KYQ@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2011-09-06T20:30:33Z","receivedAt":"2011-09-06T20:30:33Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"On 09/06/2011 02:32 PM, Brandon Casey wrote:\n> \n> FYI:\n> It should be possible to test this patch on a modern system by doing\n> something like:\n> \n> make SHELL_PATH=/bin/false\n> \n> and you should see something like this:\n> \n> make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1\n> \n> But beware, GNU make 3.81 seems to have a bug which sends it into an\n> infinite loop.\n\nJust a clarification, I didn't mean you'd actually be able to test\nthe patch for correctness, but the above would at least allow you to\nstress the code path.\n\nBut, with the Makefile in its current form (patch or no patch) the\nabove still works. Setting SHELL_PATH=/bin/false produces the desired\nerror message.\n\nThere still appears to be a bug in make 3.81 which is triggered when\nusing an ancient shell, it just manifests itself in a different way\nusing our current Makefile. Right now, make 3.81 will enter an\ninfinite loop when it tries to include the GIT-VERSION-FILE. When\nsomething like /bin/sh on Solaris processes the GIT-VERSION-GEN\nscript, it produces the following incorrect string in the\nGIT-VERSION-FILE:\n\n GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\\(.*\\))\n\nwhich then becomes part of the Makefile when GIT-VERSION-FILE is\nincluded on line 264. GNU make then begins to print the following\nto the terminal repeatedly:\n\n GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\\(.*\\))\n\nGIT-VERSION-FILE should really have a dependency on\nshell_compatibility_test since it calls GIT-VERSION-GEN which may use\nshell features that are not provided by the configured shell. If that\ndependency is added so that the GIT-VERSION-FILE rule looks like this:\n\n GIT-VERSION-FILE: shell_compatibility_test FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n -include GIT-VERSION-FILE\n\n_then_, we get the behavior I described originally, where\n\n make SHELL_PATH=/bin/false\n\nsends the make process into an infinite loop, with no output to the\nterminal.\n\nEither way, with GNU make 3.81, you get an infinite loop when you use\na shell that should trigger our error message.\n\n-Brandon\n"}]}