Re: [PATCH] replace test -f with test_path_is_file

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2021, 18:04 UTC
Message-ID: <xmqqr1kbm34j.fsf@gitster.g>
In-Reply-To: <pull.982.git.git.1616147527082.gitgitgadget@gmail.com>

"Krushnal Patel via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: krush11 <krushnalpatel11@gmail.com>
>
> 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.