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

[Mike Rappazzo <rappazzo@gmail.com>]

On Fri, May 6, 2016 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

>
>> +     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