Re: [PATCH] implemented strbuf_write_or_die()

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mar 1, 2014, 22:33 UTC
Message-ID: <53126051.7030904@alum.mit.edu>
In-Reply-To: <1393680567-2166-1-git-send-email-faiz.off93@gmail.com>

Please leave a little more time for people to give feedback between versions of a patch series (unless the first version was so broken that it would be pointless for any other reviewer to waste time on it. And please label the versions of a single patch series "[PATCH]" then "[PATCH v2]", "[PATCH v3]", etc.

I agree with Johannes's advice that the function is in the wrong place and has the wrong parameter order.

On 03/01/2014 02:29 PM, Faiz Kothari wrote:

Show 9 quoted lines

> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>
> ---
>>> -               write_or_die(1, rpc.result.buf, rpc.result.len);
>>> +               strbuf_write_or_die(1, &(rpc.result.buf));
> 
>> May be this should be
>> strbuf_write_or_die(1, &(rpc.result));
> 
> Yes, I changed that :-) Thanks again.

I find it alarming that either the compiler didn't emit warnings for the old version or that you ignored the compiler warnings. Git should compile without warnings even with with quite strict compiler settings; I use gcc with the following options

    -Wall -Werror \
        -Wdeclaration-after-statement \
        -Wno-format-zero-length \
        -Wno-format-security

Maybe you weren't including the header file that declares strbuf_write_or_die() in the file containing this invocation?

Also, the parentheses in "&(rpc.result)" are unnecessary.

And I think that some of the blank lines that you added contained invisible whitespace. Please check your whitespace! You can run "git diff --check" to detect some obvious whitespace problems.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/