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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 29, 2019, 21:32 UTC
Message-ID: <nycvar.QRO.7.76.6.1904291732370.45@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqd0l8tjph.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Sat, 27 Apr 2019, Junio C Hamano wrote:

Show 29 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

I guess I can try to find some time next week to clean up those callers. But honestly, I do not really think that this cleanup falls squarely into the goal of this here patch series.

Show 7 quoted lines

> 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.

This argument would hold a lot more water if the following lines were not part of archive-tar.c:

	#define RECORDSIZE      (512)
	#define BLOCKSIZE       (RECORDSIZE * 20)

	static char block[BLOCKSIZE];

If you can tell me how the `-b` (run-time) parameter can affect the (compile-time) `BLOCKSIZE` constant, maybe I can start to understand your concern.

:-)

Ciao, Dscho

P.S.: I just looked, and I do not even see a `-b` option of `git archive`, so I suspect that you talked about the generic tar file format? I was not talking about each and every implementation of the tar file format here, I was talking about the tar file format that Git generates.