Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jun 8, 2016 at 3:57 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> dir: introduce file_size() to check the size of file
>>>
>>> At times we require to see if the file is empty and get the size of the
>>> file. By using stat we can get the file size without actually having to
>>> open the file to check for its contents.
>>
>> The sole caller of this function in patch 4/4 does so only to check if
>> the file exists; it doesn't even care about the file's size, thus
>> neither this function nor this patch seem justified and probably ought
>> to be dropped unless some better and stronger justification can be
>> shown.
>
> Umm, actually there is a subtle difference between file_exists() and
> file_size(). file_exists() *only* checks whether the file exists or
> not while file_size() can also be used to check whether the file is
> empty or not also see the implementation of both of them which shows
> the difference. In fact it doesn't care at all about the file size.
> Now there are a lot of instances in shell scripts where there are
> quite some differences with -f and -s and some places *do care* about
> this subtle difference. For eg. in bisect_reset() we test whether the
> file .git/BISECT_START has some contents in it. But I guess I can add
> some more justification in the commit message. What do you think?

See my review of patch 4/4 which points out that C bisect_reset() does
*not* presently care about the file size, which is probably a bug
since that doesn't match the behavior of the shell code it's replacing
(which does care that the file is not empty).

I think this would be clearer if you instead added a function to
bisect--helper.c which operates at a semantically higher level than
what you have here (and drop this file_size() function). Specifically,
add a function which tells you precisely what you want to know:
whether the file exists and has non-zero size. For instance, in
bisect--helper.c:

    static int file_empty_or_missing(const char *path)
    {
        struct stat st;
        return stat(...) < 0 || st.st_size == 0;
    }

Or, if you have more cases where you want to know that it exists with
non-zero size, then name it file_non_empty() and reverse the sense of
the return value.