Re: [PATCH] replace test -f with test_path_is_file
From: Junio C Hamano <hidden>
Date: 2021-03-19 18:05:11
"Krushnal Patel via GitGitGadget" [off-list ref] writes:
From: krush11 <redacted> Although has the same functionality as test_path_is_file(), in the case where test_path_is_file() fails, we get much better debugging information. Replace with test_path_is_file so that future developers will have a better experience debugging these test cases.
While this change is not wrong per-se, in the context of this test script, I think the original use of "test -f" is not quite right to begin with. These are all "even after running 'git clean', these paths should exist without getting removed by mistake", so the intent of these "test -f" invocations are actually "test -e". Similarly, the invocations of "test ! -f" we see (and there also is at least one "! test -d") mean to say "these paths should be gone as the result of running 'git clean'". If by some accident a directory exists at the path that is checked with "test ! -f" due to a bug in 'git clean', these tests will not catch such a bug, because a directory does not pass "test -f". So most likely these negative tests this patch does not convert are better off being spelled as "! test -e", too. It would be more appropriate to use test_path_exists and test_path_is_missing to replace these "must exist as a file" and "must not exist as a file". Thanks.