On Wed, Feb 19, 2020 at 10:10:29AM -0800, Junio C Hamano wrote:
> Han-Wen Nienhuys <hanwenn@xxxxxxxxx> writes:
>
> > On Wed, Feb 19, 2020 at 6:02 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> >
> >> > your checker is tripping over code imported from zlib. I added a /*
> >> > clang-format off */ comment to avoid reformatting this code. What do
> >> > you suggest?
> >>
> >> Use zlib from the system instead?
> >
> > uncompress2 is a 2016 addition to zlib. It doesn't pass on
> > gitgitgadget's CI, because it is using an older version of zlib.
>
> Ahh.
>
> It is OK (and indeed you're right that you cannot avoid it) to ship
> a reasonably new snapshot as a fallback in such a case, but it still
> is far more preferrable to avoid linking with the fallback snapshot
> copy when a working one is available on the system, especially for a
> widely used and established library like zlib, because we have one
> less thing to keep up-to-date with the security patches made to the
> upstream.
If this were substantial code, I'd agree. But this is really just a thin
wrapper around the usual loop zlib inflate() loop that we already do in
a dozen places in our code (e.g., unpack_loose_rest(), which even does
the "did we consume all the bytes" check that uncompress2 allows).
I'm not sure it is worth the mental energy of adding a Makefile knob to
use the zlib version if we can just open-code it ourselves (or provide
our own custom helper that we always use).
And if it _is_ worth taking the zlib version because we're concerned
that it may contain or gain bugfixes[1], then possibly we should
be using it in lots more places (though probably not everywhere, as we
do sometimes need the streaming behavior of the loop).
[1] I'll admit we've hit some subtleties with that loop in the past,
especially around corrupted or bogus inputs, but I think we've
shaken them all out these days.
-Peff
