{"thread":{"id":"48875","subject":"[PATCH 0/4] test-lint: detect one-shot \"FOO=bar shell_func\"","startedAt":"2018-07-13T05:53:00Z","lastAt":"2018-07-13T05:53:03Z","messageCount":5,"participants":["Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"352447","messageId":"20180713055205.32351-1-sunshine@sunshineco.com","threadId":"48875","inReplyTo":null,"subject":"[PATCH 0/4] test-lint: detect one-shot \"FOO=bar shell_func\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:52:01Z","receivedAt":"2018-07-13T05:53:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"One-shot \"FOO=bar cmd\" environment variable assignments exist only\nduring invocation of 'cmd'. However, if 'cmd' is a shell function, then\nthe variable is assigned in the running shell and exists until the\nprocess exits (or is unset explicitly). Such a side-effect is almost\ncertainly unintended by a test author and is likely due to lack of\nfamiliarity with the problem.\n\nUpgrade \"make test-lint\" to detect this sort of suspect usage.\n\nAlso fix a couple instances of \"FOO=bar shell_func\" detected by the\nimproved linting.\n\nThis series is built atop 'jc/t3404-one-shot-export-fix'[1].\n\n[1]: https://public-inbox.org/git/xmqqefg8w73c.fsf@gitster-ct.c.googlers.com/T/\n\nEric Sunshine (4):\n t6046/t9833: fix use of \"VAR=VAL cmd\" with a shell function\n t/check-non-portable-shell: stop being so polite\n t/check-non-portable-shell: make error messages more compact\n t/check-non-portable-shell: detect \"FOO=bar shell_func\"\n\n t/check-non-portable-shell.pl | 31 +++++++++++++++++++++-----\n t/t6046-merge-skip-unneeded-updates.sh | 4 +++-\n t/t9833-errors.sh | 4 +++-\n 3 files changed, 32 insertions(+), 7 deletions(-)\n\n-- \n2.18.0.233.g985f88cf7e\n"},{"id":"352448","messageId":"20180713055205.32351-2-sunshine@sunshineco.com","threadId":"48875","inReplyTo":"20180713055205.32351-1-sunshine@sunshineco.com","subject":"[PATCH 1/4] t6046/t9833: fix use of \"VAR=VAL cmd\" with a shell function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:52:02Z","receivedAt":"2018-07-13T05:53:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Unlike \"FOO=bar cmd\" one-shot environment variable assignments\nwhich exist only for the invocation of 'cmd', those assigned by\n\"FOO=bar shell_func\" exist within the running shell and continue to\ndo so until the process exits (or are explicitly unset). It is\nunlikely that this behavior was intended by the test author.\n\nIn these particular tests, the \"FOO=bar shell_func\" invocations are\nalready in subshells, so the assignments don't last too long, don't\nappear to harm subsequent commands in the same subshells, and don't\naffect other tests in the same scripts, however, the usage is\nnevertheless misleading and poor practice, so fix the tests to assign\nand export the environment variables in the usual fashion.\n\nSigned-off-by: Eric Sunshine \n---\n t/t6046-merge-skip-unneeded-updates.sh | 4 +++-\n t/t9833-errors.sh | 4 +++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh\nindex fcefffcaec..38e24f787c 100755\n--- a/t/t6046-merge-skip-unneeded-updates.sh\n+++ b/t/t6046-merge-skip-unneeded-updates.sh\n@@ -366,7 +366,9 @@ test_expect_success '2c-check: Modify b & add c VS rename b->c' '\n \n \t\tgit checkout A^0 &&\n \n-\t\tGIT_MERGE_VERBOSITY=3 test_must_fail git merge -s recursive B^0 >out 2>err &&\n+\t\tGIT_MERGE_VERBOSITY=3 &&\n+\t\texport GIT_MERGE_VERBOSITY &&\n+\t\ttest_must_fail git merge -s recursive B^0 >out 2>err &&\n \n \t\ttest_i18ngrep \"CONFLICT (rename/add): Rename b->c\" out &&\n \t\ttest_i18ngrep ! \"Skipped c\" out &&\ndiff --git a/t/t9833-errors.sh b/t/t9833-errors.sh\nindex 9ba892de7a..277d347012 100755\n--- a/t/t9833-errors.sh\n+++ b/t/t9833-errors.sh\n@@ -26,7 +26,9 @@ test_expect_success 'error handling' '\n \t) &&\n \tp4 passwd -P newpassword &&\n \t(\n-\t\tP4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 2>errmsg &&\n+\t\tP4PASSWD=badpassword &&\n+\t\texport P4PASSWD &&\n+\t\ttest_must_fail git p4 clone //depot/foo 2>errmsg &&\n \t\tgrep -q \"failure accessing depot.*P4PASSWD\" errmsg\n \t)\n '\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352449","messageId":"20180713055205.32351-3-sunshine@sunshineco.com","threadId":"48875","inReplyTo":"20180713055205.32351-1-sunshine@sunshineco.com","subject":"[PATCH 2/4] t/check-non-portable-shell: stop being so polite","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:52:03Z","receivedAt":"2018-07-13T05:53:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Error messages emitted by this linting script are long and noisy,\nconsisting of several sections:\n\n :: error: : \n\nMany problem explanations ask the reader to \"please\" use a suggested\nalternative, however, such politeness is unnecessary and just adds to\nthe noise and length of the line, so drop \"please\" to make the message a\nbit more concise.\n\nSigned-off-by: Eric Sunshine \n---\n t/check-non-portable-shell.pl | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex e07f028437..11028578ff 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -17,12 +17,12 @@ sub err {\n while (<>) {\n \tchomp;\n \t/\\bsed\\s+-i/ and err 'sed -i is not portable';\n-\t/\\becho\\s+-[neE]/ and err 'echo with option is not portable (please use printf)';\n+\t/\\becho\\s+-[neE]/ and err 'echo with option is not portable (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/\\btest\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n-\t/\\bwc -l.*\"\\s*=/ and err '`\"$(wc -l)\"` is not portable (please use test_line_count)';\n-\t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n+\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (use type)';\n+\t/\\btest\\s+[^=]*==/ and err '\"test a == b\" is not portable (use =)';\n+\t/\\bwc -l.*\"\\s*=/ and err '`\"$(wc -l)\"` is not portable (use test_line_count)';\n+\t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n }\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352450","messageId":"20180713055205.32351-4-sunshine@sunshineco.com","threadId":"48875","inReplyTo":"20180713055205.32351-1-sunshine@sunshineco.com","subject":"[PATCH 3/4] t/check-non-portable-shell: make error messages more compact","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:52:04Z","receivedAt":"2018-07-13T05:53:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Error messages emitted by this linting script are long and noisy,\nconsisting of several sections:\n\n :: error: : \n\nThe line of failed shell text, usually coming from within a test body,\nis often indented by one or two TABs, with the result that the actual\n(important) text is separated from by a good deal of empty\nspace. This can make for a difficult read, especially on typical\n80-column terminals.\n\nMake the messages more compact and perhaps a bit easier to digest by\nfolding out the leading whitespace from .\n\nSigned-off-by: Eric Sunshine \n---\n t/check-non-portable-shell.pl | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex 11028578ff..f6dbe28b19 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -10,6 +10,8 @@\n \n sub err {\n \tmy $msg = shift;\n+\ts/^\\s+//;\n+\ts/\\s+$//;\n \tprint \"$ARGV:$.: error: $msg: $_\\n\";\n \t$exit_code = 1;\n }\n-- \n2.18.0.233.g985f88cf7e\n\n"},{"id":"352451","messageId":"20180713055205.32351-5-sunshine@sunshineco.com","threadId":"48875","inReplyTo":"20180713055205.32351-1-sunshine@sunshineco.com","subject":"[PATCH 4/4] t/check-non-portable-shell: detect \"FOO=bar shell_func\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-13T05:52:05Z","receivedAt":"2018-07-13T05:53:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"One-shot environment variable assignments, such as 'FOO' in\n\"FOO=bar cmd\", exist only during the invocation of 'cmd'. However, if\n'cmd' happens to be a shell function, then 'FOO' is assigned in the\nexecuting shell itself, and that assignment remains until the process\nexits (unless explicitly unset). Since this side-effect of\n\"FOO=bar shell_func\" is unlikely to be intentional, detect and report\nsuch usage.\n\nTo distinguish shell functions from other commands, perform a pre-scan\nof shell scripts named as input, gleaning a list of function names by\nrecognizing lines of the form (loosely matching whitespace):\n\n shell_func () {\n\nand later report suspect lines of the form (loosely matching quoted\nvalues):\n\n FOO=bar [BAR=foo ...] shell_func\n\nAlso take care to stitch together incomplete lines (those ending with\n\"\\\") since suspect invocations may be split over multiple lines:\n\n FOO=bar BAR=foo \\\n shell_func\n\nSigned-off-by: Eric Sunshine \n---\n t/check-non-portable-shell.pl | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex f6dbe28b19..d5823f71d8 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -7,17 +7,34 @@\n use warnings;\n \n my $exit_code=0;\n+my %func;\n \n sub err {\n \tmy $msg = shift;\n \ts/^\\s+//;\n \ts/\\s+$//;\n+\ts/\\s+/ /g;\n \tprint \"$ARGV:$.: error: $msg: $_\\n\";\n \t$exit_code = 1;\n }\n \n+# glean names of shell functions\n+for my $i (@ARGV) {\n+\topen(my $f, '<', $i) or die \"$0: $i: $!\\n\";\n+\twhile (<$f>) {\n+\t\t$func{$1} = 1 if /^\\s*(\\w+)\\s*\\(\\)\\s*{\\s*$/;\n+\t}\n+\tclose $f;\n+}\n+\n while (<>) {\n \tchomp;\n+\t# stitch together incomplete lines (those ending with \"\\\")\n+\twhile (s/\\\\$//) {\n+\t\t$_ .= readline;\n+\t\tchomp;\n+\t}\n+\n \t/\\bsed\\s+-i/ and err 'sed -i is not portable';\n \t/\\becho\\s+-[neE]/ and err 'echo with option is not portable (use printf)';\n \t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n@@ -25,6 +42,8 @@ sub err {\n \t/\\btest\\s+[^=]*==/ and err '\"test a == b\" is not portable (use =)';\n \t/\\bwc -l.*\"\\s*=/ and err '`\"$(wc -l)\"` is not portable (use test_line_count)';\n \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (use FOO=bar && export FOO)';\n+\t/^\\s*([A-Z0-9_]+=(\\w+|([\"']).*?\\3)\\s+)+(\\w+)/ and exists($func{$4}) and\n+\t\terr '\"FOO=bar shell_func\" assignment extends beyond \"shell_func\"';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n }\n-- \n2.18.0.233.g985f88cf7e\n\n"}]}