threads / patch / 48875

patch, 4 partstest-lint: detect one-shot "FOO=bar shell_func"

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

## tl;dr

5 messages between Jul 13, 2018 and Jul 13, 2018. Diffs are folded; open one to read it.

replies: 4people: 1as markdown or json

Eric Sunshine· Jul 13, 2018, 05:52 UTC · lore

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
Eric Sunshine· Jul 13, 2018, 05:52 UTC · re: Eric Sunshine · lore

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

t/t6046-merge-skip-unneeded-updates.sh, t/t9833-errors.sh

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
Eric Sunshine· Jul 13, 2018, 05:52 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to t/check-non-portable-shell.pl +5 −5
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
Eric Sunshine· Jul 13, 2018, 05:52 UTC · re: Eric Sunshine · lore

[PATCH 3/4] 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:

    <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(+)
Show changes to t/check-non-portable-shell.pl +2 −0
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
Eric Sunshine· Jul 13, 2018, 05:52 UTC · re: Eric Sunshine · lore

[PATCH 4/4] 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 <sunshine@sunshineco.com>
---
 t/check-non-portable-shell.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Show changes to t/check-non-portable-shell.pl +19 −0
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

← back to recent threads