threads / patch / 33598

patcht5801: properly test the test shell

Subject: [PATCH] t5801: properly test the test shell

## tl;dr

11 messages between Apr 25, 2013 and Apr 26, 2013. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Michael J Gruber· Apr 25, 2013, 10:09 UTC · lore

fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a test which was meant to skip the test unless the test shell is bash. Unfortunately, it tests for the availability of bash only. But users can opt to use a different shell (using SHELL_PATH) for the tests even though bash is available.

At least for dash, 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17) is the commit which actually introduces a test (pushing without refspec) which fails to fail even though it is supposed to. It uses the construct:

VAR=value function arguments

Make t5801 actually test whether the test shell is bash.

An even better alternative would be to make the test POSIX compliant, of course.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t5801-remote-helpers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to t/t5801-remote-helpers.sh +1 −1

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index ed962c4..c979863 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-if ! type "${BASH-bash}" >/dev/null 2>&1; then
+if test $(basename "${SHELL_PATH}") != "bash"; then
 	skip_all='skipping remote-testgit tests, bash not available'
 	test_done
 fi

-- 
1.8.2.1.882.gefdb4b7

Johannes Sixt· Apr 25, 2013, 10:59 UTC · re: Michael J Gruber · lore

Re: [PATCH] t5801: properly test the test shell

Am 4/25/2013 12:09, schrieb Michael J Gruber:

Show 5 quoted lines

> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
> test which was meant to skip the test unless the test shell is bash.
> Unfortunately, it tests for the availability of bash only. But users can
> opt to use a different shell (using SHELL_PATH) for the tests even though
> bash is available.

After my patch this morning ("avoid process substitution"), there is not much bashism left in git-remote-testgit:

Show changes to git-remote-testgit +9 −5

diff --git a/git-remote-testgit b/git-remote-testgit
index e99d5fa..178d030 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 # Copyright (c) 2012 Felipe Contreras
 
 alias=$1
@@ -23,7 +23,6 @@ then
 	testgitmarks="$dir/testgit.marks"
 	test -e "$gitmarks" || >"$gitmarks"
 	test -e "$testgitmarks" || >"$testgitmarks"
-	testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
 fi
 
 while read line
@@ -70,7 +69,10 @@ do
 		fi
 
 		echo "feature done"
-		git fast-export "${testgitmarks_args[@]}" $refs |
+		git fast-export \
+			${testgitmarks:+"--import-marks=$testgitmarks"} \
+			${testgitmarks:+"--export-marks=$testgitmarks"} \
+			$refs |
 		sed -e "s#refs/heads/#${prefix}/heads/#g"
 		echo "done"
 		;;
@@ -89,7 +91,10 @@ do
 
 		before=$(git for-each-ref --format='%(refname) %(objectname)')
 
-		git fast-import "${testgitmarks_args[@]}" --quiet
+		git fast-import \
+			${testgitmarks:+"--import-marks=$testgitmarks"} \
+			${testgitmarks:+"--export-marks=$testgitmarks"} \
+			--quiet
 
 		# figure out which refs were updated
 		git for-each-ref --format='%(refname) %(objectname)' |

What's left is to take care of the shbang line, remove the bash
check from t5801, make a proper commit from this patch. But I leave
that to the interested reader. :-)

-- Hannes

Michael J Gruber· Apr 25, 2013, 11:21 UTC · re: Johannes Sixt · lore

Re: [PATCH] t5801: properly test the test shell

Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:

Show 9 quoted lines

> Am 4/25/2013 12:09, schrieb Michael J Gruber:
>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
>> test which was meant to skip the test unless the test shell is bash.
>> Unfortunately, it tests for the availability of bash only. But users can
>> opt to use a different shell (using SHELL_PATH) for the tests even though
>> bash is available.
> 
> After my patch this morning ("avoid process substitution"), there is not
> much bashism left in git-remote-testgit:

Is that a patch you submitted?

Show 47 quoted lines

