[PATCH 0/4] test-lint: detect one-shot "FOO=bar shell_func"

[PATCH 0/4] test-lint: detect one-shot "FOO=bar shell_func"

[Eric Sunshine]

One-shot "FOO=bar cmd" environment variable assignments exist only
during invocation of 'cmd'. However, if 'cmd' is a shell function, then
the variable is assigned in the running shell and exists until the
process exits (or is unset explicitly). Such a side-effect is almost
certainly unintended by a test author and is likely due to lack of
familiarity with the problem.

Upgrade "make test-lint" to detect this sort of suspect usage.

Also fix a couple instances of "FOO=bar shell_func" detected by the
improved linting.

This series is built atop 'jc/t3404-one-shot-export-fix'[1].

[1]: https://public-inbox.org/git/xmqqefg8w73c.fsf@gitster-ct.c.googlers.com/T/

Eric Sunshine (4):
  t6046/t9833: fix use of "VAR=VAL cmd" with a shell function
  t/check-non-portable-shell: stop being so polite
  t/check-non-portable-shell: make error messages more compact
  t/check-non-portable-shell: detect "FOO=bar shell_func"

 t/check-non-portable-shell.pl          | 31 +++++++++++++++++++++-----
 t/t6046-merge-skip-unneeded-updates.sh |  4 +++-
 t/t9833-errors.sh                      |  4 +++-
 3 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.18.0.233.g985f88cf7e

[PATCH 1/4] t6046/t9833: fix use of "VAR=VAL cmd" with a shell function

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---
 t/t6046-merge-skip-unneeded-updates.sh | 4 +++-
 t/t9833-errors.sh                      | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

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
 	)
 '
-- 
2.18.0.233.g985f88cf7e

[PATCH 2/4] t/check-non-portable-shell: stop being so polite

[Eric Sunshine]

Error messages emitted by this linting script are long and noisy,
consisting of several sections:

    <test-script>:<line#>: error: <explanation>: <failed-shell-text>

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 <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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;
 }
-- 
2.18.0.233.g985f88cf7e

[PATCH 3/4] t/check-non-portable-shell: make error messages more compact

[Eric Sunshine]

Error messages emitted by this linting script are long and noisy,
consisting of several sections:

    <test-script>:<line#>: error: <explanation>: <failed-shell-text>

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 <explanation> 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 <failed-shell-text>.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 2 ++
 1 file changed, 2 insertions(+)

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 @@
 
 sub err {
 	my $msg = shift;
+	s/^\s+//;
+	s/\s+$//;
 	print "$ARGV:$.: error: $msg: $_\n";
 	$exit_code = 1;
 }
-- 
2.18.0.233.g985f88cf7e

[PATCH 4/4] t/check-non-portable-shell: detect "FOO=bar shell_func"

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

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 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 @@ sub err {
 	/\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;
 }
-- 
2.18.0.233.g985f88cf7e