{"thread":{"id":"24910","subject":"[PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","startedAt":"2010-08-30T12:00:42Z","lastAt":"2010-08-31T18:11:30Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"149306","messageId":"1283169642-21917-1-git-send-email-avarab@gmail.com","threadId":"24910","inReplyTo":null,"subject":"[PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-30T12:00:42Z","receivedAt":"2010-08-30T12:00:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the test_create_repo code added in v1.2.2~6 to use a subshell\ninstead of keeping track of the old working directory and cd-ing back\nwhen it's done.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/test-lib.sh |    6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex dc934b8..778ba8d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -687,14 +687,12 @@ test_when_finished () {\n test_create_repo () {\n \ttest \"$#\" = 1 ||\n \terror \"bug in the test script: not 1 parameter to test-create-repo\"\n-\towd=`pwd`\n \trepo=\"$1\"\n \tmkdir -p \"$repo\"\n-\tcd \"$repo\" || error \"Cannot setup test environment\"\n+\t(cd \"$repo\" || error \"Cannot setup test environment\"\n \t\"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n \terror \"cannot run git init -- have you built things yet?\"\n-\tmv .git/hooks .git/hooks-disabled\n-\tcd \"$owd\"\n+\tmv .git/hooks .git/hooks-disabled)\n }\n \n test_done () {\n-- \n1.7.2.2.518.gf0ba8\n"},{"id":"149339","messageId":"20100830155723.GC3292@burratino","threadId":"24910","inReplyTo":"1283169642-21917-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T15:57:23Z","receivedAt":"2010-08-30T15:57:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> +++ b/t/test-lib.sh\n> @@ -687,14 +687,12 @@ test_when_finished () {\n>  test_create_repo () {\n>  \ttest \"$#\" = 1 ||\n>  \terror \"bug in the test script: not 1 parameter to test-create-repo\"\n> -\towd=`pwd`\n>  \trepo=\"$1\"\n>  \tmkdir -p \"$repo\"\n> -\tcd \"$repo\" || error \"Cannot setup test environment\"\n> +\t(cd \"$repo\" || error \"Cannot setup test environment\"\n>  \t\"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n>  \terror \"cannot run git init -- have you built things yet?\"\n> -\tmv .git/hooks .git/hooks-disabled\n> -\tcd \"$owd\"\n> +\tmv .git/hooks .git/hooks-disabled)\n\nStyle: why not use\n\n\t(\n\t\tcd \"$repo\" ...\n\t\t... .git/hooks-disabled\n\t)\n\n?\n"},{"id":"149340","messageId":"AANLkTi=z9uUB_YZ9Sb0O8JzaEjazKDzqB=ZVtqTTDf2K@mail.gmail.com","threadId":"24910","inReplyTo":"20100830155723.GC3292@burratino","subject":"Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-30T16:01:43Z","receivedAt":"2010-08-30T16:01:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> +++ b/t/test-lib.sh\n>> @@ -687,14 +687,12 @@ test_when_finished () {\n>>  test_create_repo () {\n>>       test \"$#\" = 1 ||\n>>       error \"bug in the test script: not 1 parameter to test-create-repo\"\n>> -     owd=`pwd`\n>>       repo=\"$1\"\n>>       mkdir -p \"$repo\"\n>> -     cd \"$repo\" || error \"Cannot setup test environment\"\n>> +     (cd \"$repo\" || error \"Cannot setup test environment\"\n>>       \"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n>>       error \"cannot run git init -- have you built things yet?\"\n>> -     mv .git/hooks .git/hooks-disabled\n>> -     cd \"$owd\"\n>> +     mv .git/hooks .git/hooks-disabled)\n>\n> Style: why not use\n>\n>        (\n>                cd \"$repo\" ...\n>                ... .git/hooks-disabled\n>        )\n>\n\nI've seen both used and I don't know which is preferred.\n"},{"id":"149341","messageId":"20100830160444.GE3292@burratino","threadId":"24910","inReplyTo":"AANLkTi=z9uUB_YZ9Sb0O8JzaEjazKDzqB=ZVtqTTDf2K@mail.gmail.com","subject":"Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T16:04:44Z","receivedAt":"2010-08-30T16:04:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> Ævar Arnfjörð Bjarmason wrote:\n\n>>> -     cd \"$repo\" || error \"Cannot setup test environment\"\n>>> +     (cd \"$repo\" || error \"Cannot setup test environment\"\n>>>       \"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n>>>       error \"cannot run git init -- have you built things yet?\"\n>>> -     mv .git/hooks .git/hooks-disabled\n>>> -     cd \"$owd\"\n>>> +     mv .git/hooks .git/hooks-disabled)\n>>\n>> Style: why not use\n>>\n>>        (\n>>                cd \"$repo\" ...\n>>                ... .git/hooks-disabled\n>>        )\n>>\n>\n> I've seen both used and I don't know which is preferred.\n\nOkay.  I maintain that the latter is way more readable.  Is there any\nadvantage to the former?\n\nWhichever the project chooses can be enshrined in\nDocumentation/CodingGuidelines, of course.\n"},{"id":"149463","messageId":"7vaao2br58.fsf@alter.siamese.dyndns.org","threadId":"24910","inReplyTo":"20100830155723.GC3292@burratino","subject":"Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-31T17:58:43Z","receivedAt":"2010-08-31T17:58:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> +++ b/t/test-lib.sh\n>> @@ -687,14 +687,12 @@ test_when_finished () {\n>>  test_create_repo () {\n>>  \ttest \"$#\" = 1 ||\n>>  \terror \"bug in the test script: not 1 parameter to test-create-repo\"\n>> -\towd=`pwd`\n>>  \trepo=\"$1\"\n>>  \tmkdir -p \"$repo\"\n>> -\tcd \"$repo\" || error \"Cannot setup test environment\"\n>> +\t(cd \"$repo\" || error \"Cannot setup test environment\"\n>>  \t\"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n>>  \terror \"cannot run git init -- have you built things yet?\"\n>> -\tmv .git/hooks .git/hooks-disabled\n>> -\tcd \"$owd\"\n>> +\tmv .git/hooks .git/hooks-disabled)\n>\n> Style: why not use\n>\n> \t(\n> \t\tcd \"$repo\" ...\n> \t\t... .git/hooks-disabled\n> \t)\n\nThat is a sensible suggestion, but what does \"error\" do?\n\nI think you would need || exit at the end, i.e.\n\n\t(\n        \t... || error \"...\"\n\t) || exit\n\nor something.\n"},{"id":"149468","messageId":"AANLkTim7KWTR0FhnucZ7ROujZ-c7=y80Wz7UnR=+hsgZ@mail.gmail.com","threadId":"24910","inReplyTo":"7vaao2br58.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-31T18:11:30Z","receivedAt":"2010-08-31T18:11:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Aug 31, 2010 at 17:58, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>> Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> +++ b/t/test-lib.sh\n>>> @@ -687,14 +687,12 @@ test_when_finished () {\n>>>  test_create_repo () {\n>>>      test \"$#\" = 1 ||\n>>>      error \"bug in the test script: not 1 parameter to test-create-repo\"\n>>> -    owd=`pwd`\n>>>      repo=\"$1\"\n>>>      mkdir -p \"$repo\"\n>>> -    cd \"$repo\" || error \"Cannot setup test environment\"\n>>> +    (cd \"$repo\" || error \"Cannot setup test environment\"\n>>>      \"$GIT_EXEC_PATH/git-init\" \"--template=$GIT_BUILD_DIR/templates/blt/\" >&3 2>&4 ||\n>>>      error \"cannot run git init -- have you built things yet?\"\n>>> -    mv .git/hooks .git/hooks-disabled\n>>> -    cd \"$owd\"\n>>> +    mv .git/hooks .git/hooks-disabled)\n>>\n>> Style: why not use\n>>\n>>       (\n>>               cd \"$repo\" ...\n>>               ... .git/hooks-disabled\n>>       )\n>\n> That is a sensible suggestion, but what does \"error\" do?\n>\n> I think you would need || exit at the end, i.e.\n>\n>        (\n>                ... || error \"...\"\n>        ) || exit\n>\n> or something.\n\nGood catch, yes. error() calls exit so this patch as-is introduces a\nlogic error.\n"}]}