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

From: Dirk Gouders <dirk@gouders.net>
Date: Mar 2, 2024, 07:07 UTC
Message-ID: <ghr0gtglhd.fsf@gouders.net>
In-Reply-To: <CAPig+cSt4Q0e+DioFiy7yjBgn5+gWNqQOYc0eCAgdYrZUp8VBQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 19 quoted lines

> 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"
>>
>> And avoiding "X is Y, but should  be ~Y" construct, perhaps
>>
>>                 echo "'$1' should be a file with non-empty contents"
>>
>> would work better?  I dunno.
>
> 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.

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

Dirk