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

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

[Elia Pinto]

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) &&
-- 
1.7.10.4

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

[Jonathan Nieder]

Elia Pinto wrote:

> 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>

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

[Junio C Hamano]

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(-)

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" |

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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(-)

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;
 }

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

[Jonathan Nieder]

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

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

[Jonathan Nieder]

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

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(-)

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;
 }

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

[Torsten Bögershausen]

On 2014-05-23 20.44, Jonathan Nieder wrote:
> 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:

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;
 }

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

;-)

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

[Eric Sunshine]

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/

> 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) &&
> --