Re: [PATCH] test-lib-functions: simplify `test_file_not_empty` failure message

[Junio C Hamano <gitster@pobox.com>]

Dirk Gouders <dirk@gouders.net> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Fri, Mar 1, 2024 at 5:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <ericsunshine@charter.net> writes:
>>> > A more accurate message might be "'foo' is empty but
>>> > should not be (or doesn't exist)", but that's unnecessarily long-winded
>>> > and adds little information that the test author couldn't discover by
>>> > noticing the file's absence.
>>>
>>> The "adds little information" version may be
>>>
>>>                 echo "'$1' is either missing or empty, but should not be"
>>> ...
>> I find "'$1' is either missing or empty, but should not be" suggestion
>> clear and easily understood. I'll reroll with that.
>
> This is a view from a position with more distance:
>
> I find that not so easily understood -- the "but should not
> be" part is rather unexpected and I feel, it doesn't provide necessary
> information, e.g.:
>
> test_path_is_executable () {
> ...
> 		echo "$1 is not executable"
> ...
>
> also doesn't state what is wanted and I doubt that message doesn't
> clearly describe the problem.

I cannot tell if you really meant the double negative involving
"doubt", but assuming you did, you are saying that

    With "X is not Y", it is clear enough that we expect X to be Y
    (if it were not clear to somebody who read "X is not Y" that we
    want X to be Y, then "X is not Y, but it should be" may needed,
    but "X is not Y" is clear enough).

So you think "$1 is either missing or empty" is better without "but
should not be" added to the end?  Am I reading you correctly?

I think this takes us back pretty much to square one ;-) but that is
also fine.

But the above argument depends on an untold assumption.  The message
"X is not Y" must be clearly understood as a complaint, not a mere
statement of a fact.  I am not sure if that is the case.

Instead of "X is not Y, but it should be", the way to clarify these
messages may be to say "error: X is not Y", perhaps?

> While I looked at it: there is another `test -s` in test_grep () that
> perhaps could be fixed the same way:
>
> 	if test -s "$last_arg"
> 	then
> 		cat >&4 "$last_arg"
> 	else
> 		echo >&4 "<File '$last_arg' is empty>"
> 	fi

If you are worried about "test -s" failing because "$last_arg" does
not exist, then you are worried too much.  We upfront guard the
test_grep() helper with "test -f" of the same file and diagnoses the
lack of the file as a bug in the test.  And we do not assume gremlins
removing random files while we are running tests.