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

Hey Eric,

On Wed, Jun 15, 2016 at 11:52 PM, Eric Sunshine [off-list ref] wrote:

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva [off-list ref] wrote:

quoted

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.

Sure I can include that.

quoted

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

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

quoted

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

This is purposely as I want to keep this function to return only
boolean values ( 0 or 1).

quoted

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

I didn't realize the complexity the patch carried with itself before.
I probably shouldn't fidget with am code right now, its a work better
left to who are converting the code to library code. I think its the
best fit for this situation to leave it as die()'ing.

Regards,
Pranit Bauva