Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
>
> That is very much on purpose, as this code really is specific to the `tar`
> file format, which has a fixed, well-defined block size. It would make it
> easier to introduce a bug if that was a parameter.
I am not so sure for two reasons.
One is that its caller is full of BLOCKSIZE constants passed as
parameters (instead of calling a specialized function that hardcodes
the BLOCKSIZE without taking it as a parameter), and this being a
file-scope static, it does not really matter with respect to an
accidental bug of mistakenly changing BLOCKSIZE either in the caller
or callee.
Another is that I am not sure how your "fixed format" argument
meshes with the "-b blocksize" parameter to affect the tar/pax
output. The format may be fixed, but it is parameterized. If
we ever need to grow the ability to take "-b", having the knowledge
that our current code is limited to the fixed BLOCKSIZE in a single
function (i.e. the caller of this function , not the callee) would
be less error prone.
These two are in addition to the uniformity of the abstraction
concerns I raised in my original review comment.
So, sorry, I do not think your response makes much sense.
Thanks.
