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

From: Torsten Bögershausen <tboegi@web.de>
Date: Jun 12, 2016, 10:44 UTC
Message-ID: <3db60187-4b44-85b2-f714-f836e3c3bc6e@web.de>
In-Reply-To: <CAFZEwPMiyHqFa0+Z1EAfwG4APcCjJ+1qBeSHjyuOhc6QMb3hLA@mail.gmail.com>

Show 14 quoted lines

>> 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.
>

Please allow one or more late comments: If is_empty_file() does what you need, then it can be moved into wrapper.c and simply be re-used in your code.

If you want to introduce a new function, that can be used for other refactoring, then the whole thing would ideally go into a single commit, or into a single series. That may probably be out of the scope for your current efforts ?

What really makes me concern is the mixture of signed - and unsigned:
ssize_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return -1;
+       return xsize_t(st.st_size);
+}

To my understanding a file size is either 0, or a positive integer. Returning -1 is of course impossible with a positive integer.

So either the function is changed like this:

int file_size(const char *filename, size_t *len)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return -1;
+       *len = xsize_t(st.st_size);
+       return 0;
+}

Or, if that works for you:

size_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st) < 0)
+               return 0;
+       return xsize_t(st.st_size);
+}

Or, more git-ish:

size_t file_size(const char *filename)
+{
+       struct stat st;
+       if (stat(filename, &st))
+               return 0;
+       return xsize_t(st.st_size);
+}

(And then builtin/am.c  can be changed to use the new function.