Subject: Re: [PATCH] implemented strbuf_write_or_die()

From: Michael Haggerty <[email-masked]>
Date: Sat, 01 Mar 2014 23:33:53 +0100

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:
> Signed-off-by: Faiz Kothari <[email-masked]>
> ---
>>> -               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
[email-masked]
http://softwareswirl.blogspot.com/