Re: [PATCH] t5801: properly test the test shell
Michael J Gruber 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
Junio C Hamano venit, vidit, dixit 25.04.2013 19:12: > Junio C Hamano writes: > >> Michael J Gruber 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
Junio C Hamano writes: > Michael J Gruber 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 ' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
Torsten Bögershausen 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
Johannes Sixt 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
Michael J Gruber 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5801: properly test the test shell
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] t5801: properly test the test shell
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html