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

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Torsten,

On Wed, Jun 8, 2016 at 1:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 06/08/2016 09:57 AM, Pranit Bauva wrote:
>>
>> Hey Eric,
>>
>> 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?
>>
>>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>> ---
>>>> diff --git a/dir.c b/dir.c
>>>> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
>>>> +ssize_t file_size(const char *filename)
>>>> +{
>>>> +       struct stat st;
>>>> +       if (stat(filename, &st) < 0)
>>>> +               return -1;
>>>> +       return xsize_t(st.st_size);
>>>> +}
>>>> +
>>>> diff --git a/dir.h b/dir.h
>>>> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list
>>>> *el);
>>>> +/*
>>>> + * Return the size of the file `filename`. It returns -1 if error
>>>> + * occurred, 0 if file is empty and a positive number denoting the size
>>>> + * of the file.
>>>> + */
>>>> +extern ssize_t file_size(const char *);
>>
>>
> So what I understand, you want something like this:
>
> +ssize_t file_size_not_zero(const char *filename)
> +{
> +       struct stat st;
> +       if (stat(filename, &st) < 0)
> +               return -1;
> +       return !!st.st_size);
> +}

For the purpose of bisect_reset(), Yes. BTW a similar function exist
in builtin/am.c with the name is_empty_file(). But as Christian points
out file_size() could help to refactor other parts of code.

Regards,
Pranit Bauva