Jeff King <p...@peff.net> writes:
>> +/* writes out the whole block, or dies if fails */
>> +static void write_block_or_die(const char *block) {
>> + if (gzip) {
>> + if (gzwrite(gzip, block, (unsigned) BLOCKSIZE) != BLOCKSIZE)
>> + die(_("gzwrite failed"));
>> + } else {
>> + write_or_die(1, block, BLOCKSIZE);
>> + }
>> +}
I agree everything you said you your two review messages.
One thing you did not mention but I found disturbing was that this
does not take size argument but hardcodes BLOCKSIZE. If the patch
were removing use of BLOCKSIZE in its callers (because everybody
uses the same constant), this would not have bothered me, but as the
caller passes BLOCKSIZE to all its callees except this one, I found
that the interface optimizes for a wrong thing (i.e. reducing
one-time pain of writing this single patch of having to repeat
BLOCKSIZE in all calls to this function). This function should be
updated to take the size_t and have its caller(s) pass BLOCKSIZE.
Thanks for a review, and thanks Rohit for starting to get rid of
external dependency on gzip binary.
