From 079b087c8eaf8119c4b159598e7b6965c1ca3fe9 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Fri, 13 Jul 2018 01:52:02 -0400 Subject: t6046/t9833: fix use of "VAR=VAL cmd" with a shell function Unlike "FOO=bar cmd" one-shot environment variable assignments which exist only for the invocation of 'cmd', those assigned by "FOO=bar shell_func" exist within the running shell and continue to do so until the process exits (or are explicitly unset). It is unlikely that this behavior was intended by the test author. In these particular tests, the "FOO=bar shell_func" invocations are already in subshells, so the assignments don't last too long, don't appear to harm subsequent commands in the same subshells, and don't affect other tests in the same scripts, however, the usage is nevertheless misleading and poor practice, so fix the tests to assign and export the environment variables in the usual fashion. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/t6046-merge-skip-unneeded-updates.sh | 4 +++- t/t9833-errors.sh | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 't') diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh index fcefffcaec..38e24f787c 100755 --- a/t/t6046-merge-skip-unneeded-updates.sh +++ b/t/t6046-merge-skip-unneeded-updates.sh @@ -366,7 +366,9 @@ test_expect_success '2c-check: Modify b & add c VS rename b->c' ' git checkout A^0 && - GIT_MERGE_VERBOSITY=3 test_must_fail git merge -s recursive B^0 >out 2>err && + GIT_MERGE_VERBOSITY=3 && + export GIT_MERGE_VERBOSITY && + test_must_fail git merge -s recursive B^0 >out 2>err && test_i18ngrep "CONFLICT (rename/add): Rename b->c" out && test_i18ngrep ! "Skipped c" out && diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh index 9ba892de7a..277d347012 100755 --- a/t/t9833-errors.sh +++ b/t/t9833-errors.sh @@ -26,7 +26,9 @@ test_expect_success 'error handling' ' ) && p4 passwd -P newpassword && ( - P4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 2>errmsg && + P4PASSWD=badpassword && + export P4PASSWD && + test_must_fail git p4 clone //depot/foo 2>errmsg && grep -q "failure accessing depot.*P4PASSWD" errmsg ) ' -- cgit v1.2.3 From ef2d2acceffaa426e27ad21350fb707ddb5f5eec Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Fri, 13 Jul 2018 01:52:03 -0400 Subject: t/check-non-portable-shell: stop being so polite Error messages emitted by this linting script are long and noisy, consisting of several sections: :: error: : Many problem explanations ask the reader to "please" use a suggested alternative, however, such politeness is unnecessary and just adds to the noise and length of the line, so drop "please" to make the message a bit more concise. Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 't') diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index e07f028437..11028578ff 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -17,12 +17,12 @@ sub err { while (<>) { chomp; /\bsed\s+-i/ and err 'sed -i is not portable'; - /\becho\s+-[neE]/ and err 'echo with option is not portable (please use printf)'; + /\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)'; /^\s*declare\s+/ and err 'arrays/declare not portable'; - /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)'; - /\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use =)'; - /\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (please use test_line_count)'; - /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)'; + /^\s*[^#]\s*which\s/ and err 'which is not portable (use type)'; + /\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)'; + /\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)'; + /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; # this resets our $. for each file close ARGV if eof; } -- cgit v1.2.3 From c433600593b1db15a9ba80136dc2096e192322b6 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Fri, 13 Jul 2018 01:52:04 -0400 Subject: t/check-non-portable-shell: make error messages more compact Error messages emitted by this linting script are long and noisy, consisting of several sections: :: error: : The line of failed shell text, usually coming from within a test body, is often indented by one or two TABs, with the result that the actual (important) text is separated from by a good deal of empty space. This can make for a difficult read, especially on typical 80-column terminals. Make the messages more compact and perhaps a bit easier to digest by folding out the leading whitespace from . Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 2 ++ 1 file changed, 2 insertions(+) (limited to 't') diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index 11028578ff..f6dbe28b19 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -10,6 +10,8 @@ my $exit_code=0; sub err { my $msg = shift; + s/^\s+//; + s/\s+$//; print "$ARGV:$.: error: $msg: $_\n"; $exit_code = 1; } -- cgit v1.2.3 From a0a630192dca77794b2ea94aeb2e6dff0cc1810b Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Fri, 13 Jul 2018 01:52:05 -0400 Subject: t/check-non-portable-shell: detect "FOO=bar shell_func" One-shot environment variable assignments, such as 'FOO' in "FOO=bar cmd", exist only during the invocation of 'cmd'. However, if 'cmd' happens to be a shell function, then 'FOO' is assigned in the executing shell itself, and that assignment remains until the process exits (unless explicitly unset). Since this side-effect of "FOO=bar shell_func" is unlikely to be intentional, detect and report such usage. To distinguish shell functions from other commands, perform a pre-scan of shell scripts named as input, gleaning a list of function names by recognizing lines of the form (loosely matching whitespace): shell_func () { and later report suspect lines of the form (loosely matching quoted values): FOO=bar [BAR=foo ...] shell_func Also take care to stitch together incomplete lines (those ending with "\") since suspect invocations may be split over multiple lines: FOO=bar BAR=foo \ shell_func Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano --- t/check-non-portable-shell.pl | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 't') diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index f6dbe28b19..d5823f71d8 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -7,17 +7,34 @@ use strict; use warnings; my $exit_code=0; +my %func; sub err { my $msg = shift; s/^\s+//; s/\s+$//; + s/\s+/ /g; print "$ARGV:$.: error: $msg: $_\n"; $exit_code = 1; } +# glean names of shell functions +for my $i (@ARGV) { + open(my $f, '<', $i) or die "$0: $i: $!\n"; + while (<$f>) { + $func{$1} = 1 if /^\s*(\w+)\s*\(\)\s*{\s*$/; + } + close $f; +} + while (<>) { chomp; + # stitch together incomplete lines (those ending with "\") + while (s/\\$//) { + $_ .= readline; + chomp; + } + /\bsed\s+-i/ and err 'sed -i is not portable'; /\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)'; /^\s*declare\s+/ and err 'arrays/declare not portable'; @@ -25,6 +42,8 @@ while (<>) { /\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)'; /\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)'; /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; + /^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and + err '"FOO=bar shell_func" assignment extends beyond "shell_func"'; # this resets our $. for each file close ARGV if eof; } -- cgit v1.2.3