# [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old 6 messages from 2010-08-30 to 2010-08-31. Participants: Ævar Arnfjörð Bjarmason, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/24910 ## Ævar Arnfjörð Bjarmason, 2010-08-30 12:00 Subject: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: <1283169642-21917-1-git-send-email-avarab@gmail.com> URL: https://gitlist.dev/e/1283169642-21917-1-git-send-email-avarab%40gmail.com ``` Change the test_create_repo code added in v1.2.2~6 to use a subshell instead of keeping track of the old working directory and cd-ing back when it's done. Signed-off-by: Ævar Arnfjörð Bjarmason --- t/test-lib.sh | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index dc934b8..778ba8d 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -687,14 +687,12 @@ test_when_finished () { test_create_repo () { test "$#" = 1 || error "bug in the test script: not 1 parameter to test-create-repo" - owd=`pwd` repo="$1" mkdir -p "$repo" - cd "$repo" || error "Cannot setup test environment" + (cd "$repo" || error "Cannot setup test environment" "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || error "cannot run git init -- have you built things yet?" - mv .git/hooks .git/hooks-disabled - cd "$owd" + mv .git/hooks .git/hooks-disabled) } test_done () { -- 1.7.2.2.518.gf0ba8 ``` ## Jonathan Nieder, 2010-08-30 15:57 Subject: Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: <20100830155723.GC3292@burratino> URL: https://gitlist.dev/e/20100830155723.GC3292%40burratino In-Reply-To: <1283169642-21917-1-git-send-email-avarab@gmail.com> ``` Ævar Arnfjörð Bjarmason wrote: > +++ b/t/test-lib.sh > @@ -687,14 +687,12 @@ test_when_finished () { > test_create_repo () { > test "$#" = 1 || > error "bug in the test script: not 1 parameter to test-create-repo" > - owd=`pwd` > repo="$1" > mkdir -p "$repo" > - cd "$repo" || error "Cannot setup test environment" > + (cd "$repo" || error "Cannot setup test environment" > "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || > error "cannot run git init -- have you built things yet?" > - mv .git/hooks .git/hooks-disabled > - cd "$owd" > + mv .git/hooks .git/hooks-disabled) Style: why not use ( cd "$repo" ... ... .git/hooks-disabled ) ? ``` ## Ævar Arnfjörð Bjarmason, 2010-08-30 16:01 Subject: Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: URL: https://gitlist.dev/e/AANLkTi%3Dz9uUB_YZ9Sb0O8JzaEjazKDzqB%3DZVtqTTDf2K%40mail.gmail.com In-Reply-To: <20100830155723.GC3292@burratino> ``` On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder wrote: > Ævar Arnfjörð Bjarmason wrote: > >> +++ b/t/test-lib.sh >> @@ -687,14 +687,12 @@ test_when_finished () { >>  test_create_repo () { >>       test "$#" = 1 || >>       error "bug in the test script: not 1 parameter to test-create-repo" >> -     owd=`pwd` >>       repo="$1" >>       mkdir -p "$repo" >> -     cd "$repo" || error "Cannot setup test environment" >> +     (cd "$repo" || error "Cannot setup test environment" >>       "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || >>       error "cannot run git init -- have you built things yet?" >> -     mv .git/hooks .git/hooks-disabled >> -     cd "$owd" >> +     mv .git/hooks .git/hooks-disabled) > > Style: why not use > >        ( >                cd "$repo" ... >                ... .git/hooks-disabled >        ) > I've seen both used and I don't know which is preferred. ``` ## Jonathan Nieder, 2010-08-30 16:04 Subject: Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: <20100830160444.GE3292@burratino> URL: https://gitlist.dev/e/20100830160444.GE3292%40burratino In-Reply-To: ``` Ævar Arnfjörð Bjarmason wrote: > On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder wrote: >> Ævar Arnfjörð Bjarmason wrote: >>> -     cd "$repo" || error "Cannot setup test environment" >>> +     (cd "$repo" || error "Cannot setup test environment" >>>       "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || >>>       error "cannot run git init -- have you built things yet?" >>> -     mv .git/hooks .git/hooks-disabled >>> -     cd "$owd" >>> +     mv .git/hooks .git/hooks-disabled) >> >> Style: why not use >> >>        ( >>                cd "$repo" ... >>                ... .git/hooks-disabled >>        ) >> > > I've seen both used and I don't know which is preferred. Okay. I maintain that the latter is way more readable. Is there any advantage to the former? Whichever the project chooses can be enshrined in Documentation/CodingGuidelines, of course. ``` ## Junio C Hamano, 2010-08-31 17:58 Subject: Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: <7vaao2br58.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vaao2br58.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100830155723.GC3292@burratino> ``` Jonathan Nieder writes: > Ævar Arnfjörð Bjarmason wrote: > >> +++ b/t/test-lib.sh >> @@ -687,14 +687,12 @@ test_when_finished () { >> test_create_repo () { >> test "$#" = 1 || >> error "bug in the test script: not 1 parameter to test-create-repo" >> - owd=`pwd` >> repo="$1" >> mkdir -p "$repo" >> - cd "$repo" || error "Cannot setup test environment" >> + (cd "$repo" || error "Cannot setup test environment" >> "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || >> error "cannot run git init -- have you built things yet?" >> - mv .git/hooks .git/hooks-disabled >> - cd "$owd" >> + mv .git/hooks .git/hooks-disabled) > > Style: why not use > > ( > cd "$repo" ... > ... .git/hooks-disabled > ) That is a sensible suggestion, but what does "error" do? I think you would need || exit at the end, i.e. ( ... || error "..." ) || exit or something. ``` ## Ævar Arnfjörð Bjarmason, 2010-08-31 18:11 Subject: Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old Message-ID: URL: https://gitlist.dev/e/AANLkTim7KWTR0FhnucZ7ROujZ-c7%3Dy80Wz7UnR%3D%2BhsgZ%40mail.gmail.com In-Reply-To: <7vaao2br58.fsf@alter.siamese.dyndns.org> ``` On Tue, Aug 31, 2010 at 17:58, Junio C Hamano wrote: > Jonathan Nieder writes: > >> Ævar Arnfjörð Bjarmason wrote: >> >>> +++ b/t/test-lib.sh >>> @@ -687,14 +687,12 @@ test_when_finished () { >>>  test_create_repo () { >>>      test "$#" = 1 || >>>      error "bug in the test script: not 1 parameter to test-create-repo" >>> -    owd=`pwd` >>>      repo="$1" >>>      mkdir -p "$repo" >>> -    cd "$repo" || error "Cannot setup test environment" >>> +    (cd "$repo" || error "Cannot setup test environment" >>>      "$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 || >>>      error "cannot run git init -- have you built things yet?" >>> -    mv .git/hooks .git/hooks-disabled >>> -    cd "$owd" >>> +    mv .git/hooks .git/hooks-disabled) >> >> Style: why not use >> >>       ( >>               cd "$repo" ... >>               ... .git/hooks-disabled >>       ) > > That is a sensible suggestion, but what does "error" do? > > I think you would need || exit at the end, i.e. > >        ( >                ... || error "..." >        ) || exit > > or something. Good catch, yes. error() calls exit so this patch as-is introduces a logic error. ```