On Tue, Mar 18, 2025 at 5:14 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> "Sampriyo Guin via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
Thanks for stating what I was thinking upon seeing this change.
> > 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.
And, again, you've said everything I was going to say, thus saving me
the effort of doing so.
Referring to the other thread at [*], perhaps this (avoiding `!` in
front of test_path_*) is yet another clarification which ought to be
added to the microproject description in order to lead candidates in a
more profitable direction.
[*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@xxxxxxxxxxxxxx/
> 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.
One other recommendation I would make is to restrict the microproject
submission to just a single test script (rather than updating twelve
of them) in order to avoid exhausting the pool for other potential
candidates.
