Hi Junio,
On Sat, 27 Apr 2019, Junio C Hamano wrote:
> 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.
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.
> 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.
