Re: [PATCH] t5801: properly test the test shell
Junio C Hamano venit, vidit, dixit 25.04.2013 19:12: Junio C Hamano gits...@pobox.com writes: Michael J Gruber g...@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 -- 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 g...@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. -- 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
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 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
Michael J Gruber g...@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 g...@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 21; 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
Johannes Sixt j.s...@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
--
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 tbo...@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. -- 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 gits...@pobox.com writes: Michael J Gruber g...@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 ' -- 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
