Re: [PATCH 1/2] archive: replace write_or_die() calls with write_block_or_die()

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2019, 23:44 UTC
Message-ID: <xmqqd0l8tjph.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1904261028220.45@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 18 quoted lines

>> >> +/* 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.