Re: [PATCH v3 5/5] archive-tar: use internal gzip by default

["René Scharfe" <l.s.r@web.de>]

Am 14.06.22 um 18:29 schrieb Johannes Schindelin:
> Hi René,
>
> On Tue, 14 Jun 2022, René Scharfe wrote:
>
>> Am 14.06.22 um 13:27 schrieb Johannes Schindelin:
>>
>>> The solution is to move the heap variable back into a scope that matches
>>> the lifetime of the compression:
>>>
>>> -- snip --
>>> diff --git a/archive-tar.c b/archive-tar.c
>>> index 60669eb7b9c..3d77e0f7509 100644
>>> --- a/archive-tar.c
>>> +++ b/archive-tar.c
>>> @@ -460,17 +460,12 @@ static void tgz_write_block(const void *data)
>>>
>>>  static const char internal_gzip_command[] = "git archive gzip";
>>>
>>> -static void tgz_set_os(git_zstream *strm, int os)
>>> -{
>>> -#if ZLIB_VERNUM >= 0x1221
>>> -	struct gz_header_s gzhead = { .os = os };
>>> -	deflateSetHeader(&strm->z, &gzhead);
>>> -#endif
>>> -}
>>> -
>>>  static int write_tar_filter_archive(const struct archiver *ar,
>>>  				    struct archiver_args *args)
>>>  {
>>> +#if ZLIB_VERNUM >= 0x1221
>>> +	struct gz_header_s gzhead = { .os = 3 }; /* Unix, for reproducibility */
>>> +#endif
>>>  	struct strbuf cmd = STRBUF_INIT;
>>>  	struct child_process filter = CHILD_PROCESS_INIT;
>>>  	int r;
>>> @@ -481,7 +476,10 @@ static int write_tar_filter_archive(const struct archiver *ar,
>>>  	if (!strcmp(ar->filter_command, internal_gzip_command)) {
>>>  		write_block = tgz_write_block;
>>>  		git_deflate_init_gzip(&gzstream, args->compression_level);
>>> -		tgz_set_os(&gzstream, 3); /* Unix, for reproducibility */
>>> +#if ZLIB_VERNUM >= 0x1221
>>> +		if (deflateSetHeader(&gzstream.z, &gzhead) != Z_OK)
>>> +			BUG("deflateSetHeader() called too late");
>>> +#endif
>>>  		gzstream.next_out = outbuf;
>>>  		gzstream.avail_out = sizeof(outbuf);
>>>
>>> -- snap --
>>
>> Good find, thank you!  A shorter solution would be to make gzhead static.
>
> I should have said that I had considered this, but decided against it
> because it would introduce yet another issue: it would render the code
> needlessly un-multi-threadable. And that can be avoided _really_ easily.

archive-tar.c (and archive-zip.c) use other static variables, so a
static gzhead won't break or block anything in this regard.  There was
no interest in running it in parallel threads so far AFAIK, and it's
hard for me to imagine the usefulness of creating multiple .tgz files at
the same time.

The doubled ZLIB_VERNUM is unsightly and I'm not sure the BUG check is
useful -- I omitted error checking because there is no recurse for us if
deflateSetHeader() doesn't work, and on ancient zlib versions we
silently continue anyway.

But that's all just minor quibbling -- I'll include your changes in the
next version, they look fine overall.

René