Re: [PATCH v2] t7611: replace test -f with test_path_is* helpers

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2024, 14:16 UTC
Message-ID: <xmqqcyhmh22q.fsf@gitster.g>
In-Reply-To: <20241220130632.11826-1-meetsoni3017@gmail.com>

Meet Soni <meetsoni3017@gmail.com> writes:

> Replace `test -f` and `test ! -f` with `test_path_is_file` and
> `test_path_is_missing` for better verbosity.

OK.  "verbosity" -> "debuggability" perhaps, but that is minor.

> While `test -f` ensures that the file exists and is a regular file,
> `test_path_is_file` provides clearer error messages on failure.

Correct.

> Similarly,
> `test ! -f`, used to check either the absence of a regular file or the
> presence of a directory, has been replaced with `test_path_is_missing` for
> better readability and consistent handling of such cases.

This is misleading. If you rewrite "test ! -f" that intends to be happy when it sees a directory, you would be changing the meaning of the program if you replace it with test_path_is_missing. Rather, when you see "test ! -f foo", you should first think if the intent of the test script there is to allow presence of "foo" that is not a file, and if that is not the case, in other words, the "test ! -f" you see should have been written "test ! -e" (i.e. "I do not want to see 'foo' there on the filesystem, no matter what kind of filesystem entity it is!"), it would give us the same meaning with better debuggability to rewrite such "test ! -f" with test_path_is_missing.

IOW, unlike "test -f foo" that can pretty much blindly replaceable with "test_path_is_file foo" without thinking at all, you must be a bit more careful. And _if_ you did such a more careful analysis before replacing "test ! -f", then you should restate the above, perhaps something along the lines of ...

    On the other hand, `test ! -f` literally means there should not
    be a file (implication is that we are OK if there is a directory
    or device nodes or other things at the given path).  But by
    looking at each of these in the test individually, many of them
    should rather have said "test ! -e", i.e. "there shouldn't be
    anything at the given path on the filesystem".  Rewrite these
    cases to test_path_is_missing for better debuggability.

I didn't check myself if all of the "test ! -f" you touched are indeed the original should have said "test ! -e". Hopefully you have done it yourself?

Thanks.