Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite

> "Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
> >       shellpath=$(git var GIT_SHELL_PATH) &&
> >       case "$shellpath" in
> > -     [A-Z]:/*/sh.exe) test -f "$shellpath";;
> > +     [A-Z]:/*/sh.exe) test_path_is_file "$shellpath";;
> >       *) return 1;;
> >       esac
> >  '
>
> ... this one is iffy.  How well does it mesh with the "return 1"
> case in the same case/esac?  I do not use Windows, but if
> GIT_SHELL_PATH set by that system is not in a form that the platform
> expects (i.e. a drive letter and then path to a file "sh.exe"), it
> is just as grave a problem worth reporting as the path given is not
> a regular file, yet "return 1" case does not give any specific error
> message (instead it just lets the test fail), so it feels a bit
> uneven to make the "test -f" failure alone louder than it currently
> is.

> >  test_expect_success 'see if git pack-refs --prune remove ref files' '
> >       git branch f &&
> >       git pack-refs --all --prune &&
> > -     ! test -f .git/refs/heads/f
> > +     ! test_path_is_file .git/refs/heads/f
> >  '
>
> This conversion is wrong.  [...]
>
>     test_path_is_file () {
>             test "$#" -ne 1 && BUG "1 param"
>             if ! test -f "$1"
>             then
>                     echo "File $1 doesn't exist"
>                     false
>             fi
>     }
>
> The test wants to make sure that 'f' is not a file.  So you want to
> see a loud complaint when 'f' exists as a file.  Does it do so when
> you say
>
>         ! test_path_is_file .git/refs/heads/f
>
> in this test?  No, it will not enter the "then" block and silently
> succeed, and that return status is negated by that "!", so the test
> will notice that the expectation is not met, but you didn't achieve
> your goal of making it louder when it fail.  Worse, if the file is
> not there, as the test expects when Git is working correctly, your
>
>         ! test_path_is_file .git/refs/heads/f
>
> will enter the "then" block, complain that the file does not exist,
> returns a failure, and your "!" will turn it into a success.  The
> test passes, but the user is given an error message when the test is
> run with "-v" option.

> I'll stop here.  I think all "! test_path_foo" changes in this patch
> are wrong.
>
> Unlike "test_grep" that lets you write "test_grep ! foo bar" to make
> sure that file 'bar' has no 'foo' in it (and complains loudly if
> 'foo' appears in 'bar'), test_path_foo family of helper functions do
> not let you write "test_path_exists ! no-such-file" unfortunately.
> So my recommendation for a microproject sample is to just drop these
> negations from the changes and stop there.