threads / patch / 36737

v2Get rid of the non portable shell export VAR=VALUE costruct

Subject: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

## tl;dr

10 messages between May 23, 2014 and May 23, 2014. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Elia Pinto· May 23, 2014, 10:15 UTC · lore
Found by check-non-portable-shell.pl
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
This is the second version of the patch that includes 
Junio suggestions.
 contrib/subtree/t/t7900-subtree.sh |    3 ++-
 git-remote-testgit.sh              |    3 ++-
 git-stash.sh                       |    3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)
Show changes to 3 files +6 −3

contrib/subtree/t/t7900-subtree.sh, git-remote-testgit.sh, git-stash.sh

diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 66ce4b0..8dc6840 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -8,7 +8,8 @@ This test verifies the basic operation of the merge, pull, add
 and split subcommands of git subtree.
 '
 
-export TEST_DIRECTORY=$(pwd)/../../../t
+TEST_DIRECTORY=$(pwd)/../../../t
+export TEST_DIRECTORY
 
 . ../../../t/test-lib.sh
 
diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 1c006a0..a9c75a2 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -13,7 +13,8 @@ refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}"
 
 test -z "$refspec" && prefix="refs"
 
-export GIT_DIR="$url/.git"
+GIT_DIR="$url/.git"
+export GIT_DIR
 
 force=
 