> diff --git a/git-remote-testgit b/git-remote-testgit
> index e99d5fa..178d030 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/bin/sh
>  # Copyright (c) 2012 Felipe Contreras
>  
>  alias=$1
> @@ -23,7 +23,6 @@ then
>  	testgitmarks="$dir/testgit.marks"
>  	test -e "$gitmarks" || >"$gitmarks"
>  	test -e "$testgitmarks" || >"$testgitmarks"
> -	testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
>  fi
>  
>  while read line
> @@ -70,7 +69,10 @@ do
>  		fi
>  
>  		echo "feature done"
> -		git fast-export "${testgitmarks_args[@]}" $refs |
> +		git fast-export \
> +			${testgitmarks:+"--import-marks=$testgitmarks"} \
> +			${testgitmarks:+"--export-marks=$testgitmarks"} \
> +			$refs |
>  		sed -e "s#refs/heads/#${prefix}/heads/#g"
>  		echo "done"
>  		;;
> @@ -89,7 +91,10 @@ do
>  
>  		before=$(git for-each-ref --format='%(refname) %(objectname)')
>  
> -		git fast-import "${testgitmarks_args[@]}" --quiet
> +		git fast-import \
> +			${testgitmarks:+"--import-marks=$testgitmarks"} \
> +			${testgitmarks:+"--export-marks=$testgitmarks"} \
> +			--quiet
>  
>  		# figure out which refs were updated
>  		git for-each-ref --format='%(refname) %(objectname)' |
> 
> What's left is to take care of the shbang line, remove the bash
> check from t5801, make a proper commit from this patch. But I leave
> that to the interested reader. :-)
>

