Re: [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 5, 2019, 12:43 UTC
- Message-ID
- <xmqqmum9v6dx.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CAPig+cTTJgXERud0=svc5b+ctwQxoQ6cmpiA7WHMa7TUZ37BgQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
>> +test_file_not_empty () {
>> + 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.)Yeah, that is subjective. The expectation of the test is "not-empty", so I do not see this double-negation as being too bad, though.
Show 10 quoted lines
> Much later in this same file, a function named test_must_be_empty() is > defined, which is the complement of your new test_file_not_empty() > function. The dissimilar names may cause confusion, so choosing a name > more like the existing function might be warranted. > > 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).
Very good suggestions. Looking at neighbouring helpers around must-be-empty, it seems to me that the latter, i.e. moving it to sit next to other "path" helpers, would make the most sense.
Thanks.