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

[Eric Sunshine <sunshine@sunshineco.com>]

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.