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

Hey Torsten,

On Sun, Jun 12, 2016 at 4:14 PM, Torsten Bögershausen [off-list ref] wrote:

quoted

quoted

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:

That's perfectly fine.

If is_empty_file() does what you need, then it can be moved into wrapper.c
and simply be re-used in your code.

Thanks for informing. I was unaware about the use of wrapper.c

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 ?

On re-thinking, I think introducing file_size() is out of the scope
for the current efforts and I will stick to is_empty_file(). Will move
it to wrapper.c and then use it in my code. I am not sure but I think
a few other parts could also use is_empty_file(). I will check on that
probably after GSoC as a cleanup.

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.

True.

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.

I think I will just skip file_size() for now.

Thanks for your comments!

Regards,
Pranit Bauva