RE: [PATCH] cache.h: auto-detect if zlib has uncompress2()

[<rsbecker@nexbridge.com>]

On January 17, 2022 1:28 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > diff --git a/Makefile b/Makefile
> > index 5580859afdb..3e90820bbfd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -256,7 +256,10 @@ all::
> >  #
> >  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
> >  #
> > -# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
> > +# Define NO_UNCOMPRESS2 if your zlib is older than v1.2.9 and does
> > +not # have uncompress2. You should not need to define this unless
> > +your # zlib's ZLIB_VERNUM is broken. We'll auto-detect this on the
> > +basis of # that macro.
> 
> If their ZLIB_VERNUM is broken, and their library lacks
> uncompress2() but ZLIB_VERNUM makes us think they do, then thanks to
> 
> > +#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
> 
> in <cache.h>, we would use our own, and everybody is happy.
> 
> If their library does have uncompress2() but ZLIB_VERNUM makes us think they
> don't, there is no way to correct for that, is there?
> 
> With or without NO_UNCOMPRESS2 defined, ZLIB_VERNUM that is broken will
> lead us to redefine and override uncompress2().  Which is not the end of the
> world as the linker should know what to do, but then what we'd end up with is a
> more brittle alternative to always using our own replacement, which is not
> satisfactory, either.
> 
> > @@ -1728,7 +1731,6 @@ endif
> >
> >  ifdef NO_UNCOMPRESS2
> >  	BASIC_CFLAGS += -DNO_UNCOMPRESS2
> > -	REFTABLE_OBJS += compat/zlib-uncompress2.o
> 
> I very much like this; with or without auto detection, this "compat"
> thinkg should have been in LIB_OBJS from day one---there is no reason to expect
> reftable will remain to be the only user of the
> uncompress2() function (it should be part of COMPAT_OBJS).
> 
> > diff --git a/cache.h b/cache.h
> > index 281f00ab1b1..02b355fcf08 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -29,6 +29,11 @@ typedef struct git_zstream {
> >  	unsigned char *next_out;
> >  } git_zstream;
> >
> > +#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290 #define
> > +GIT_NO_UNCOMPRESS2 1 int uncompress2(Bytef *dest, uLongf *destLen,
> > +const Bytef *source,
> > +		uLong *sourceLen);
> > +#endif
> 
> I mentioned the other side of the coin already.  <git-compat-util.h> would be a
> better home for this change.  I do not think <cache.h> is the right place to do this
> (notice the zstream things around this area are all about _our_ invention and
> _our_ use pattern).
> 
> >  void git_inflate_init(git_zstream *);  void
> > git_inflate_init_gzip_only(git_zstream *);  void
> > git_inflate_end(git_zstream *);
> 
> 
> > diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
> > index 722610b9718..915796e85ac 100644
> > --- a/compat/zlib-uncompress2.c
> > +++ b/compat/zlib-uncompress2.c
> > @@ -8,15 +8,12 @@
> >
> >  */
> >
> > -#include "../reftable/system.h"
> 
> Again, I very much in favor of this lossage, as compat/ that depends on reftable
> feels backwards.
> 
> > -#define z_const
> 
> OK, the reason why we lose this is because we do not include the <zlib.h> header
> from here [*].
> 
> 	Side note: it is a way to tell <zlib.h> that whenever they
> 	say z_const (because they know the thing is not modified),
> 	we do not want to use "const".
> 
> >  /*
> >   * Copyright (C) 1995-2003, 2010, 2014, 2016 Jean-loup Gailly, Mark Adler
> >   * For conditions of distribution and use, see copyright notice in zlib.h
> >   */
> >
> > -#include <zlib.h>
> > +/* No "#include <zlib.h>", done in cache.h */
> 
> So, if we lose this include, we should lose #define z_const", too.
> 
> > diff --git a/config.mak.uname b/config.mak.uname
> 
> Losing these, and more importantly, not having to worry about maintaining, is
> refreshing ;-)
> 
> > diff --git a/reftable/block.c b/reftable/block.c index
> > 855e3f5c947..946edd0f34e 100644
> > --- a/reftable/block.c
> > +++ b/reftable/block.c
> > @@ -13,7 +13,7 @@ license that can be found in the LICENSE file or at
> > #include "record.h"
> >  #include "reftable-error.h"
> >  #include "system.h"
> > -#include <zlib.h>
> > +#include "zlib.h"
> 
> I do not think this is warranted.
> 
> Isn't it that using "<header.h>" with "-I<location>" a more language-laywers kosher
> way to include even our headers than using double-quotes (which is
> implementation-defined) anyway?  Going the other direction needs to be
> seriously justified.
> 
> If the upstream author wants to differentiate <a-system-header.h> and "our-
> header.h", let them do so [*].
> 
> 	Side note: even though my personal preference is to use <>
> 	consistently, that is not project preference.  And use of ""
> 	over <> is even less.
> 
> > diff --git a/reftable/system.h b/reftable/system.h index
> > 4907306c0c5..2cebbc94d4d 100644
> > --- a/reftable/system.h
> > +++ b/reftable/system.h
> > @@ -15,17 +15,7 @@ license that can be found in the LICENSE file or at
> > #include "strbuf.h"
> >  #include "hash.h" /* hash ID, sizes.*/  #include "dir.h" /*
> > remove_dir_recursively, for tests.*/
> > -
> > -#include <zlib.h>
> > -
> > -#ifdef NO_UNCOMPRESS2
> > -/*
> > - * This is uncompress2, which is only available in zlib >= 1.2.9
> > - * (released as of early 2017)
> > - */
> > -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
> > -		uLong *sourceLen);
> > -#endif
> > +#include "zlib.h"
> 
> Likewise.
> 
> > diff --git a/zlib.c b/zlib.c
> > index d594cba3fc9..d9440dfb784 100644
> > --- a/zlib.c
> > +++ b/zlib.c
> > @@ -3,6 +3,9 @@
> >   * at init time.
> >   */
> >  #include "cache.h"
> > +#ifdef GIT_NO_UNCOMPRESS2
> > +#include "compat/zlib-uncompress2.c"
> > +#endif
> 
> I am not sure if this is a good place to add it.  Everything in this file is _ours_, and
> zlib.o is on LIB_OBJS because it is all _ours_.
> 
> I agree that it is a convenient place to hook into, though.
> 
> I like the basic idea of the patch, but I am afraid that it is way too late in the cycle.  I
> would unconditionally be happy to see a change like this one as part of an earliest
> batch after the release, but there isn't enough in here to convince me that it is
> safe enough or urgent enough to be in the upcoming release.

I am good with this change. On our platform, we have 
#define ZLIB_VERNUM 0x1280

1.2.9 introduced some porting issues and I do not have an updated version. Looking forward to testing this.

I approve this change.

Regards,
Randall