Hey Eric,
On Wed, Jun 15, 2016 at 11:52 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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.
Sure I can include that.
>> 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@xxxxxxxxx>
>> ---
>> 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...
This is purposely as I want to keep this function to return only
boolean values ( 0 or 1).
>> + }
>> +
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html