{"thread":{"id":"36737","subject":"[PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","startedAt":"2014-05-23T10:15:31Z","lastAt":"2014-05-23T21:56:26Z","messageCount":10,"participants":["Elia Pinto","Jonathan Nieder","Junio C Hamano","Eric Sunshine","Torsten Bögershausen"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"242561","messageId":"1400840131-966-1-git-send-email-gitter.spiros@gmail.com","threadId":"36737","inReplyTo":null,"subject":"[PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2014-05-23T10:15:31Z","receivedAt":"2014-05-23T10:15:31Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Found by check-non-portable-shell.pl\n\nSigned-off-by: Elia Pinto \n---\nThis is the second version of the patch that includes \nJunio suggestions.\n\n contrib/subtree/t/t7900-subtree.sh | 3 ++-\n git-remote-testgit.sh | 3 ++-\n git-stash.sh | 3 ++-\n 3 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 66ce4b0..8dc6840 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -8,7 +8,8 @@ This test verifies the basic operation of the merge, pull, add\n and split subcommands of git subtree.\n '\n \n-export TEST_DIRECTORY=$(pwd)/../../../t\n+TEST_DIRECTORY=$(pwd)/../../../t\n+export TEST_DIRECTORY\n \n . ../../../t/test-lib.sh\n \ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex 1c006a0..a9c75a2 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -13,7 +13,8 @@ refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n \n test -z \"$refspec\" && prefix=\"refs\"\n \n-export GIT_DIR=\"$url/.git\"\n+GIT_DIR=\"$url/.git\"\n+export GIT_DIR\n \n force=\n \ndiff --git a/git-stash.sh b/git-stash.sh\nindex 4798bcf..4621d81 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -94,7 +94,8 @@ create_stash () {\n \t\t# ease of unpacking later.\n \t\tu_commit=$(\n \t\t\tuntracked_files | (\n-\t\t\t\texport GIT_INDEX_FILE=\"$TMPindex\"\n+\t\t\t\tGIT_INDEX_FILE=\"$TMPindex\" &&\n+\t\t\t\texport GIT_INDEX_FILE &&\n \t\t\t\trm -f \"$TMPindex\" &&\n \t\t\t\tgit update-index -z --add --remove --stdin &&\n \t\t\t\tu_tree=$(git write-tree) &&\n-- \n1.7.10.4\n"},{"id":"242576","messageId":"20140523161823.GB4065@google.com","threadId":"36737","inReplyTo":"1400840131-966-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-23T16:18:23Z","receivedAt":"2014-05-23T16:18:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Elia Pinto wrote:\n\n> Signed-off-by: Elia Pinto \n> ---\n> contrib/subtree/t/t7900-subtree.sh | 3 ++-\n> git-remote-testgit.sh | 3 ++-\n> git-stash.sh | 3 ++-\n> 3 files changed, 6 insertions(+), 3 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"242587","messageId":"xmqqmwe8jr44.fsf@gitster.dls.corp.google.com","threadId":"36737","inReplyTo":"1400840131-966-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T18:18:35Z","receivedAt":"2014-05-23T18:18:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> Found by check-non-portable-shell.pl\n\nThanks.\n\nMakes me wonder why these two were missed, though.\n\n\n t/t3032-merge-recursive-options.sh | 6 +++++-\n t/t5560-http-backend-noserver.sh | 6 +++++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\nindex 5fd7bbb..4029c9c 100755\n--- a/t/t3032-merge-recursive-options.sh\n+++ b/t/t3032-merge-recursive-options.sh\n@@ -14,7 +14,11 @@ test_description='merge-recursive options\n . ./test-lib.sh\n \n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n-test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U\n+if test_have_prereq GREP_STRIPS_CR\n+then\n+\tGREP_OPTIONS=-U\n+\texport GREP_OPTIONS\n+fi\n \n test_expect_success 'setup' '\n \tconflict_hunks () {\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9be9ae3..1e25128 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -5,7 +5,11 @@ test_description='test git-http-backend-noserver'\n \n HTTPD_DOCUMENT_ROOT_PATH=\"$TRASH_DIRECTORY\"\n \n-test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U\n+if test_have_prereq GREP_STRIPS_CR\n+then\n+\tGREP_OPTIONS=-U\n+\texport GREP_OPTIONS\n+fi\n \n run_backend() {\n \techo \"$2\" |\n"},{"id":"242590","messageId":"xmqqegzkjq5e.fsf@gitster.dls.corp.google.com","threadId":"36737","inReplyTo":"xmqqmwe8jr44.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T18:39:25Z","receivedAt":"2014-05-23T18:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Elia Pinto writes:\n>\n>> Found by check-non-portable-shell.pl\n>\n> Thanks.\n>\n> Makes me wonder why these two were missed, though.\n\nPerhaps something like this?\n\nI didn't check other rules, though, because I still have a feeling\nthat this \"pretend to understand the shell syntax and point out\nissues, without really parsing the script\" is fundamentally an\nerror-prone approach and am hesitant to loosen the patterns too\nmuch.\n\n t/check-non-portable-shell.pl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex 45971f4..24d7555 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -21,7 +21,7 @@ while (<>) {\n \t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n \t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n \t/test\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n-\t/^\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n+\t/(?:^|[^-a-zA-Z0-9_])export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n }\n"},{"id":"242592","messageId":"20140523184405.GE12314@google.com","threadId":"36737","inReplyTo":"xmqqmwe8jr44.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-23T18:44:05Z","receivedAt":"2014-05-23T18:44:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Elia Pinto writes:\n\n>> Found by check-non-portable-shell.pl\n>\n> Thanks.\n>\n> Makes me wonder why these two were missed, though.\n\nGood catch. check-non-portable-shell.pl uses an anchored regex:\n\n\t/^\\s*export\\s+[^=]*=/\n\nPerhaps something like\n\n\t/\\bexport\\s+[A-Za-z0-9_]*=/\n\nwithout anchoring would work better.\n\n-- >8 --\nSubject: test-lint: find unportable sed, echo, test, and export usage after &&\n\nInstead of anchoring these checks with \"^\\s*\", just check that the\nusage is preceded by a word boundary. So now we can catch\n\n\ttest $cond && export foo=bar\n\njust like we already catch\n\n\ttest $cond &&\n\texport foo=bar\n\nAs a side effect, this will detect usage of \"sed -i\", \"echo -n\", \"test\na == b\", and \"export a=b\" in comments. That is not ideal but it's\npotentially useful because people sometimes copy code from comments so\nit can be good to also avoid nonportable patterns there.\n\nTo avoid false positives, keep the checks for 'declare' and 'which'\nanchored. Those are frequently used words in normal English-language\ncomments.\n\nSigned-off-by: Jonathan Nieder \n---\n t/check-non-portable-shell.pl | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git i/t/check-non-portable-shell.pl w/t/check-non-portable-shell.pl\nindex 45971f4..b170cbc 100755\n--- i/t/check-non-portable-shell.pl\n+++ w/t/check-non-portable-shell.pl\n@@ -16,12 +16,12 @@ sub err {\n \n while (<>) {\n \tchomp;\n-\t/^\\s*sed\\s+-i/ and err 'sed -i is not portable';\n-\t/^\\s*echo\\s+-n/ and err 'echo -n is not portable (please use printf)';\n+\t/\\bsed\\s+-i/ and err 'sed -i is not portable';\n+\t/\\becho\\s+-n/ and err 'echo -n is not portable (please use printf)';\n \t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n \t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n-\t/test\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n-\t/^\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n+\t/\\btest\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n+\t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n }\n"},{"id":"242593","messageId":"20140523184858.GF12314@google.com","threadId":"36737","inReplyTo":"xmqqegzkjq5e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-23T18:48:59Z","receivedAt":"2014-05-23T18:48:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Junio C Hamano writes:\n\n>> Makes me wonder why these two were missed, though.\n>\n> Perhaps something like this?\n[...]\n> -\t/^\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n> +\t/(?:^|[^-a-zA-Z0-9_])export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n\nMessages crossed.\n\nI think I like \"\\b\" better for this, while I also share your lack of\nenthusiasm for parsing shell with something that is not a shell.\n\nMaybe it's worth filing bugs against \"posh -n\" to make it\n(optionally?) notice things we want to detect with a goal of\neventually moving to that.\n\nThanks,\nJonathan\n"},{"id":"242609","messageId":"CAPig+cTt-+k3ufUX=MSo1z1tdU3+BRjJ7hO7iQ8bdhfJ4b-J3Q@mail.gmail.com","threadId":"36737","inReplyTo":"1400840131-966-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-23T20:38:57Z","receivedAt":"2014-05-23T20:38:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 23, 2014 at 6:15 AM, Elia Pinto wrote:\n> Subject: Get rid of the non portable shell export VAR=VALUE costruct\n\ns/costruct/construct/\ns/non portable/non-portable/\n\n> Found by check-non-portable-shell.pl\n>\n> Signed-off-by: Elia Pinto \n> ---\n> This is the second version of the patch that includes\n> Junio suggestions.\n>\n> contrib/subtree/t/t7900-subtree.sh | 3 ++-\n> git-remote-testgit.sh | 3 ++-\n> git-stash.sh | 3 ++-\n> 3 files changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 66ce4b0..8dc6840 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -8,7 +8,8 @@ This test verifies the basic operation of the merge, pull, add\n> and split subcommands of git subtree.\n> '\n>\n> -export TEST_DIRECTORY=$(pwd)/../../../t\n> +TEST_DIRECTORY=$(pwd)/../../../t\n> +export TEST_DIRECTORY\n>\n> . ../../../t/test-lib.sh\n>\n> diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\n> index 1c006a0..a9c75a2 100755\n> --- a/git-remote-testgit.sh\n> +++ b/git-remote-testgit.sh\n> @@ -13,7 +13,8 @@ refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n>\n> test -z \"$refspec\" && prefix=\"refs\"\n>\n> -export GIT_DIR=\"$url/.git\"\n> +GIT_DIR=\"$url/.git\"\n> +export GIT_DIR\n>\n> force=\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 4798bcf..4621d81 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -94,7 +94,8 @@ create_stash () {\n> # ease of unpacking later.\n> u_commit=$(\n> untracked_files | (\n> - export GIT_INDEX_FILE=\"$TMPindex\"\n> + GIT_INDEX_FILE=\"$TMPindex\" &&\n> + export GIT_INDEX_FILE &&\n> rm -f \"$TMPindex\" &&\n> git update-index -z --add --remove --stdin &&\n> u_tree=$(git write-tree) &&\n> --\n"},{"id":"242610","messageId":"537FB249.8050708@web.de","threadId":"36737","inReplyTo":"20140523184405.GE12314@google.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-05-23T20:40:41Z","receivedAt":"2014-05-23T20:40:41Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-05-23 20.44, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>> Elia Pinto writes:\n> \n>>> Found by check-non-portable-shell.pl\n>>\n>> Thanks.\n>>\n>> Makes me wonder why these two were missed, though.\n> \n> Good catch. check-non-portable-shell.pl uses an anchored regex:\n> \n> \t/^\\s*export\\s+[^=]*=/\n> \n> Perhaps something like\n> \n> \t/\\bexport\\s+[A-Za-z0-9_]*=/\n> \n> without anchoring would work better.\n> \n> -- >8 --\n> Subject: test-lint: find unportable sed, echo, test, and export usage after &&\n> \n> Instead of anchoring these checks with \"^\\s*\", just check that the\n> usage is preceded by a word boundary. So now we can catch\n> \n> \ttest $cond && export foo=bar\nThanks for digging.\n\nI wonder if we could keep the anchoring (to reduce false positives)\nand try to catch a line \"command1 && command2\" at the same time:\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex 45971f4..f64e054 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -16,12 +16,12 @@ sub err {\n \n while (<>) {\n chomp;\n- /^\\s*sed\\s+-i/ and err 'sed -i is not portable';\n- /^\\s*echo\\s+-n/ and err 'echo -n is not portable (please use printf)';\n- /^\\s*declare\\s+/ and err 'arrays/declare not portable';\n- /^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n+ /(^|&&)\\s*sed\\s+-i/ and err 'sed -i is not portable';\n+ /(^|&&)\\s*echo\\s+-n/ and err 'echo -n is not portable (please use printf)';\n+ /(^|&&)\\s*declare\\s+/ and err 'arrays/declare not portable';\n+ /(^|&&)\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n /test\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n- /^\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n+ /(^|&&)\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n # this resets our $. for each file\n close ARGV if eof;\n }\n"},{"id":"242615","messageId":"xmqq8upsi3ww.fsf@gitster.dls.corp.google.com","threadId":"36737","inReplyTo":"537FB249.8050708@web.de","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T21:25:03Z","receivedAt":"2014-05-23T21:25:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> I wonder if we could keep the anchoring (to reduce false positives)\n> and try to catch a line \"command1 && command2\" at the same time:\n\nWell, \"command1 || command2\" would be the next thing you want to\ncatch, and I think you are at the entrance of a slipperly slope at\nthat point.\n"},{"id":"242618","messageId":"xmqqtx8ggnw5.fsf@gitster.dls.corp.google.com","threadId":"36737","inReplyTo":"20140523184405.GE12314@google.com","subject":"Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-23T21:56:26Z","receivedAt":"2014-05-23T21:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> -\t/^\\s*export\\s+[^=]*=/ and err '\"export FOO=bar\" is not por...\n> +\t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is n...\n\nI see you also tightened what the \"variable\" to be assigned should\nlook like, which is a good change to avoid false positives.\n\nBecause the inter-word dash is taken as a word \"\\b\"oundary, that\nchange incidentally prevents a false positive from triggering on\nsomething like:\n\n\tgit fast-export --option=value\n\n;-)\n"}]}