Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> is_empty_file() can help to refactor a lot of code. Also it is quite
>> helpful while converting shell scripts which use `test -s`. Since
>
> As justification, "can help to refactor a lot of code" is very
> nebulous. It would be better to give a concrete reason for moving the
> function, such as explaining that the functionality will be needed by
> the "git bisect" port to C.

>> is_empty_file() is now a "library" function, its inappropriate to die() so
>> instead error_errno() is used to convey the message to stderr while the
>> appropriate boolean value is returned.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/am.c b/builtin/am.c
>> @@ -30,22 +30,6 @@
>>  /**
>> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
>> - */
>> -static int is_empty_file(const char *filename)
>> -{
>> -       struct stat st;
>> -
>> -       if (stat(filename, &st) < 0) {
>> -               if (errno == ENOENT)
>> -                       return 1;
>> -               die_errno(_("could not stat %s"), filename);
>> -       }
>> -
>> -       return !st.st_size;
>> -}
>
> So, the original function die()'d for unexpected errors, but the
> rewrite does not. This is a big behavior change. To account for such a
> change in behavior I'd expect to see git-am updated to die() on its
> own for such failures, but no such changes are present in this patch.
> More about this below...
>
>> diff --git a/wrapper.c b/wrapper.c
>> @@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
>> +int is_empty_file(const char *filename)
>> +{
>> +       struct stat st;
>> +
>> +       if (stat(filename, &st) < 0) {
>> +               if (errno == ENOENT)
>> +                       return 1;
>> +               error_errno(_("could not stat %s"), filename);
>
> Mental note: There is no 'return' in front of error_errno(), so the
> function does not exit here...

>> +       }
>> +
>> +       return !st.st_size;
>> +}
>
> If stat() returns some error other than ENOENT, then the value of 'st'
> will be undefined, yet this return statement accesses its 'st_size'
> field, which is clearly a bad thing to do.
>
> You either need to return a designated value (such as -1) upon errors
> other than ENOENT (and update the documentation to mention -1) so that
> the caller can decided what to do, or die() as the original did. While
> it's true that die()'ing is not necessarily friendly in library code,
> it may be acceptable until such time that you find a caller which
> needs different behavior.