threads / patch / 24910

patchtest-lib: use subshell instead of cd $new && .. && cd $old

Subject: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

## tl;dr

6 messages between Aug 30, 2010 and Aug 31, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Aug 30, 2010, 12:00 UTC · lore

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 <avarab@gmail.com>
---
 t/test-lib.sh |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
Show changes to t/test-lib.sh +2 −4
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· Aug 30, 2010, 15:57 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> +++ 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· Aug 30, 2010, 16:01 UTC · re: Jonathan Nieder · lore

Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 25 quoted lines
> Æ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· Aug 30, 2010, 16:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

Ævar Arnfjörð Bjarmason wrote:
> On Mon, Aug 30, 2010 at 15:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Ævar Arnfjörð Bjarmason wrote:
Show 17 quoted lines
>>> -     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· Aug 31, 2010, 17:58 UTC · re: Jonathan Nieder · lore

Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 24 quoted lines
> Æ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· Aug 31, 2010, 18:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] test-lib: use subshell instead of cd $new && .. && cd $old

On Tue, Aug 31, 2010 at 17:58, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> 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.

← back to recent threads