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

On Fri, May 6, 2016 at 6:10 PM, Junio C Hamano [off-list ref] wrote:

Michael Rappazzo [off-list ref] writes:

quoted

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

quoted

 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

quoted

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

quoted

+     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