No, the problem (that I'm adressing) is not git-remote-testgit which uses bash unconditionally, independent of SHELL_PATH.

The problem is bashism(s) in t5801 itself. That is completely orthogonal to your (non-) patch.

Michael

Johannes Sixt· Apr 25, 2013, 11:35 UTC · re: Michael J Gruber · lore

Re: [PATCH] t5801: properly test the test shell

Am 4/25/2013 13:21, schrieb Michael J Gruber:

Show 12 quoted lines

> Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:
>> Am 4/25/2013 12:09, schrieb Michael J Gruber:
>>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
>>> test which was meant to skip the test unless the test shell is bash.
>>> Unfortunately, it tests for the availability of bash only. But users can
>>> opt to use a different shell (using SHELL_PATH) for the tests even though
>>> bash is available.
>>
>> After my patch this morning ("avoid process substitution"), there is not
>> much bashism left in git-remote-testgit:
> 
> Is that a patch you submitted?

It is not the patch I submitted this morning, but a patch on top that removes the remaining bashisms from git-remote-testgit.

Show 5 quoted lines

> No, the problem (that I'm adressing) is not git-remote-testgit which
> uses bash unconditionally, independent of SHELL_PATH.
> 
> The problem is bashism(s) in t5801 itself. That is completely orthogonal
> to your (non-) patch.

OK. But wouldn't it be nicer to remove that bashism as well and have portable t5801 and git-remote-testgit? :-)

-- Hannes

Junio C Hamano· Apr 25, 2013, 16:22 UTC · re: Johannes Sixt · lore

Re: [PATCH] t5801: properly test the test shell

Johannes Sixt <j.sixt@viscovery.net> writes:

Show 9 quoted lines

> Am 4/25/2013 12:09, schrieb Michael J Gruber:
>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
>> test which was meant to skip the test unless the test shell is bash.
>> Unfortunately, it tests for the availability of bash only. But users can
>> opt to use a different shell (using SHELL_PATH) for the tests even though
>> bash is available.
>
> After my patch this morning ("avoid process substitution"), there is not
> much bashism left in git-remote-testgit:

Not much, or no more?  I think the latter is a very worthy goal.

Even if it is only a test helper, we would prefer to have a portable one.

Show 48 quoted lines

> diff --git a/git-remote-testgit b/git-remote-testgit
> index e99d5fa..178d030 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/bin/sh
>  # Copyright (c) 2012 Felipe Contreras
>  
>  alias=$1
> @@ -23,7 +23,6 @@ then
>  	testgitmarks="$dir/testgit.marks"
>  	test -e "$gitmarks" || >"$gitmarks"
>  	test -e "$testgitmarks" || >"$testgitmarks"
> -	testgitmarks_args=( "--"{import,export}"-marks=$testgitmarks" )
>  fi
>  
>  while read line
> @@ -70,7 +69,10 @@ do
>  		fi
>  
>  		echo "feature done"
> -		git fast-export "${testgitmarks_args[@]}" $refs |
> +		git fast-export \
> +			${testgitmarks:+"--import-marks=$testgitmarks"} \
> +			${testgitmarks:+"--export-marks=$testgitmarks"} \
> +			$refs |
>  		sed -e "s#refs/heads/#${prefix}/heads/#g"
>  		echo "done"
>  		;;
> @@ -89,7 +91,10 @@ do
>  
>  		before=$(git for-each-ref --format='%(refname) %(objectname)')
>  
> -		git fast-import "${testgitmarks_args[@]}" --quiet
> +		git fast-import \
> +			${testgitmarks:+"--import-marks=$testgitmarks"} \
> +			${testgitmarks:+"--export-marks=$testgitmarks"} \
> +			--quiet
>  
>  		# figure out which refs were updated
>  		git for-each-ref --format='%(refname) %(objectname)' |
>
> What's left is to take care of the shbang line, remove the bash
> check from t5801, make a proper commit from this patch. But I leave
> that to the interested reader. :-)
>
> -- Hannes

Torsten Bögershausen· Apr 25, 2013, 13:06 UTC · re: Michael J Gruber · lore

Re: [PATCH] t5801: properly test the test shell

On 25.04.13 12:09, Michael J Gruber wrote:

Show 38 quoted lines

> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
> test which was meant to skip the test unless the test shell is bash.
> Unfortunately, it tests for the availability of bash only. But users can
> opt to use a different shell (using SHELL_PATH) for the tests even though
> bash is available.
> 
> At least for dash,
> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
> is the commit which actually introduces a test (pushing without refspec)
> which fails to fail even though it is supposed to. It uses the
> construct:
> 
> VAR=value function arguments
> 
> Make t5801 actually test whether the test shell is bash.
> 
> An even better alternative would be to make the test POSIX compliant, of
> course.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  t/t5801-remote-helpers.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index ed962c4..c979863 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-gpg.sh
>  
> -if ! type "${BASH-bash}" >/dev/null 2>&1; then
> +if test $(basename "${SHELL_PATH}") != "bash"; then
>  	skip_all='skipping remote-testgit tests, bash not available'
>  	test_done
>  fi
>

test_description='test git-specific bash prompt functions'

. ./lib-bash.sh

Junio C Hamano· Apr 25, 2013, 16:25 UTC · re: Torsten Bögershausen · lore

Re: [PATCH] t5801: properly test the test shell

Torsten Bögershausen <tboegi@web.de> writes:

> Could we use the same logic as in t9903:
> ...
> . ./lib-bash.sh

Please no. The test itself is not about something that checks how we behave under bash (which is what 9903 wants to see). The use of construct that is portable in t5801 is a pure and simple bug, and it should not be hard to fix it.

If there still is such an unportable construct left, that is.

Junio C Hamano· Apr 25, 2013, 16:20 UTC · re: Michael J Gruber · lore

Re: [PATCH] t5801: properly test the test shell

Michael J Gruber <git@drmicha.warpmail.net> writes:

> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
> test which was meant to skip the test unless the test shell is bash.
> Unfortunately, it tests for the availability of bash only.

True.

Show 11 quoted lines

> But users can
> opt to use a different shell (using SHELL_PATH) for the tests even though
> bash is available.
>
> At least for dash,
> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
> is the commit which actually introduces a test (pushing without refspec)
> which fails to fail even though it is supposed to. It uses the
> construct:
>
> VAR=value function arguments

The right fix for that is to fix that line, so that the test itself can run under any sane POSIX shell, isn't it? The test in turn may need to run git-remote-testgit, which, without J6t's updates, only is usable under bash, but to make sure the test will choke on absence of bash, the existing check should be sufficient, no?

Show 23 quoted lines

> Make t5801 actually test whether the test shell is bash.
>
> An even better alternative would be to make the test POSIX compliant, of
> course.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  t/t5801-remote-helpers.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index ed962c4..c979863 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-gpg.sh
>  
> -if ! type "${BASH-bash}" >/dev/null 2>&1; then
> +if test $(basename "${SHELL_PATH}") != "bash"; then
>  	skip_all='skipping remote-testgit tests, bash not available'
>  	test_done
>  fi

Junio C Hamano· Apr 25, 2013, 17:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5801: properly test the test shell

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

Show 25 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
>> test which was meant to skip the test unless the test shell is bash.
>> Unfortunately, it tests for the availability of bash only.
>
> True.
>
>> But users can
>> opt to use a different shell (using SHELL_PATH) for the tests even though
>> bash is available.
>>
>> At least for dash,
>> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
>> is the commit which actually introduces a test (pushing without refspec)
>> which fails to fail even though it is supposed to. It uses the
>> construct:
>>
>> VAR=value function arguments
>
> The right fix for that is to fix that line, so that the test itself
> can run under any sane POSIX shell, isn't it?  The test in turn may
> need to run git-remote-testgit, which, without J6t's updates, only
> is usable under bash, but to make sure the test will choke on
> absence of bash, the existing check should be sufficient, no?

Curiously enough, there were a few instances of the correct "set and export environment explicitly during the life of subshell" construct already in the script. I found only this one as problematic.

Does it fix your issue without your change?

It is a separate issue to port git-remote-testgit to POSIX (J6t already has a two part draft), move it to git-remote-testgit.sh, and get its shebang line preprocessed like all other shell scripts. I think it is worth doing.

Takers?

 t/t5801-remote-helpers.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to t/t5801-remote-helpers.sh +3 −1

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 4dcf744..c956abd 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '
 	(cd local2 &&
 	echo content >>file &&
 	git commit -a -m ten &&
-	GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2>../error) &&
+	GIT_REMOTE_TESTGIT_REFSPEC="" &&
+	export GIT_REMOTE_TESTGIT_REFSPEC &&
+	test_must_fail git push 2>../error) &&
 	grep "remote-helper doesn.t support push; refspec needed" error
 '

