{"thread":{"id":"33598","subject":"[PATCH] t5801: properly test the test shell","startedAt":"2013-04-25T10:09:51Z","lastAt":"2013-04-26T18:33:58Z","messageCount":11,"participants":["Michael J Gruber","Johannes Sixt","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"215421","messageId":"ff7ccf22012f069ceca054d90aa0f72666cc11c2.1366884583.git.git@drmicha.warpmail.net","threadId":"33598","inReplyTo":null,"subject":"[PATCH] t5801: properly test the test shell","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-25T10:09:51Z","receivedAt":"2013-04-25T10:09:51Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\ntest which was meant to skip the test unless the test shell is bash.\nUnfortunately, it tests for the availability of bash only. But users can\nopt to use a different shell (using SHELL_PATH) for the tests even though\nbash is available.\n\nAt least for dash,\n21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)\nis the commit which actually introduces a test (pushing without refspec)\nwhich fails to fail even though it is supposed to. It uses the\nconstruct:\n\nVAR=value function arguments\n\nMake t5801 actually test whether the test shell is bash.\n\nAn even better alternative would be to make the test POSIX compliant, of\ncourse.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n t/t5801-remote-helpers.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex ed962c4..c979863 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n-if ! type \"${BASH-bash}\" >/dev/null 2>&1; then\n+if test $(basename \"${SHELL_PATH}\") != \"bash\"; then\n \tskip_all='skipping remote-testgit tests, bash not available'\n \ttest_done\n fi\n-- \n1.8.2.1.882.gefdb4b7\n"},{"id":"215425","messageId":"51790C9D.8080901@viscovery.net","threadId":"33598","inReplyTo":"ff7ccf22012f069ceca054d90aa0f72666cc11c2.1366884583.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-04-25T10:59:41Z","receivedAt":"2013-04-25T10:59:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/25/2013 12:09, schrieb Michael J Gruber:\n> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n> test which was meant to skip the test unless the test shell is bash.\n> Unfortunately, it tests for the availability of bash only. But users can\n> opt to use a different shell (using SHELL_PATH) for the tests even though\n> bash is available.\n\nAfter my patch this morning (\"avoid process substitution\"), there is not\nmuch bashism left in git-remote-testgit:\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex e99d5fa..178d030 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -1,4 +1,4 @@\n-#!/usr/bin/env bash\n+#!/bin/sh\n # Copyright (c) 2012 Felipe Contreras\n \n alias=$1\n@@ -23,7 +23,6 @@ then\n \ttestgitmarks=\"$dir/testgit.marks\"\n \ttest -e \"$gitmarks\" || >\"$gitmarks\"\n \ttest -e \"$testgitmarks\" || >\"$testgitmarks\"\n-\ttestgitmarks_args=( \"--\"{import,export}\"-marks=$testgitmarks\" )\n fi\n \n while read line\n@@ -70,7 +69,10 @@ do\n \t\tfi\n \n \t\techo \"feature done\"\n-\t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n+\t\tgit fast-export \\\n+\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n+\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n+\t\t\t$refs |\n \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n \t\techo \"done\"\n \t\t;;\n@@ -89,7 +91,10 @@ do\n \n \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n \n-\t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\n+\t\tgit fast-import \\\n+\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n+\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n+\t\t\t--quiet\n \n \t\t# figure out which refs were updated\n \t\tgit for-each-ref --format='%(refname) %(objectname)' |\n\nWhat's left is to take care of the shbang line, remove the bash\ncheck from t5801, make a proper commit from this patch. But I leave\nthat to the interested reader. :-)\n\n-- Hannes\n"},{"id":"215427","messageId":"517911CC.2060902@drmicha.warpmail.net","threadId":"33598","inReplyTo":"51790C9D.8080901@viscovery.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-25T11:21:48Z","receivedAt":"2013-04-25T11:21:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:\n> Am 4/25/2013 12:09, schrieb Michael J Gruber:\n>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n>> test which was meant to skip the test unless the test shell is bash.\n>> Unfortunately, it tests for the availability of bash only. But users can\n>> opt to use a different shell (using SHELL_PATH) for the tests even though\n>> bash is available.\n> \n> After my patch this morning (\"avoid process substitution\"), there is not\n> much bashism left in git-remote-testgit:\n\nIs that a patch you submitted?\n\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index e99d5fa..178d030 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -1,4 +1,4 @@\n> -#!/usr/bin/env bash\n> +#!/bin/sh\n>  # Copyright (c) 2012 Felipe Contreras\n>  \n>  alias=$1\n> @@ -23,7 +23,6 @@ then\n>  \ttestgitmarks=\"$dir/testgit.marks\"\n>  \ttest -e \"$gitmarks\" || >\"$gitmarks\"\n>  \ttest -e \"$testgitmarks\" || >\"$testgitmarks\"\n> -\ttestgitmarks_args=( \"--\"{import,export}\"-marks=$testgitmarks\" )\n>  fi\n>  \n>  while read line\n> @@ -70,7 +69,10 @@ do\n>  \t\tfi\n>  \n>  \t\techo \"feature done\"\n> -\t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n> +\t\tgit fast-export \\\n> +\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n> +\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n> +\t\t\t$refs |\n>  \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n>  \t\techo \"done\"\n>  \t\t;;\n> @@ -89,7 +91,10 @@ do\n>  \n>  \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n>  \n> -\t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\n> +\t\tgit fast-import \\\n> +\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n> +\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n> +\t\t\t--quiet\n>  \n>  \t\t# figure out which refs were updated\n>  \t\tgit for-each-ref --format='%(refname) %(objectname)' |\n> \n> What's left is to take care of the shbang line, remove the bash\n> check from t5801, make a proper commit from this patch. But I leave\n> that to the interested reader. :-)\n> \n\nNo, the problem (that I'm adressing) is not git-remote-testgit which\nuses bash unconditionally, independent of SHELL_PATH.\n\nThe problem is bashism(s) in t5801 itself. That is completely orthogonal\nto your (non-) patch.\n\nMichael\n"},{"id":"215440","messageId":"51791514.9000409@viscovery.net","threadId":"33598","inReplyTo":"517911CC.2060902@drmicha.warpmail.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-04-25T11:35:48Z","receivedAt":"2013-04-25T11:35:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/25/2013 13:21, schrieb Michael J Gruber:\n> Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:\n>> Am 4/25/2013 12:09, schrieb Michael J Gruber:\n>>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n>>> test which was meant to skip the test unless the test shell is bash.\n>>> Unfortunately, it tests for the availability of bash only. But users can\n>>> opt to use a different shell (using SHELL_PATH) for the tests even though\n>>> bash is available.\n>>\n>> After my patch this morning (\"avoid process substitution\"), there is not\n>> much bashism left in git-remote-testgit:\n> \n> Is that a patch you submitted?\n\nIt is not the patch I submitted this morning, but a patch on top that\nremoves the remaining bashisms from git-remote-testgit.\n\n> No, the problem (that I'm adressing) is not git-remote-testgit which\n> uses bash unconditionally, independent of SHELL_PATH.\n> \n> The problem is bashism(s) in t5801 itself. That is completely orthogonal\n> to your (non-) patch.\n\nOK. But wouldn't it be nicer to remove that bashism as well and have\nportable t5801 and git-remote-testgit? :-)\n\n-- Hannes\n"},{"id":"215443","messageId":"51792A48.60603@web.de","threadId":"33598","inReplyTo":"ff7ccf22012f069ceca054d90aa0f72666cc11c2.1366884583.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-04-25T13:06:16Z","receivedAt":"2013-04-25T13:06:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 25.04.13 12:09, Michael J Gruber wrote:\n> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n> test which was meant to skip the test unless the test shell is bash.\n> Unfortunately, it tests for the availability of bash only. But users can\n> opt to use a different shell (using SHELL_PATH) for the tests even though\n> bash is available.\n> \n> At least for dash,\n> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)\n> is the commit which actually introduces a test (pushing without refspec)\n> which fails to fail even though it is supposed to. It uses the\n> construct:\n> \n> VAR=value function arguments\n> \n> Make t5801 actually test whether the test shell is bash.\n> \n> An even better alternative would be to make the test POSIX compliant, of\n> course.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n>  t/t5801-remote-helpers.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index ed962c4..c979863 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY\"/lib-gpg.sh\n>  \n> -if ! type \"${BASH-bash}\" >/dev/null 2>&1; then\n> +if test $(basename \"${SHELL_PATH}\") != \"bash\"; then\n>  \tskip_all='skipping remote-testgit tests, bash not available'\n>  \ttest_done\n>  fi\n> \nCould we use the same logic as in t9903:\n#!/bin/sh\n#\n# Copyright (c) 2012 SZEDER Gábor\n#\n\ntest_description='test git-specific bash prompt functions'\n\n. ./lib-bash.sh\n"},{"id":"215448","messageId":"7vppxi60pj.fsf@alter.siamese.dyndns.org","threadId":"33598","inReplyTo":"ff7ccf22012f069ceca054d90aa0f72666cc11c2.1366884583.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-25T16:20:56Z","receivedAt":"2013-04-25T16:20:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n> test which was meant to skip the test unless the test shell is bash.\n> Unfortunately, it tests for the availability of bash only.\n\nTrue.\n\n> But users can\n> opt to use a different shell (using SHELL_PATH) for the tests even though\n> bash is available.\n>\n> At least for dash,\n> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)\n> is the commit which actually introduces a test (pushing without refspec)\n> which fails to fail even though it is supposed to. It uses the\n> construct:\n>\n> VAR=value function arguments\n\nThe right fix for that is to fix that line, so that the test itself\ncan run under any sane POSIX shell, isn't it?  The test in turn may\nneed to run git-remote-testgit, which, without J6t's updates, only\nis usable under bash, but to make sure the test will choke on\nabsence of bash, the existing check should be sufficient, no?\n\n> Make t5801 actually test whether the test shell is bash.\n>\n> An even better alternative would be to make the test POSIX compliant, of\n> course.\n>\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n>  t/t5801-remote-helpers.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index ed962c4..c979863 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY\"/lib-gpg.sh\n>  \n> -if ! type \"${BASH-bash}\" >/dev/null 2>&1; then\n> +if test $(basename \"${SHELL_PATH}\") != \"bash\"; then\n>  \tskip_all='skipping remote-testgit tests, bash not available'\n>  \ttest_done\n>  fi\n"},{"id":"215449","messageId":"7vli8660nj.fsf@alter.siamese.dyndns.org","threadId":"33598","inReplyTo":"51790C9D.8080901@viscovery.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-25T16:22:08Z","receivedAt":"2013-04-25T16:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 4/25/2013 12:09, schrieb Michael J Gruber:\n>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n>> test which was meant to skip the test unless the test shell is bash.\n>> Unfortunately, it tests for the availability of bash only. But users can\n>> opt to use a different shell (using SHELL_PATH) for the tests even though\n>> bash is available.\n>\n> After my patch this morning (\"avoid process substitution\"), there is not\n> much bashism left in git-remote-testgit:\n\nNot much, or no more?  I think the latter is a very worthy goal.\n\nEven if it is only a test helper, we would prefer to have a portable\none.\n\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index e99d5fa..178d030 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -1,4 +1,4 @@\n> -#!/usr/bin/env bash\n> +#!/bin/sh\n>  # Copyright (c) 2012 Felipe Contreras\n>  \n>  alias=$1\n> @@ -23,7 +23,6 @@ then\n>  \ttestgitmarks=\"$dir/testgit.marks\"\n>  \ttest -e \"$gitmarks\" || >\"$gitmarks\"\n>  \ttest -e \"$testgitmarks\" || >\"$testgitmarks\"\n> -\ttestgitmarks_args=( \"--\"{import,export}\"-marks=$testgitmarks\" )\n>  fi\n>  \n>  while read line\n> @@ -70,7 +69,10 @@ do\n>  \t\tfi\n>  \n>  \t\techo \"feature done\"\n> -\t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n> +\t\tgit fast-export \\\n> +\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n> +\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n> +\t\t\t$refs |\n>  \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n>  \t\techo \"done\"\n>  \t\t;;\n> @@ -89,7 +91,10 @@ do\n>  \n>  \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n>  \n> -\t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\n> +\t\tgit fast-import \\\n> +\t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n> +\t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n> +\t\t\t--quiet\n>  \n>  \t\t# figure out which refs were updated\n>  \t\tgit for-each-ref --format='%(refname) %(objectname)' |\n>\n> What's left is to take care of the shbang line, remove the bash\n> check from t5801, make a proper commit from this patch. But I leave\n> that to the interested reader. :-)\n>\n> -- Hannes\n"},{"id":"215451","messageId":"7vhaiu60he.fsf@alter.siamese.dyndns.org","threadId":"33598","inReplyTo":"51792A48.60603@web.de","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-25T16:25:49Z","receivedAt":"2013-04-25T16:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Could we use the same logic as in t9903:\n> ...\n> . ./lib-bash.sh\n\nPlease no.  The test itself is not about something that checks how\nwe behave under bash (which is what 9903 wants to see).  The use of\nconstruct that is portable in t5801 is a pure and simple bug, and it\nshould not be hard to fix it.\n\nIf there still is such an unportable construct left, that is.\n"},{"id":"215458","messageId":"7vfvye4jrc.fsf@alter.siamese.dyndns.org","threadId":"33598","inReplyTo":"7vppxi60pj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-25T17:12:23Z","receivedAt":"2013-04-25T17:12:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>\n>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n>> test which was meant to skip the test unless the test shell is bash.\n>> Unfortunately, it tests for the availability of bash only.\n>\n> True.\n>\n>> But users can\n>> opt to use a different shell (using SHELL_PATH) for the tests even though\n>> bash is available.\n>>\n>> At least for dash,\n>> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)\n>> is the commit which actually introduces a test (pushing without refspec)\n>> which fails to fail even though it is supposed to. It uses the\n>> construct:\n>>\n>> VAR=value function arguments\n>\n> The right fix for that is to fix that line, so that the test itself\n> can run under any sane POSIX shell, isn't it?  The test in turn may\n> need to run git-remote-testgit, which, without J6t's updates, only\n> is usable under bash, but to make sure the test will choke on\n> absence of bash, the existing check should be sufficient, no?\n\nCuriously enough, there were a few instances of the correct \"set and\nexport environment explicitly during the life of subshell\" construct\nalready in the script.  I found only this one as problematic.\n\nDoes it fix your issue without your change?\n\nIt is a separate issue to port git-remote-testgit to POSIX (J6t\nalready has a two part draft), move it to git-remote-testgit.sh, and\nget its shebang line preprocessed like all other shell scripts.  I\nthink it is worth doing.\n\nTakers?\n\n t/t5801-remote-helpers.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 4dcf744..c956abd 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '\n \t(cd local2 &&\n \techo content >>file &&\n \tgit commit -a -m ten &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2>../error) &&\n+\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" &&\n+\texport GIT_REMOTE_TESTGIT_REFSPEC &&\n+\ttest_must_fail git push 2>../error) &&\n \tgrep \"remote-helper doesn.t support push; refspec needed\" error\n '\n \n"},{"id":"215584","messageId":"517A4F05.5030609@drmicha.warpmail.net","threadId":"33598","inReplyTo":"7vfvye4jrc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-04-26T09:55:17Z","receivedAt":"2013-04-26T09:55:17Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 25.04.2013 19:12:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>>\n>>> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a\n>>> test which was meant to skip the test unless the test shell is bash.\n>>> Unfortunately, it tests for the availability of bash only.\n>>\n>> True.\n>>\n>>> But users can\n>>> opt to use a different shell (using SHELL_PATH) for the tests even though\n>>> bash is available.\n>>>\n>>> At least for dash,\n>>> 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)\n>>> is the commit which actually introduces a test (pushing without refspec)\n>>> which fails to fail even though it is supposed to. It uses the\n>>> construct:\n>>>\n>>> VAR=value function arguments\n>>\n>> The right fix for that is to fix that line, so that the test itself\n>> can run under any sane POSIX shell, isn't it?  The test in turn may\n>> need to run git-remote-testgit, which, without J6t's updates, only\n>> is usable under bash, but to make sure the test will choke on\n>> absence of bash, the existing check should be sufficient, no?\n> \n> Curiously enough, there were a few instances of the correct \"set and\n> export environment explicitly during the life of subshell\" construct\n> already in the script.  I found only this one as problematic.\n> \n> Does it fix your issue without your change?\n> \n> It is a separate issue to port git-remote-testgit to POSIX (J6t\n> already has a two part draft), move it to git-remote-testgit.sh, and\n> get its shebang line preprocessed like all other shell scripts.  I\n> think it is worth doing.\n> \n> Takers?\n> \n>  t/t5801-remote-helpers.sh | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 4dcf744..c956abd 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '\n>  \t(cd local2 &&\n>  \techo content >>file &&\n>  \tgit commit -a -m ten &&\n> -\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2>../error) &&\n> +\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" &&\n> +\texport GIT_REMOTE_TESTGIT_REFSPEC &&\n> +\ttest_must_fail git push 2>../error) &&\n>  \tgrep \"remote-helper doesn.t support push; refspec needed\" error\n>  '\n>  \n> \n\nPerfect, I just failed to notice that the subshell would make the export\nlocal to that test.\n\nThanks!\n\nMichael\n"},{"id":"215607","messageId":"7vzjwlw38p.fsf@alter.siamese.dyndns.org","threadId":"33598","inReplyTo":"517A4F05.5030609@drmicha.warpmail.net","subject":"Re: [PATCH] t5801: properly test the test shell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-26T18:33:58Z","receivedAt":"2013-04-26T18:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n>> It is a separate issue to port git-remote-testgit to POSIX (J6t\n>> already has a two part draft), move it to git-remote-testgit.sh, and\n>> get its shebang line preprocessed like all other shell scripts.  I\n>> think it is worth doing.\n>> \n>> Takers?\n\nBy the way, this hint still stands ;-)\n\n>> \n>>  t/t5801-remote-helpers.sh | 4 +++-\n>>  1 file changed, 3 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n>> index 4dcf744..c956abd 100755\n>> --- a/t/t5801-remote-helpers.sh\n>> +++ b/t/t5801-remote-helpers.sh\n>> @@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '\n>>  \t(cd local2 &&\n>>  \techo content >>file &&\n>>  \tgit commit -a -m ten &&\n>> -\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2>../error) &&\n>> +\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" &&\n>> +\texport GIT_REMOTE_TESTGIT_REFSPEC &&\n>> +\ttest_must_fail git push 2>../error) &&\n>>  \tgrep \"remote-helper doesn.t support push; refspec needed\" error\n>>  '\n>>  \n>> \n>\n> Perfect, I just failed to notice that the subshell would make the export\n> local to that test.\n\nGood.\n\nI think I queued the fix near the tip of that topic yesterday.\n"}]}