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

[Dirk Gouders <dirk@gouders.net>]

Junio C Hamano <gitster@pobox.com> writes:

> 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

I'm sorry about that double negative which was probably wrong wording of
a non-native speaker.

>     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?

That is exactly what came to my mind when I was later re-thinking
what I had written.

>> 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.

Yes, thank you for clarification and sorry for the noise.

Dirk