Michael J Gruber· Apr 26, 2013, 09:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5801: properly test the test shell

Junio C Hamano venit, vidit, dixit 25.04.2013 19:12:

Show 60 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
>>> test which was meant to skip the test unless the test shell is bash.
>>> Unfortunately, it tests for the availability of bash only.
>>
>> True.
>>
>>> But users can
>>> opt to use a different shell (using SHELL_PATH) for the tests even though
>>> bash is available.
>>>
>>> At least for dash,
>>> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
>>> is the commit which actually introduces a test (pushing without refspec)
>>> which fails to fail even though it is supposed to. It uses the
>>> construct:
>>>
>>> VAR=value function arguments
>>
>> The right fix for that is to fix that line, so that the test itself
>> can run under any sane POSIX shell, isn't it?  The test in turn may
>> need to run git-remote-testgit, which, without J6t's updates, only
>> is usable under bash, but to make sure the test will choke on
>> absence of bash, the existing check should be sufficient, no?
> 
> Curiously enough, there were a few instances of the correct "set and
> export environment explicitly during the life of subshell" construct
> already in the script.  I found only this one as problematic.
> 
> Does it fix your issue without your change?
> 
> It is a separate issue to port git-remote-testgit to POSIX (J6t
> already has a two part draft), move it to git-remote-testgit.sh, and
> get its shebang line preprocessed like all other shell scripts.  I
> think it is worth doing.
> 
> Takers?
> 
>  t/t5801-remote-helpers.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> index 4dcf744..c956abd 100755
> --- a/t/t5801-remote-helpers.sh
> +++ b/t/t5801-remote-helpers.sh
> @@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '
>  	(cd local2 &&
>  	echo content >>file &&
>  	git commit -a -m ten &&
> -	GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2>../error) &&
> +	GIT_REMOTE_TESTGIT_REFSPEC="" &&
> +	export GIT_REMOTE_TESTGIT_REFSPEC &&
> +	test_must_fail git push 2>../error) &&
>  	grep "remote-helper doesn.t support push; refspec needed" error
>  '
>  
>

Perfect, I just failed to notice that the subshell would make the export local to that test.

Thanks!

Michael

Junio C Hamano· Apr 26, 2013, 18:33 UTC · re: Michael J Gruber · lore

Re: [PATCH] t5801: properly test the test shell

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 6 quoted lines

>> It is a separate issue to port git-remote-testgit to POSIX (J6t
>> already has a two part draft), move it to git-remote-testgit.sh, and
>> get its shebang line preprocessed like all other shell scripts.  I
>> think it is worth doing.
>> 
>> Takers?

By the way, this hint still stands ;-)

Show 23 quoted lines

>> 
>>  t/t5801-remote-helpers.sh | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
>> index 4dcf744..c956abd 100755
>> --- a/t/t5801-remote-helpers.sh
>> +++ b/t/t5801-remote-helpers.sh
>> @@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '
>>  	(cd local2 &&
>>  	echo content >>file &&
>>  	git commit -a -m ten &&
>> -	GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2>../error) &&
>> +	GIT_REMOTE_TESTGIT_REFSPEC="" &&
>> +	export GIT_REMOTE_TESTGIT_REFSPEC &&
>> +	test_must_fail git push 2>../error) &&
>>  	grep "remote-helper doesn.t support push; refspec needed" error
>>  '
>>  
>> 
>
> Perfect, I just failed to notice that the subshell would make the export
> local to that test.

Good.

I think I queued the fix near the tip of that topic yesterday.