diff --git a/git-stash.sh b/git-stash.sh
index 4798bcf..4621d81 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -94,7 +94,8 @@ create_stash () {
 		# ease of unpacking later.
 		u_commit=$(
 			untracked_files | (
-				export GIT_INDEX_FILE="$TMPindex"
+				GIT_INDEX_FILE="$TMPindex" &&
+				export GIT_INDEX_FILE &&
 				rm -f "$TMPindex" &&
 				git update-index -z --add --remove --stdin &&
 				u_tree=$(git write-tree) &&

-- 
1.7.10.4
Jonathan Nieder· May 23, 2014, 16:18 UTC · re: Elia Pinto · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Elia Pinto wrote:
Show 6 quoted lines
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  contrib/subtree/t/t7900-subtree.sh |    3 ++-
>  git-remote-testgit.sh              |    3 ++-
>  git-stash.sh                       |    3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Junio C Hamano· May 23, 2014, 18:18 UTC · re: Elia Pinto · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Elia Pinto <gitter.spiros@gmail.com> writes:
> Found by check-non-portable-shell.pl
Thanks.
Makes me wonder why these two were missed, though.
 t/t3032-merge-recursive-options.sh | 6 +++++-
 t/t5560-http-backend-noserver.sh   | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)
Show changes to 2 files +10 −2

t/t3032-merge-recursive-options.sh, t/t5560-http-backend-noserver.sh

diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 5fd7bbb..4029c9c 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -14,7 +14,11 @@ test_description='merge-recursive options
 . ./test-lib.sh
 
 test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
-test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U
+if test_have_prereq GREP_STRIPS_CR
+then
+	GREP_OPTIONS=-U
+	export GREP_OPTIONS
+fi
 
 test_expect_success 'setup' '
 	conflict_hunks () {
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 9be9ae3..1e25128 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -5,7 +5,11 @@ test_description='test git-http-backend-noserver'
 
 HTTPD_DOCUMENT_ROOT_PATH="$TRASH_DIRECTORY"
 
-test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U
+if test_have_prereq GREP_STRIPS_CR
+then
+	GREP_OPTIONS=-U
+	export GREP_OPTIONS
+fi
 
 run_backend() {
 	echo "$2" |
Junio C Hamano· May 23, 2014, 18:39 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> Found by check-non-portable-shell.pl
>
> Thanks.
>
> Makes me wonder why these two were missed, though.
Perhaps something like this?

I didn't check other rules, though, because I still have a feeling that this "pretend to understand the shell syntax and point out issues, without really parsing the script" is fundamentally an error-prone approach and am hesitant to loosen the patterns too much.

 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/check-non-portable-shell.pl +1 −1
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 45971f4..24d7555 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -21,7 +21,7 @@ while (<>) {
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
 	/test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
-	/^\s*export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
+	/(?:^|[^-a-zA-Z0-9_])export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
 	# this resets our $. for each file
 	close ARGV if eof;
 }
Jonathan Nieder· May 23, 2014, 18:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Hi,
Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Makes me wonder why these two were missed, though.
>
> Perhaps something like this?
[...]
> -	/^\s*export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
> +	/(?:^|[^-a-zA-Z0-9_])export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
Messages crossed.

I think I like "\b" better for this, while I also share your lack of enthusiasm for parsing shell with something that is not a shell.

Maybe it's worth filing bugs against "posh -n" to make it (optionally?) notice things we want to detect with a goal of eventually moving to that.

Thanks, Jonathan

Jonathan Nieder· May 23, 2014, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Junio C Hamano wrote:
> Elia Pinto <gitter.spiros@gmail.com> writes:
Show 5 quoted lines
>> Found by check-non-portable-shell.pl
>
> Thanks.
>
> Makes me wonder why these two were missed, though.
Good catch.  check-non-portable-shell.pl uses an anchored regex:
	/^\s*export\s+[^=]*=/
Perhaps something like
	/\bexport\s+[A-Za-z0-9_]*=/
without anchoring would work better.
-- >8 --
Subject: test-lint: find unportable sed, echo, test, and export usage after &&

Instead of anchoring these checks with "^\s*", just check that the usage is preceded by a word boundary. So now we can catch

	test $cond && export foo=bar
just like we already catch
	test $cond &&
	export foo=bar

As a side effect, this will detect usage of "sed -i", "echo -n", "test a == b", and "export a=b" in comments. That is not ideal but it's potentially useful because people sometimes copy code from comments so it can be good to also avoid nonportable patterns there.

To avoid false positives, keep the checks for 'declare' and 'which' anchored. Those are frequently used words in normal English-language comments.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/check-non-portable-shell.pl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to diff +4 −4
diff --git i/t/check-non-portable-shell.pl w/t/check-non-portable-shell.pl
index 45971f4..b170cbc 100755
--- i/t/check-non-portable-shell.pl
+++ w/t/check-non-portable-shell.pl
@@ -16,12 +16,12 @@ sub err {
 
 while (<>) {
 	chomp;
-	/^\s*sed\s+-i/ and err 'sed -i is not portable';
-	/^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+	/\bsed\s+-i/ and err 'sed -i is not portable';
+	/\becho\s+-n/ and err 'echo -n is not portable (please use printf)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
-	/test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
-	/^\s*export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
+	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
+	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
 	# this resets our $. for each file
 	close ARGV if eof;
 }
Torsten Bögershausen· May 23, 2014, 20:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

On 2014-05-23 20.44, Jonathan Nieder wrote:
Show 26 quoted lines
> Junio C Hamano wrote:
>> Elia Pinto <gitter.spiros@gmail.com> writes:
> 
>>> Found by check-non-portable-shell.pl
>>
>> Thanks.
>>
>> Makes me wonder why these two were missed, though.
> 
> Good catch.  check-non-portable-shell.pl uses an anchored regex:
> 
> 	/^\s*export\s+[^=]*=/
> 
> Perhaps something like
> 
> 	/\bexport\s+[A-Za-z0-9_]*=/
> 
> without anchoring would work better.
> 
> -- >8 --
> Subject: test-lint: find unportable sed, echo, test, and export usage after &&
> 
> Instead of anchoring these checks with "^\s*", just check that the
> usage is preceded by a word boundary.  So now we can catch
> 
> 	test $cond && export foo=bar
Thanks for digging.

I wonder if we could keep the anchoring (to reduce false positives) and try to catch a line "command1 && command2" at the same time:

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 45971f4..f64e054 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -16,12 +16,12 @@ sub err {
 
 while (<>) {
        chomp;
-       /^\s*sed\s+-i/ and err 'sed -i is not portable';
-       /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
-       /^\s*declare\s+/ and err 'arrays/declare not portable';
-       /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+       /(^|&&)\s*sed\s+-i/ and err 'sed -i is not portable';
+       /(^|&&)\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+       /(^|&&)\s*declare\s+/ and err 'arrays/declare not portable';
+       /(^|&&)\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
        /test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
-       /^\s*export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
+       /(^|&&)\s*export\s+[^=]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
        # this resets our $. for each file
        close ARGV if eof;
 }
Junio C Hamano· May 23, 2014, 21:25 UTC · re: Torsten Bögershausen · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Torsten Bögershausen <tboegi@web.de> writes:
> I wonder if we could keep the anchoring (to reduce false positives)
> and try to catch a line "command1 && command2" at the same time:

Well, "command1 || command2" would be the next thing you want to catch, and I think you are at the entrance of a slipperly slope at that point.

Junio C Hamano· May 23, 2014, 21:56 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

Jonathan Nieder <jrnieder@gmail.com> writes:
> -	/^\s*export\s+[^=]*=/ and err '"export FOO=bar" is not por...
> +	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is n...

I see you also tightened what the "variable" to be assigned should look like, which is a good change to avoid false positives.

Because the inter-word dash is taken as a word "\b"oundary, that change incidentally prevents a false positive from triggering on something like:

	git fast-export --option=value
;-)
Eric Sunshine· May 23, 2014, 20:38 UTC · re: Elia Pinto · lore

Re: [PATCH v2] Get rid of the non portable shell export VAR=VALUE costruct

On Fri, May 23, 2014 at 6:15 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> Subject: Get rid of the non portable shell export VAR=VALUE costruct

s/costruct/construct/ s/non portable/non-portable/

Show 55 quoted lines
> Found by check-non-portable-shell.pl
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is the second version of the patch that includes
> Junio suggestions.
>
>  contrib/subtree/t/t7900-subtree.sh |    3 ++-
>  git-remote-testgit.sh              |    3 ++-
>  git-stash.sh                       |    3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 66ce4b0..8dc6840 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -8,7 +8,8 @@ This test verifies the basic operation of the merge, pull, add
>  and split subcommands of git subtree.
>  '
>
> -export TEST_DIRECTORY=$(pwd)/../../../t
> +TEST_DIRECTORY=$(pwd)/../../../t
> +export TEST_DIRECTORY
>
>  . ../../../t/test-lib.sh
>
> diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
> index 1c006a0..a9c75a2 100755
> --- a/git-remote-testgit.sh
> +++ b/git-remote-testgit.sh
> @@ -13,7 +13,8 @@ refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}"
>
>  test -z "$refspec" && prefix="refs"
>
> -export GIT_DIR="$url/.git"
> +GIT_DIR="$url/.git"
> +export GIT_DIR
>
>  force=
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4798bcf..4621d81 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -94,7 +94,8 @@ create_stash () {
>                 # ease of unpacking later.
>                 u_commit=$(
>                         untracked_files | (
> -                               export GIT_INDEX_FILE="$TMPindex"
> +                               GIT_INDEX_FILE="$TMPindex" &&
> +                               export GIT_INDEX_FILE &&
>                                 rm -f "$TMPindex" &&
>                                 git update-index -z --add --remove --stdin &&
>                                 u_tree=$(git write-tree) &&
> --

← back to recent threads