Re: [GSoc][PATCH v3 1/3] test functions: add function `test_file_not_empty`

From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Date: Mar 5, 2019, 13:27 UTC
Message-ID: <20190305132701.9657-1-rohit.ashiwal265@gmail.com>
In-Reply-To: <CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com>

Hello Eric

On 2019-03-04 19:17:50 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 12 quoted lines

> On Mon, Mar 4, 2019 at 7:08 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> > if ! test -s "$1"
> > then
> > 	echo "'$1' is not a non-empty file."
>
> Although not incorrect, the double-negative is hard to digest. I had
> to read it a few times to convince myself that it matched the intent
> of the new function. I wonder if a message such as
>
>    echo "'$1' is unexpectedly empty"
>
> would be better. (Subjective, and not at all worth a re-roll.)

I think the current message is more accurate as it implies both:
	1. There is no file, and
	2. If there is, it is not empty

"unexpectedly empty" may imply that there is a directory which is not empty and that is not the intention of the function.

Show 5 quoted lines

> Also, it might be a good idea to add this new function as a neighbor
> of test_must_be_empty() rather than defining it a couple hundred lines
> earlier in the file. Alternately, perhaps a preparatory patch could
> move test_must_be_empty() closer to the other similar functions
> (test_path_is_missing() and cousins).

I think we should relocate the function `test_must_be_empty` in a separate patch as this patch deals with a different issue.

Thanks Rohit