Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

From: Mike Rappazzo <rappazzo@gmail.com>
Date: May 7, 2016, 13:35 UTC
Message-ID: <CANoM8SVr1_G6KevbGSHifGyQS-ei57q+5D+GE_QmKvf_ysF2Sg@mail.gmail.com>
In-Reply-To: <xmqqy47m25z4.fsf@gitster.mtv.corp.google.com>

On Fri, May 6, 2016 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>> t1500-rev-parse contains envrionment leaks (changing dir without
>> changing back, setting config variables, etc).  Add a test to clean this
>> up up so that future tests can be added without worry of any setting
>> from a previous test.
>
> This is a wonderful thing to do, but...
>
>>  test_rev_parse toplevel false false true '' .git
>>
>> @@ -84,4 +85,41 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
>>  git config --unset core.bare
>>  test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>>
>> +test_expect_success 'cleanup from the previous tests' '
>> +     cd .. &&
>> +     rm -r work &&
>
> Instead of cleaning things up like this, could you please please
> please fix these existing tests that chdir around without being in a
> subshell?  If the "previous tests" failed before going down as this
> step expects, the "cd .. && rm -r" can make things worse.

I still have fixing this test up on my do-to list. My previous attempt[1] had some flaws in addition to some objection to the approach I took to expand each test. Eric Sunshine suggested using a table approach, but I am not sure if that can be done cleanly.

I figured that a fix to the rev-parse code would supersede test cleanup, so I separated my efforts.

I originally copied the pattern from above this code:

> +#cleanup from the above
> +cd ..
> +rm -r work
> +mv repo.git .git || exit 1

but Gábor had an objection to it [2].  So I went with this simple cleanup test.

I could move it back to outside of a test, and do some checks around it. Something like:

    dir=$(pwd)
    target=${dir##*/}
    if [ "$target" == "work" ]
    then
        cd ..
        rm -r "work"
    fi

Show 16 quoted lines

>
>> +     mv repo.git .git &&
>> +     unset GIT_DIR &&
>> +     unset GIT_CONFIG &&
>
> The spirit of this change is to make the test more independent from
> the effects of what happened previously.  Use sane_unset so that
> we do not have to worry about previous step that may have failed
> before it has a chance to set GIT_DIR and GIT_CONFIG (which would
> cause these unset to fail).
>
>> +     git config core.bare $original_core_bare
>
> Is this (rather, the capturing of $original_core_bare up above)
> necessary?  We are in the default 'trash' repository when the
> capturing happens, and we know it is not a bare repository, right?

My goal was to have the test be in the state exactly as it was at the beginning of the test. Right above my cleanup test this line is executed:

> git config --unset core.bare

I just wanted to be absolutely sure that the value was the same. I could certainly simplify it to assume core.bare is "true" though.

Thanks, _Mike

[1] http://thread.gmane.org/gmane.comp.version-control.git/291729 [2] http://article.gmane.org/gmane.comp.version-control.git/293003