[PATCH] t5801: properly test the test shell

[PATCH] t5801: properly test the test shell

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

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
-- 
1.8.2.1.882.gefdb4b7

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

[Johannes Sixt]

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:

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

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

[Michael J Gruber]

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?

> 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

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

[Johannes Sixt]

Am 4/25/2013 13:21, schrieb Michael J Gruber:
> 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.

> 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

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

[Torsten Bögershausen]

On 25.04.13 12:09, Michael J Gruber wrote:
> 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
> 
Could we use the same logic as in t9903:
#!/bin/sh
#
# Copyright (c) 2012 SZEDER Gábor
#

test_description='test git-specific bash prompt functions'

. ./lib-bash.sh

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

[Junio C Hamano]

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?

> 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

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

[Junio C Hamano]

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

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

> 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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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
 '
 

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 25.04.2013 19:12:
> 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

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

[Junio C Hamano]

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

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

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