Re: [PATCH 2/2] archive: avoid spawning `gzip`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 2, 2019, 20:20 UTC
Message-ID: <87ftpwip5m.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1904261051310.45@tvgsbejvaqbjf.bet>

On Fri, Apr 26 2019, Johannes Schindelin wrote:

Show 32 quoted lines

> Hi brian,
>
> On Sat, 13 Apr 2019, brian m. carlson wrote:
>
>> On Fri, Apr 12, 2019 at 09:51:02PM -0400, Jeff King wrote:
>> > I wondered how you were going to kick this in, since users can define
>> > arbitrary filters. I think it's kind of neat to automagically convert
>> > "gzip -cn" (which also happens to be the default). But I think we should
>> > mention that in the Documentation, in case somebody tries to use a
>> > custom version of gzip and wonders why it isn't kicking in.
>> >
>> > Likewise, it might make sense in the tests to put a poison gzip in the
>> > $PATH so that we can be sure we're using our internal code, and not just
>> > calling out to gzip (on platforms that have it, of course).
>> >
>> > The alternative is that we could use a special token like ":zlib" or
>> > something to indicate that the internal implementation should be used
>> > (and then tweak the baked-in default, too). That might be less
>> > surprising for users, but most people would still get the benefit since
>> > they'd be using the default config.
>>
>> I agree that a special value (or NULL, if that's possible) would be
>> nicer here. That way, if someone does specify a custom gzip, we honor
>> it, and it serves to document the code better. For example, if someone
>> symlinked pigz to gzip and used "gzip -cn", then they might not get the
>> parallelization benefits they expected.
>
> I went with `:zlib`. The `NULL` value would not really work, as there is
> no way to specify that via `archive.tgz.command`.
>
> About the symlinked thing: I do not really want to care to support such
> hacks.

It's the standard way by which a lot of systems do this, e.g. on my Debian box:

    $ find /{,s}bin /usr/{,s}bin -type l -exec file {} \;|grep /etc/alternatives|wc -l
    108

To write this E-Mail I'm invoking one such symlink :)

> If you want a different compressor than the default (which can
> change), you should specify it specifically.

You might want to do so system-wide, or for each program at a time.

I don't care about this for gzip myself, just pointing out it *is* a thing people use.

Show 8 quoted lines

>> I'm fine overall with the idea of bringing the compression into the
>> binary using zlib, provided that we preserve the "-n" behavior
>> (producing reproducible archives).
>
> Thanks for voicing this concern. I had a look at zlib's source code, and
> it looks like it requires an extra function call (that we don't call) to
> make the resulting file non-reproducible. In other words, it has the
> opposite default behavior from `gzip`.

Just commenting on the overall thread: I like René's "new built-in" patch best.

You mentioned "new command that we have to support for eternity". I think calling it "git gzip" is a bad idea. We'd make it "git archive--gzip" or "git archive--helper", and we could hide building it behind some compat flag.

Then we'd carry no if/else internal/external code, and the portability issue that started this would be addressed, no?

As a bonus we could also drop the "GZIP" prereq from the test suite entirely and just put that "gzip" in $PATH for the purposes of the tests.

I spied on your yet-to-be-submitted patches and you could drop GZIP from the "git archive" tests, but we'd still need it in t/t5562-http-backend-content-length.sh, but not if we had a "gzip" compat helper.

There's also a long-standing bug/misfeature in git-archive that I wonder about: When you combine --format with --remote you can only generate e.g. tar.gz if the remote is OK with it, if it says no you can't even if it supports "tar" and you could do the "gz" part locally. Would such a patch be harder with :zlib than if we always just spewed out to external "gzip" after satisfying some criteria?