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

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

[Ævar Arnfjörð Bjarmason]

Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
versions that don't have uncompress2().

This makes the compilation of git less annoying on older systems,
since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
dependency on a zlib 1.2.9 or newer unless NO_UNCOMPRESS2=Y is
specified has resulted in errors when git is compiled.

To get around those errors we've needed to bundle config.mak.uname
changes such as such as 68d1da41c4e (build: NonStop ships with an
older zlib, 2022-01-10) and the in-flight
https://lore.kernel.org/git/20220116020520.26895-1-davvid@gmail.com/.

Let's instead rely on ZLIB_VERNUM. Now only those systems where zlib
is so broken that it can't be rely on (such a system probably doesn't
exist) need to provide a NO_UNCOMPRESS2=Y.

See 9da3acfb194 ([PATCH] compat: support pre-1.2 zlib, 2005-04-30) and
609a2289d76 (Improve accuracy of check for presence of deflateBound.,
2007-11-07) for in-tree prior art using ZLIB_VERNUM.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I think this should be strongly considered for inclusion before the
final v2.35.0 release.

Aside from the ones already (and in-flight) in config.mak.uname, I've
run into numerous other cases where NO_UNCOMPRESS2=y is needed (so far
gcc{10,14,45,111,119,135,210} on the GCC farm). Adding
config.mak.uname detections to those would be tedious, we'd need to
start detecting various other OS versions.

Or, we can just ask zlib.h abuot its ZLIB_VERSION instead, and include
compat/zlib-uncompress2.c in our own zlib.c wrapper.

This has an interaction with da/rhel7-lacks-uncompress2-and-c99 (the
merge should preferably delete the NO_UNCOMPRESS2=Y it adds), it's in
"next", but I didn't base this on that topic as "nex" clearly won't be
merged down before v2.35.0.

 Makefile                  |  6 ++++--
 cache.h                   |  5 +++++
 compat/zlib-uncompress2.c |  5 +----
 config.mak.uname          |  6 ------
 reftable/block.c          |  2 +-
 reftable/system.h         | 12 +-----------
 zlib.c                    |  3 +++
 7 files changed, 15 insertions(+), 24 deletions(-)

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.
 #
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
@@ -1728,7 +1731,6 @@ endif
 
 ifdef NO_UNCOMPRESS2
 	BASIC_CFLAGS += -DNO_UNCOMPRESS2
-	REFTABLE_OBJS += compat/zlib-uncompress2.o
 endif
 
 ifdef NO_POSIX_GOODIES
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
 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"
-#define z_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 */
 
 /* clang-format off */
 
diff --git a/config.mak.uname b/config.mak.uname
index 9b3e9bff5f5..d0701f9beb0 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -261,10 +261,6 @@ ifeq ($(uname_S),FreeBSD)
 	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
-	# Versions < 7.0 need compatibility layer
-	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
-		NO_UNCOMPRESS2 = UnfortunatelyYes
-	endif
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
@@ -520,7 +516,6 @@ ifeq ($(uname_S),Interix)
 	endif
 endif
 ifeq ($(uname_S),Minix)
-	NO_UNCOMPRESS2 = YesPlease
 	NO_IPV6 = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
@@ -576,7 +571,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_UNCOMPRESS2 = YesPlease
 	# Currently libiconv-1.9.1.
 	OLD_ICONV = UnfortunatelyYes
 	NO_REGEX = NeedsStartEnd
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"
 
 int header_size(int version)
 {
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"
 
 int hash_size(uint32_t id);
 
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
 
 static const char *zerr_to_string(int status)
 {
-- 
2.35.0.rc1.855.gbd520c8f475

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

[Junio C Hamano]

Æ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.

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

[rsbecker]

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

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

[René Scharfe]

Am 17.01.22 um 18:13 schrieb Ævar Arnfjörð Bjarmason:
> Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
> versions that don't have uncompress2().
>
> This makes the compilation of git less annoying on older systems,
> since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
> compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
> dependency on a zlib 1.2.9 or newer unless NO_UNCOMPRESS2=Y is
> specified has resulted in errors when git is compiled.
>
> To get around those errors we've needed to bundle config.mak.uname
> changes such as such as 68d1da41c4e (build: NonStop ships with an
> older zlib, 2022-01-10) and the in-flight
> https://lore.kernel.org/git/20220116020520.26895-1-davvid@gmail.com/.
>
> Let's instead rely on ZLIB_VERNUM. Now only those systems where zlib
> is so broken that it can't be rely on (such a system probably doesn't
> exist) need to provide a NO_UNCOMPRESS2=Y.
>
> See 9da3acfb194 ([PATCH] compat: support pre-1.2 zlib, 2005-04-30) and
> 609a2289d76 (Improve accuracy of check for presence of deflateBound.,
> 2007-11-07) for in-tree prior art using ZLIB_VERNUM.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> I think this should be strongly considered for inclusion before the
> final v2.35.0 release.
>
> Aside from the ones already (and in-flight) in config.mak.uname, I've
> run into numerous other cases where NO_UNCOMPRESS2=y is needed (so far
> gcc{10,14,45,111,119,135,210} on the GCC farm). Adding
> config.mak.uname detections to those would be tedious, we'd need to
> start detecting various other OS versions.
>
> Or, we can just ask zlib.h abuot its ZLIB_VERSION instead, and include
> compat/zlib-uncompress2.c in our own zlib.c wrapper.
>
> This has an interaction with da/rhel7-lacks-uncompress2-and-c99 (the
> merge should preferably delete the NO_UNCOMPRESS2=Y it adds), it's in
> "next", but I didn't base this on that topic as "nex" clearly won't be
> merged down before v2.35.0.
>
>  Makefile                  |  6 ++++--
>  cache.h                   |  5 +++++
>  compat/zlib-uncompress2.c |  5 +----
>  config.mak.uname          |  6 ------
>  reftable/block.c          |  2 +-
>  reftable/system.h         | 12 +-----------
>  zlib.c                    |  3 +++
>  7 files changed, 15 insertions(+), 24 deletions(-)
>
> 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.
>  #
>  # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
>  # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
> @@ -1728,7 +1731,6 @@ endif
>
>  ifdef NO_UNCOMPRESS2
>  	BASIC_CFLAGS += -DNO_UNCOMPRESS2
> -	REFTABLE_OBJS += compat/zlib-uncompress2.o
>  endif
>
>  ifdef NO_POSIX_GOODIES
> 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
>  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"
> -#define z_const

Why is it safe to remove this definition?  Because it's defined in
zconf.h, included by zlib.h.  But why did we need it in the first place?
Not caused by this patch, of course, but still strange.

> -
>  /*
>   * 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 */

Well, it's rather something like "No #include, period.  Because this
file is not meant to be compiled on its own, but is included itself.",
isn't it?

>
>  /* clang-format off */
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 9b3e9bff5f5..d0701f9beb0 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -261,10 +261,6 @@ ifeq ($(uname_S),FreeBSD)
>  	FILENO_IS_A_MACRO = UnfortunatelyYes
>  endif
>  ifeq ($(uname_S),OpenBSD)
> -	# Versions < 7.0 need compatibility layer
> -	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
> -		NO_UNCOMPRESS2 = UnfortunatelyYes
> -	endif

No longer relying on OS version trivia like this is very nice.

>  	NO_STRCASESTR = YesPlease
>  	NO_MEMMEM = YesPlease
>  	USE_ST_TIMESPEC = YesPlease
> @@ -520,7 +516,6 @@ ifeq ($(uname_S),Interix)
>  	endif
>  endif
>  ifeq ($(uname_S),Minix)
> -	NO_UNCOMPRESS2 = YesPlease
>  	NO_IPV6 = YesPlease
>  	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
>  	NO_NSEC = YesPlease
> @@ -576,7 +571,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	NO_SETENV = YesPlease
>  	NO_UNSETENV = YesPlease
>  	NO_MKDTEMP = YesPlease
> -	NO_UNCOMPRESS2 = YesPlease
>  	# Currently libiconv-1.9.1.
>  	OLD_ICONV = UnfortunatelyYes
>  	NO_REGEX = NeedsStartEnd
> 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"

Why?  We don't have a local zlib.h and this patch doesn't add one.

And don't you need to rather include cache.h, to get the definition
of uncompress2 on systems that don't have it in their zlib.h?

>
>  int header_size(int version)
>  {
> 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"

Same here.  Or rather: Just here.  The include in reftable/block.c
can be removed safely, because it includes this reftable/system.h
here anyway.

>
>  int hash_size(uint32_t id);
>
> 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

Keeping the function definition in its own file in compat/ with its
own license note is a good choice for maintainability, I think.

>
>  static const char *zerr_to_string(int status)
>  {

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

[Ævar Arnfjörð Bjarmason]

Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
versions that don't have uncompress2().

This makes the compilation of git less annoying on older systems.
Since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
dependency on a zlib 1.2.9 or newer, unless NO_UNCOMPRESS2=Y is
specified. This results in new errors when git is compiled on older
systems, requiring the packager to define NO_UNCOMPRESS2=Y.

To get around those errors we've needed to bundle config.mak.uname
changes such as 68d1da41c4e (build: NonStop ships with an older zlib,
2022-01-10), and ffb9f298090 (build: centos/RHEL 7 ships with an older
gcc and zlib, 2022-01-15).

Let's instead rely on ZLIB_VERNUM, as we in zlib.c already for
NO_DEFLATE_BOUND. See 9da3acfb194 ([PATCH] compat: support pre-1.2
zlib, 2005-04-30) and 609a2289d76 (Improve accuracy of check for
presence of deflateBound., 2007-11-07) for that prior art.

With this change it should be safe to remove the NO_UNCOMPRESS2=Y
lines from config.mak.uname, but let's leave them in place for now.

Ideally we'd add compat/zlib-uncompress2.o to COMPAT_OBJS, but it's
being added to our zlib.c instead. This is because we need to look at
ZLIB_VERNUM to know if we need it. Hoisting that logic over to the
Makefile would be inconvenient (we'd need shell out to "cc -E" or
equivalent just to get the macro value). The zlib.c file already has
the similar deflateBound() compatibility macro added in 9da3acfb194.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Per https://lore.kernel.org/git/xmqqwniwo738.fsf@gitster.g/ this
re-roll is probably too late for rc2 & the release, but here's a v2
just in case (or for after the release).

I think this addresses all the points Junio and René brought up on the
v1. I've also omitted any changes to config.mak.uname, they're
redundant now, but we can keep them out of an abundance of
caution. The removal of the already-redundant "z_const" definition. is
also gone here

The wrapper also lives in git-compat-util.h now, instead of cache.h.

Range-diff against v1:
1:  9cea01a1395 ! 1:  444eacf30be cache.h: auto-detect if zlib has uncompress2()
    @@ Commit message
         Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
         versions that don't have uncompress2().
     
    -    This makes the compilation of git less annoying on older systems,
    -    since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
    +    This makes the compilation of git less annoying on older systems.
    +    Since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
         compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
    -    dependency on a zlib 1.2.9 or newer unless NO_UNCOMPRESS2=Y is
    -    specified has resulted in errors when git is compiled.
    +    dependency on a zlib 1.2.9 or newer, unless NO_UNCOMPRESS2=Y is
    +    specified. This results in new errors when git is compiled on older
    +    systems, requiring the packager to define NO_UNCOMPRESS2=Y.
     
         To get around those errors we've needed to bundle config.mak.uname
    -    changes such as such as 68d1da41c4e (build: NonStop ships with an
    -    older zlib, 2022-01-10) and the in-flight
    -    https://lore.kernel.org/git/20220116020520.26895-1-davvid@gmail.com/.
    +    changes such as 68d1da41c4e (build: NonStop ships with an older zlib,
    +    2022-01-10), and ffb9f298090 (build: centos/RHEL 7 ships with an older
    +    gcc and zlib, 2022-01-15).
     
    -    Let's instead rely on ZLIB_VERNUM. Now only those systems where zlib
    -    is so broken that it can't be rely on (such a system probably doesn't
    -    exist) need to provide a NO_UNCOMPRESS2=Y.
    +    Let's instead rely on ZLIB_VERNUM, as we in zlib.c already for
    +    NO_DEFLATE_BOUND. See 9da3acfb194 ([PATCH] compat: support pre-1.2
    +    zlib, 2005-04-30) and 609a2289d76 (Improve accuracy of check for
    +    presence of deflateBound., 2007-11-07) for that prior art.
     
    -    See 9da3acfb194 ([PATCH] compat: support pre-1.2 zlib, 2005-04-30) and
    -    609a2289d76 (Improve accuracy of check for presence of deflateBound.,
    -    2007-11-07) for in-tree prior art using ZLIB_VERNUM.
    +    With this change it should be safe to remove the NO_UNCOMPRESS2=Y
    +    lines from config.mak.uname, but let's leave them in place for now.
    +
    +    Ideally we'd add compat/zlib-uncompress2.o to COMPAT_OBJS, but it's
    +    being added to our zlib.c instead. This is because we need to look at
    +    ZLIB_VERNUM to know if we need it. Hoisting that logic over to the
    +    Makefile would be inconvenient (we'd need shell out to "cc -E" or
    +    equivalent just to get the macro value). The zlib.c file already has
    +    the similar deflateBound() compatibility macro added in 9da3acfb194.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
     @@ Makefile: 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.
    + # Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
    ++# You should not need to define this, it will be auto-detected using
    ++# the ZLIB_VERNUM macro.
      #
      # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
      # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
    @@ Makefile: endif
      
      ifdef NO_POSIX_GOODIES
     
    - ## cache.h ##
    -@@ cache.h: 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
    - void git_inflate_init(git_zstream *);
    - void git_inflate_init_gzip_only(git_zstream *);
    - void git_inflate_end(git_zstream *);
    -
      ## compat/zlib-uncompress2.c ##
     @@
      
      */
      
     -#include "../reftable/system.h"
    --#define z_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
    -  */
    + #define z_const
      
    --#include <zlib.h>
    -+/* No "#include <zlib.h>", done in cache.h */
    - 
    - /* clang-format off */
    - 
    -
    - ## config.mak.uname ##
    -@@ config.mak.uname: ifeq ($(uname_S),FreeBSD)
    - 	FILENO_IS_A_MACRO = UnfortunatelyYes
    - endif
    - ifeq ($(uname_S),OpenBSD)
    --	# Versions < 7.0 need compatibility layer
    --	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
    --		NO_UNCOMPRESS2 = UnfortunatelyYes
    --	endif
    - 	NO_STRCASESTR = YesPlease
    - 	NO_MEMMEM = YesPlease
    - 	USE_ST_TIMESPEC = YesPlease
    -@@ config.mak.uname: ifeq ($(uname_S),Interix)
    - 	endif
    - endif
    - ifeq ($(uname_S),Minix)
    --	NO_UNCOMPRESS2 = YesPlease
    - 	NO_IPV6 = YesPlease
    - 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
    - 	NO_NSEC = YesPlease
    -@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
    - 	NO_SETENV = YesPlease
    - 	NO_UNSETENV = YesPlease
    - 	NO_MKDTEMP = YesPlease
    --	NO_UNCOMPRESS2 = YesPlease
    - 	# Currently libiconv-1.9.1.
    - 	OLD_ICONV = UnfortunatelyYes
    - 	NO_REGEX = NeedsStartEnd
    + /*
     
    - ## reftable/block.c ##
    -@@ reftable/block.c: 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"
    + ## git-compat-util.h ##
    +@@ git-compat-util.h: void unleak_memory(const void *ptr, size_t len);
    + #define UNLEAK(var) do {} while (0)
    + #endif
      
    - int header_size(int version)
    - {
    ++#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
    ++#include <zlib.h>
    ++#define GIT_NO_UNCOMPRESS2 1
    ++int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
    ++		uLong *sourceLen);
    ++#endif
    ++
    + /*
    +  * This include must come after system headers, since it introduces macros that
    +  * replace system names.
     
      ## reftable/system.h ##
     @@ reftable/system.h: 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>
    --
    + 
    + #include <zlib.h>
    + 
     -#ifdef NO_UNCOMPRESS2
     -/*
     - * This is uncompress2, which is only available in zlib >= 1.2.9
    @@ reftable/system.h: license that can be found in the LICENSE file or at
     -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
     -		uLong *sourceLen);
     -#endif
    -+#include "zlib.h"
    - 
    +-
      int hash_size(uint32_t id);
      
    + #endif
     
      ## zlib.c ##
     @@

 Makefile                  | 3 ++-
 compat/zlib-uncompress2.c | 1 -
 git-compat-util.h         | 7 +++++++
 reftable/system.h         | 9 ---------
 zlib.c                    | 3 +++
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 5580859afdb..616f6527f07 100644
--- a/Makefile
+++ b/Makefile
@@ -257,6 +257,8 @@ all::
 # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
 #
 # Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
+# You should not need to define this, it will be auto-detected using
+# the ZLIB_VERNUM macro.
 #
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
@@ -1728,7 +1730,6 @@ endif
 
 ifdef NO_UNCOMPRESS2
 	BASIC_CFLAGS += -DNO_UNCOMPRESS2
-	REFTABLE_OBJS += compat/zlib-uncompress2.o
 endif
 
 ifdef NO_POSIX_GOODIES
diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
index 722610b9718..8592dc3dab5 100644
--- a/compat/zlib-uncompress2.c
+++ b/compat/zlib-uncompress2.c
@@ -8,7 +8,6 @@
 
 */
 
-#include "../reftable/system.h"
 #define z_const
 
 /*
diff --git a/git-compat-util.h b/git-compat-util.h
index 1229c8296b9..0c5e373e917 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1386,6 +1386,13 @@ void unleak_memory(const void *ptr, size_t len);
 #define UNLEAK(var) do {} while (0)
 #endif
 
+#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
+#include <zlib.h>
+#define GIT_NO_UNCOMPRESS2 1
+int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
+		uLong *sourceLen);
+#endif
+
 /*
  * This include must come after system headers, since it introduces macros that
  * replace system names.
diff --git a/reftable/system.h b/reftable/system.h
index 4907306c0c5..c69caaabeba 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -18,15 +18,6 @@ license that can be found in the LICENSE file or at
 
 #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
-
 int hash_size(uint32_t id);
 
 #endif
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
 
 static const char *zerr_to_string(int status)
 {
-- 
2.35.0.rc1.864.g57621b115b6

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
> versions that don't have uncompress2().
>
> This makes the compilation of git less annoying on older systems.
> Since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
> compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
> dependency on a zlib 1.2.9 or newer, unless NO_UNCOMPRESS2=Y is
> specified. This results in new errors when git is compiled on older
> systems, requiring the packager to define NO_UNCOMPRESS2=Y.
>
> To get around those errors we've needed to bundle config.mak.uname
> changes such as 68d1da41c4e (build: NonStop ships with an older zlib,
> 2022-01-10), and ffb9f298090 (build: centos/RHEL 7 ships with an older
> gcc and zlib, 2022-01-15).
>
> Let's instead rely on ZLIB_VERNUM, as we in zlib.c already for
> NO_DEFLATE_BOUND. See 9da3acfb194 ([PATCH] compat: support pre-1.2
> zlib, 2005-04-30) and 609a2289d76 (Improve accuracy of check for
> presence of deflateBound., 2007-11-07) for that prior art.

> With this change it should be safe to remove the NO_UNCOMPRESS2=Y
> lines from config.mak.uname, but let's leave them in place for now.

It is not a big deal but I think removing is probably a more
sensible decision.  If ZLIB_VERNUM does not misdetect for them, they
will not be harmed.  And if it does, we'd rather hear from them
about the misdetection.

> Ideally we'd add compat/zlib-uncompress2.o to COMPAT_OBJS, but it's
> being added to our zlib.c instead. This is because we need to look at
> ZLIB_VERNUM to know if we need it. Hoisting that logic over to the
> Makefile would be inconvenient (we'd need shell out to "cc -E" or
> equivalent just to get the macro value). The zlib.c file already has
> the similar deflateBound() compatibility macro added in 9da3acfb194.

I am still unhappy about this part (meaning, I am happier about
everything else, compared to the previous iteration), that zlib.o
becomes mixture of our code and borrowed code.

The COMPAT mechanism have been that compat/foo.o is only compiled
and linked on a platform that lack foo and needs our replacement.
But in this case, I think the best would be to enclose the bulk of
zlib-uncompress2.c file inside a "#if ZLIB_VERNUM < ... #endif"
block and _always_ comiple and link the resulting zlib-uncompress2.o
file.  We probably need to include a throw-away global symbol,
either a variable or a function, that is externally visible but
otherwise unused, to appease compilers linkers that do not want to
deal with an absolutely empty object file on platforms with a
working uncompress2() in their system zlib.

That way, we do not have to touch an unrelated file (zlib.c) for
this, which may be a lot cleaner.

> diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
> index 722610b9718..8592dc3dab5 100644
> --- a/compat/zlib-uncompress2.c
> +++ b/compat/zlib-uncompress2.c
> @@ -8,7 +8,6 @@
>  
>  */
>  
> -#include "../reftable/system.h"
>  #define z_const
>  
>  /*
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 1229c8296b9..0c5e373e917 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1386,6 +1386,13 @@ void unleak_memory(const void *ptr, size_t len);
>  #define UNLEAK(var) do {} while (0)
>  #endif
>  
> +#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
> +#include <zlib.h>
> +#define GIT_NO_UNCOMPRESS2 1
> +int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
> +		uLong *sourceLen);
> +#endif

It is odd that we did not include <zlib.h> in this file, as the
design goal of git-compat-util.h is to free individual sources from
having to worry about order of inclusion and proprocessor symbols we
need to define before including certain system headers, etc. (for
example, defining z_const before including <zlib.h> would change the
"behaviour" of <zlib.h> header).

We are including <zlib.h> in <cache.h>, way after including the
<git-compat-util.h> file.  I have a feeling that it may become
cleaner (and more correct) if we unconditionally include <zlib.h>
*BEFORE* we check ZLIB_VERNUM here, and lost the inclusion of
<zlib.h> from <cache.h>.  With the posted patch, where are we
getting ZLIB_VERNUM from, which is used to decide if we declare
uncompress2() ourselves here?

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

[Ævar Arnfjörð Bjarmason]

Remove the NO_UNCOMPRESS2=Y setting added in a322920d0bb (Provide
zlib's uncompress2 from compat/zlib-compat.c, 2021-10-07) and its
configure probe in favor of auto-detecting if we need to provide a
fallback uncompress2().

This makes the compilation of git less annoying on older systems.
Since that a322920d0bb in included in v2.35.0-rc0 we've started
requiring a zlib version 1.2.9 or later. Those compiling on older
versions needed to specify NO_UNCOMPRESS2=Y. That resulted in new
errors when git was compiled on older systems, requiring the packager
to define NO_UNCOMPRESS2=Y.

Let's instead add a compat/zlib-uncompress.o object which'll define
uncompress2() if the ZLIB_VERNUM indicates that libz doesn't have
it. The approach taken here requires some juggling of *.o lists in the
Makefile. It would be simpler to e.g. include this code
unconditionally in our own zlib.c, but per [1] this approach preserves
the compat/* object divide.

Because the code in reftable/* needs this, and we compile an
intermediate reftable.a we'll need a new $(ZLIB_COMPAT_OBJS) list. We
add it to $(LIB_OBJS) (currently redundant, but means using
uncompress2() elsewhere will work), as well as compiling the
$(REFTABLE_LIB) with $(ZLIB_COMPAT_OBJS).

We can then remove the NO_UNCOMPRESS2 setting, and the NO_UNCOMPRESS2
settings in config.mak.uname added in a322920d0bb, and then
subsequently in 68d1da41c4e (build: NonStop ships with an older zlib,
2022-01-10) and ffb9f298090 (build: centos/RHEL 7 ships with an older
gcc and zlib, 2022-01-15).

Since the prototype of uncompress2() lives in git-compat-util.h we'll
need to include zlib.h there. We can then remove the inclusion of
zlib.h from cache,h, since it includes git-compat-util.h. The
inclusion of zlib.h in cache.h dates back to e83c5163316 (Initial
revision of "git", the information manager from hell, 2005-04-07).

1. https://lore.kernel.org/git/xmqq1r13o7rf.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A v3 addressing the points Junio raised in
https://lore.kernel.org/git/xmqq1r13o7rf.fsf@gitster.g/.

As noted in the updated commit message this approach of having an
object just for this fallback function comes at the cost of some
complexity, but now the compat object lives neatly in its own object.

I didn't run into the "We probably need to include a throw-away global
symbol[...]" issue Junio noted when testing this.

Range-diff against v2:
1:  444eacf30be ! 1:  e9cb8763fd4 cache.h: auto-detect if zlib has uncompress2()
    @@ Metadata
      ## Commit message ##
         cache.h: auto-detect if zlib has uncompress2()
     
    -    Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
    -    versions that don't have uncompress2().
    +    Remove the NO_UNCOMPRESS2=Y setting added in a322920d0bb (Provide
    +    zlib's uncompress2 from compat/zlib-compat.c, 2021-10-07) and its
    +    configure probe in favor of auto-detecting if we need to provide a
    +    fallback uncompress2().
     
         This makes the compilation of git less annoying on older systems.
    -    Since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
    -    compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
    -    dependency on a zlib 1.2.9 or newer, unless NO_UNCOMPRESS2=Y is
    -    specified. This results in new errors when git is compiled on older
    -    systems, requiring the packager to define NO_UNCOMPRESS2=Y.
    -
    -    To get around those errors we've needed to bundle config.mak.uname
    -    changes such as 68d1da41c4e (build: NonStop ships with an older zlib,
    -    2022-01-10), and ffb9f298090 (build: centos/RHEL 7 ships with an older
    -    gcc and zlib, 2022-01-15).
    +    Since that a322920d0bb in included in v2.35.0-rc0 we've started
    +    requiring a zlib version 1.2.9 or later. Those compiling on older
    +    versions needed to specify NO_UNCOMPRESS2=Y. That resulted in new
    +    errors when git was compiled on older systems, requiring the packager
    +    to define NO_UNCOMPRESS2=Y.
    +
    +    Let's instead add a compat/zlib-uncompress.o object which'll define
    +    uncompress2() if the ZLIB_VERNUM indicates that libz doesn't have
    +    it. The approach taken here requires some juggling of *.o lists in the
    +    Makefile. It would be simpler to e.g. include this code
    +    unconditionally in our own zlib.c, but per [1] this approach preserves
    +    the compat/* object divide.
    +
    +    Because the code in reftable/* needs this, and we compile an
    +    intermediate reftable.a we'll need a new $(ZLIB_COMPAT_OBJS) list. We
    +    add it to $(LIB_OBJS) (currently redundant, but means using
    +    uncompress2() elsewhere will work), as well as compiling the
    +    $(REFTABLE_LIB) with $(ZLIB_COMPAT_OBJS).
     
    -    Let's instead rely on ZLIB_VERNUM, as we in zlib.c already for
    -    NO_DEFLATE_BOUND. See 9da3acfb194 ([PATCH] compat: support pre-1.2
    -    zlib, 2005-04-30) and 609a2289d76 (Improve accuracy of check for
    -    presence of deflateBound., 2007-11-07) for that prior art.
    +    We can then remove the NO_UNCOMPRESS2 setting, and the NO_UNCOMPRESS2
    +    settings in config.mak.uname added in a322920d0bb, and then
    +    subsequently in 68d1da41c4e (build: NonStop ships with an older zlib,
    +    2022-01-10) and ffb9f298090 (build: centos/RHEL 7 ships with an older
    +    gcc and zlib, 2022-01-15).
     
    -    With this change it should be safe to remove the NO_UNCOMPRESS2=Y
    -    lines from config.mak.uname, but let's leave them in place for now.
    +    Since the prototype of uncompress2() lives in git-compat-util.h we'll
    +    need to include zlib.h there. We can then remove the inclusion of
    +    zlib.h from cache,h, since it includes git-compat-util.h. The
    +    inclusion of zlib.h in cache.h dates back to e83c5163316 (Initial
    +    revision of "git", the information manager from hell, 2005-04-07).
     
    -    Ideally we'd add compat/zlib-uncompress2.o to COMPAT_OBJS, but it's
    -    being added to our zlib.c instead. This is because we need to look at
    -    ZLIB_VERNUM to know if we need it. Hoisting that logic over to the
    -    Makefile would be inconvenient (we'd need shell out to "cc -E" or
    -    equivalent just to get the macro value). The zlib.c file already has
    -    the similar deflateBound() compatibility macro added in 9da3acfb194.
    +    1. https://lore.kernel.org/git/xmqq1r13o7rf.fsf@gitster.g/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
     @@ Makefile: all::
    - # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
      #
    - # Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
    -+# You should not need to define this, it will be auto-detected using
    -+# the ZLIB_VERNUM macro.
    + # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
      #
    +-# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
    +-#
      # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
      # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
    -@@ Makefile: endif
    + #
    +@@ Makefile: TEST_BUILTINS_OBJS =
    + TEST_OBJS =
    + TEST_PROGRAMS_NEED_X =
    + THIRD_PARTY_SOURCES =
    ++ZLIB_COMPAT_OBJS =
      
    - ifdef NO_UNCOMPRESS2
    - 	BASIC_CFLAGS += -DNO_UNCOMPRESS2
    --	REFTABLE_OBJS += compat/zlib-uncompress2.o
    + # Having this variable in your environment would break pipelines because
    + # you cause "cd" to echo its destination to stdout.  It can also take
    +@@ Makefile: ifdef NO_DEFLATE_BOUND
    + 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
      endif
      
    +-ifdef NO_UNCOMPRESS2
    +-	BASIC_CFLAGS += -DNO_UNCOMPRESS2
    +-	REFTABLE_OBJS += compat/zlib-uncompress2.o
    +-endif
    ++# Detected using the ZLIB_VERNUM macro
    ++ZLIB_COMPAT_OBJS += compat/zlib-uncompress2.o
    + 
      ifdef NO_POSIX_GOODIES
    + 	BASIC_CFLAGS += -DNO_POSIX_GOODIES
    +@@ Makefile: LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
    + 
    + BASIC_CFLAGS += $(COMPAT_CFLAGS)
    + LIB_OBJS += $(COMPAT_OBJS)
    ++LIB_OBJS += $(ZLIB_COMPAT_OBJS)
    + 
    + # Quote for C
    + 
    +@@ Makefile: $(LIB_FILE): $(LIB_OBJS)
    + $(XDIFF_LIB): $(XDIFF_OBJS)
    + 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
    + 
    +-$(REFTABLE_LIB): $(REFTABLE_OBJS)
    ++$(REFTABLE_LIB): $(REFTABLE_OBJS) $(ZLIB_COMPAT_OBJS)
    + 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
    + 
    + $(REFTABLE_TEST_LIB): $(REFTABLE_TEST_OBJS)
    +
    + ## cache.h ##
    +@@
    + #include "repository.h"
    + #include "mem-pool.h"
    + 
    +-#include <zlib.h>
    + typedef struct git_zstream {
    + 	z_stream z;
    + 	unsigned long avail_in;
    +
    + ## ci/lib.sh ##
    +@@ ci/lib.sh: esac
    + case "$jobname" in
    + linux32)
    + 	CC=gcc
    +-	MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1"
    + 	;;
    + linux-musl)
    + 	CC=gcc
     
      ## compat/zlib-uncompress2.c ##
     @@
    ++#include "git-compat-util.h"
    ++
    ++#if ZLIB_VERNUM < 0x1290
    + /* taken from zlib's uncompr.c
    + 
    +    commit cacf7f1d4e3d44d871b605da3b647f07d718623f
    +@@
      
      */
      
    @@ compat/zlib-uncompress2.c
      #define z_const
      
      /*
    +@@
    +  * For conditions of distribution and use, see copyright notice in zlib.h
    +  */
    + 
    +-#include <zlib.h>
    +-
    + /* clang-format off */
    + 
    + /* ===========================================================================
    +@@ compat/zlib-uncompress2.c: int ZEXPORT uncompress2 (
    + 	   err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR :
    + 	   err;
    + }
    ++#endif
    +
    + ## config.mak.uname ##
    +@@ config.mak.uname: ifeq ($(uname_S),Linux)
    + 	# centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7.
    + 	ifneq ($(findstring .el7.,$(uname_R)),)
    + 		BASIC_CFLAGS += -std=c99
    +-		NO_UNCOMPRESS2 = YesPlease
    + 	endif
    + endif
    + ifeq ($(uname_S),GNU/kFreeBSD)
    +@@ config.mak.uname: ifeq ($(uname_S),FreeBSD)
    + 	FILENO_IS_A_MACRO = UnfortunatelyYes
    + endif
    + ifeq ($(uname_S),OpenBSD)
    +-	# Versions < 7.0 need compatibility layer
    +-	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
    +-		NO_UNCOMPRESS2 = UnfortunatelyYes
    +-	endif
    + 	NO_STRCASESTR = YesPlease
    + 	NO_MEMMEM = YesPlease
    + 	USE_ST_TIMESPEC = YesPlease
    +@@ config.mak.uname: ifeq ($(uname_S),Interix)
    + 	endif
    + endif
    + ifeq ($(uname_S),Minix)
    +-	NO_UNCOMPRESS2 = YesPlease
    + 	NO_IPV6 = YesPlease
    + 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
    + 	NO_NSEC = YesPlease
    +@@ config.mak.uname: ifeq ($(uname_S),NONSTOP_KERNEL)
    + 	NO_SETENV = YesPlease
    + 	NO_UNSETENV = YesPlease
    + 	NO_MKDTEMP = YesPlease
    +-	NO_UNCOMPRESS2 = YesPlease
    + 	# Currently libiconv-1.9.1.
    + 	OLD_ICONV = UnfortunatelyYes
    + 	NO_REGEX = NeedsStartEnd
    +
    + ## configure.ac ##
    +@@ configure.ac: AC_LINK_IFELSE([ZLIBTEST_SRC],
    + 	NO_DEFLATE_BOUND=yes])
    + LIBS="$old_LIBS"
    + 
    +-AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [
    +-AC_LANG_PROGRAM([#include <zlib.h>],
    +- [uncompress2(NULL,NULL,NULL,NULL);])])
    +-AC_MSG_CHECKING([for uncompress2 in -lz])
    +-old_LIBS="$LIBS"
    +-LIBS="$LIBS -lz"
    +-AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC],
    +-	[AC_MSG_RESULT([yes])],
    +-	[AC_MSG_RESULT([no])
    +-	NO_UNCOMPRESS2=yes])
    +-LIBS="$old_LIBS"
    +-
    + GIT_UNSTASH_FLAGS($ZLIB_PATH)
    + 
    + GIT_CONF_SUBST([NO_DEFLATE_BOUND])
    +-GIT_CONF_SUBST([NO_UNCOMPRESS2])
    + 
    + #
    + # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
     
      ## git-compat-util.h ##
     @@ git-compat-util.h: void unleak_memory(const void *ptr, size_t len);
      #define UNLEAK(var) do {} while (0)
      #endif
      
    -+#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
     +#include <zlib.h>
    -+#define GIT_NO_UNCOMPRESS2 1
    ++
    ++#if ZLIB_VERNUM < 0x1290
    ++/*
    ++ * This is uncompress2, which is only available in zlib >= 1.2.9
    ++ * (released as of early 2017). See compat/zlib-uncompress2.c.
    ++ */
     +int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
     +		uLong *sourceLen);
     +#endif
    @@ git-compat-util.h: void unleak_memory(const void *ptr, size_t len);
     
      ## reftable/system.h ##
     @@ reftable/system.h: license that can be found in the LICENSE file or at
    + #include "hash.h" /* hash ID, sizes.*/
    + #include "dir.h" /* remove_dir_recursively, for tests.*/
      
    - #include <zlib.h>
    - 
    +-#include <zlib.h>
    +-
     -#ifdef NO_UNCOMPRESS2
     -/*
     - * This is uncompress2, which is only available in zlib >= 1.2.9
    @@ reftable/system.h: license that can be found in the LICENSE file or at
      int hash_size(uint32_t id);
      
      #endif
    -
    - ## zlib.c ##
    -@@
    -  * at init time.
    -  */
    - #include "cache.h"
    -+#ifdef GIT_NO_UNCOMPRESS2
    -+#include "compat/zlib-uncompress2.c"
    -+#endif
    - 
    - static const char *zerr_to_string(int status)
    - {

 Makefile                  | 12 +++++-------
 cache.h                   |  1 -
 ci/lib.sh                 |  1 -
 compat/zlib-uncompress2.c |  7 ++++---
 config.mak.uname          |  7 -------
 configure.ac              | 13 -------------
 git-compat-util.h         | 11 +++++++++++
 reftable/system.h         | 11 -----------
 8 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/Makefile b/Makefile
index 5580859afdb..416498bbe69 100644
--- a/Makefile
+++ b/Makefile
@@ -256,8 +256,6 @@ all::
 #
 # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
 #
-# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
-#
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
 #
@@ -588,6 +586,7 @@ TEST_BUILTINS_OBJS =
 TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
+ZLIB_COMPAT_OBJS =
 
 # Having this variable in your environment would break pipelines because
 # you cause "cd" to echo its destination to stdout.  It can also take
@@ -1726,10 +1725,8 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
 
-ifdef NO_UNCOMPRESS2
-	BASIC_CFLAGS += -DNO_UNCOMPRESS2
-	REFTABLE_OBJS += compat/zlib-uncompress2.o
-endif
+# Detected using the ZLIB_VERNUM macro
+ZLIB_COMPAT_OBJS += compat/zlib-uncompress2.o
 
 ifdef NO_POSIX_GOODIES
 	BASIC_CFLAGS += -DNO_POSIX_GOODIES
@@ -2076,6 +2073,7 @@ LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
+LIB_OBJS += $(ZLIB_COMPAT_OBJS)
 
 # Quote for C
 
@@ -2641,7 +2639,7 @@ $(LIB_FILE): $(LIB_OBJS)
 $(XDIFF_LIB): $(XDIFF_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
 
-$(REFTABLE_LIB): $(REFTABLE_OBJS)
+$(REFTABLE_LIB): $(REFTABLE_OBJS) $(ZLIB_COMPAT_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
 
 $(REFTABLE_TEST_LIB): $(REFTABLE_TEST_OBJS)
diff --git a/cache.h b/cache.h
index 281f00ab1b1..3a0142aa56f 100644
--- a/cache.h
+++ b/cache.h
@@ -18,7 +18,6 @@
 #include "repository.h"
 #include "mem-pool.h"
 
-#include <zlib.h>
 typedef struct git_zstream {
 	z_stream z;
 	unsigned long avail_in;
diff --git a/ci/lib.sh b/ci/lib.sh
index 9d28ab50fb4..cbc2f8f1caa 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -197,7 +197,6 @@ esac
 case "$jobname" in
 linux32)
 	CC=gcc
-	MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1"
 	;;
 linux-musl)
 	CC=gcc
diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
index 722610b9718..6836a1d37d8 100644
--- a/compat/zlib-uncompress2.c
+++ b/compat/zlib-uncompress2.c
@@ -1,3 +1,6 @@
+#include "git-compat-util.h"
+
+#if ZLIB_VERNUM < 0x1290
 /* taken from zlib's uncompr.c
 
    commit cacf7f1d4e3d44d871b605da3b647f07d718623f
@@ -8,7 +11,6 @@
 
 */
 
-#include "../reftable/system.h"
 #define z_const
 
 /*
@@ -16,8 +18,6 @@
  * For conditions of distribution and use, see copyright notice in zlib.h
  */
 
-#include <zlib.h>
-
 /* clang-format off */
 
 /* ===========================================================================
@@ -93,3 +93,4 @@ int ZEXPORT uncompress2 (
 	   err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR :
 	   err;
 }
+#endif
diff --git a/config.mak.uname b/config.mak.uname
index c48db45106c..92ea00c219d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -66,7 +66,6 @@ ifeq ($(uname_S),Linux)
 	# centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7.
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
-		NO_UNCOMPRESS2 = YesPlease
 	endif
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
@@ -266,10 +265,6 @@ ifeq ($(uname_S),FreeBSD)
 	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
-	# Versions < 7.0 need compatibility layer
-	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
-		NO_UNCOMPRESS2 = UnfortunatelyYes
-	endif
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
@@ -525,7 +520,6 @@ ifeq ($(uname_S),Interix)
 	endif
 endif
 ifeq ($(uname_S),Minix)
-	NO_UNCOMPRESS2 = YesPlease
 	NO_IPV6 = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
@@ -581,7 +575,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_UNCOMPRESS2 = YesPlease
 	# Currently libiconv-1.9.1.
 	OLD_ICONV = UnfortunatelyYes
 	NO_REGEX = NeedsStartEnd
diff --git a/configure.ac b/configure.ac
index d60d494ee4c..5ee25ec95c8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -664,22 +664,9 @@ AC_LINK_IFELSE([ZLIBTEST_SRC],
 	NO_DEFLATE_BOUND=yes])
 LIBS="$old_LIBS"
 
-AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [
-AC_LANG_PROGRAM([#include <zlib.h>],
- [uncompress2(NULL,NULL,NULL,NULL);])])
-AC_MSG_CHECKING([for uncompress2 in -lz])
-old_LIBS="$LIBS"
-LIBS="$LIBS -lz"
-AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC],
-	[AC_MSG_RESULT([yes])],
-	[AC_MSG_RESULT([no])
-	NO_UNCOMPRESS2=yes])
-LIBS="$old_LIBS"
-
 GIT_UNSTASH_FLAGS($ZLIB_PATH)
 
 GIT_CONF_SUBST([NO_DEFLATE_BOUND])
-GIT_CONF_SUBST([NO_UNCOMPRESS2])
 
 #
 # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
diff --git a/git-compat-util.h b/git-compat-util.h
index 1229c8296b9..96ee35f9e06 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1386,6 +1386,17 @@ void unleak_memory(const void *ptr, size_t len);
 #define UNLEAK(var) do {} while (0)
 #endif
 
+#include <zlib.h>
+
+#if ZLIB_VERNUM < 0x1290
+/*
+ * This is uncompress2, which is only available in zlib >= 1.2.9
+ * (released as of early 2017). See compat/zlib-uncompress2.c.
+ */
+int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
+		uLong *sourceLen);
+#endif
+
 /*
  * This include must come after system headers, since it introduces macros that
  * replace system names.
diff --git a/reftable/system.h b/reftable/system.h
index 4907306c0c5..18f9207dfee 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -16,17 +16,6 @@ license that can be found in the LICENSE file or at
 #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
-
 int hash_size(uint32_t id);
 
 #endif
-- 
2.35.0.rc1.864.g57621b115b6

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

[Carlo Arenas]

On Mon, Jan 17, 2022 at 11:49 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 17.01.22 um 18:13 schrieb Ævar Arnfjörð Bjarmason:
> > 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"
> > -#define z_const
>
> Why is it safe to remove this definition?
> Because it's defined in
> zconf.h, included by zlib.h.  But why did we need it in the first place?

because really old versions of zlib don't have it defined in zconf.h,
and therefore the "compat" code would fail to build.

Carlo

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As noted in the updated commit message this approach of having an
> object just for this fallback function comes at the cost of some
> complexity, but now the compat object lives neatly in its own object.

I do not see any change in this patch adding costly complexity, but
I notice lack of one possible trick that might become problem with
some compilers and linkers when their zlib has uncompress2()
function.  Let's have this graduate very early in the next cycle, to
see if anybody on a rarer system sees a complaint due to having to
deal with a totally empty object file.

Will queue.

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

[rsbecker]

On January 21, 2022 6:23 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > As noted in the updated commit message this approach of having an
> > object just for this fallback function comes at the cost of some
> > complexity, but now the compat object lives neatly in its own object.
> 
> I do not see any change in this patch adding costly complexity, but I notice lack of
> one possible trick that might become problem with some compilers and linkers
> when their zlib has uncompress2() function.  Let's have this graduate very early in
> the next cycle, to see if anybody on a rarer system sees a complaint due to having
> to deal with a totally empty object file.
> 
> Will queue.

On behalf of the "rarer systems", I will certainly be putting this through the regression suite.
With thanks,
--Randall

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

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> On January 21, 2022 6:23 PM, Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> 
>> > As noted in the updated commit message this approach of having an
>> > object just for this fallback function comes at the cost of some
>> > complexity, but now the compat object lives neatly in its own object.
>> 
>> I do not see any change in this patch adding costly complexity, but I notice lack of
>> one possible trick that might become problem with some compilers and linkers
>> when their zlib has uncompress2() function.  Let's have this graduate very early in
>> the next cycle, to see if anybody on a rarer system sees a complaint due to having
>> to deal with a totally empty object file.
>> 
>> Will queue.
>
> On behalf of the "rarer systems", I will certainly be putting this through the regression suite.
> With thanks,
> --Randall

;-)  

I know you are on a rarer system, but I also know you need a real
replacement code in the compat object file, so your linker will not
be dealing with an empty object file.

Thanks.

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

[Beat Bolli]

On 22.01.22 00:23, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
>> As noted in the updated commit message this approach of having an
>> object just for this fallback function comes at the cost of some
>> complexity, but now the compat object lives neatly in its own object.
> 
> I do not see any change in this patch adding costly complexity, but
> I notice lack of one possible trick that might become problem with
> some compilers and linkers when their zlib has uncompress2()
> function.  Let's have this graduate very early in the next cycle, to
> see if anybody on a rarer system sees a complaint due to having to
> deal with a totally empty object file.

OpenSSL has a macro in include/openssl/macros.h to counteract exactly this:

     /*
      * Sometimes OPENSSL_NO_xxx ends up with an empty file and some 
compilers
      * don't like that.  This will hopefully silence them.
      */
     #define NON_EMPTY_TRANSLATION_UNIT static void *dummy = &dummy;

They insert it in the otherwise empty "#else" branch of conditionally 
complied code.


Cheers, Beat

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

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> On 22.01.22 00:23, Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> 
>>> As noted in the updated commit message this approach of having an
>>> object just for this fallback function comes at the cost of some
>>> complexity, but now the compat object lives neatly in its own object.
>> I do not see any change in this patch adding costly complexity, but
>> I notice lack of one possible trick that might become problem with
>> some compilers and linkers when their zlib has uncompress2()
>> function.  Let's have this graduate very early in the next cycle, to
>> see if anybody on a rarer system sees a complaint due to having to
>> deal with a totally empty object file.
>
> OpenSSL has a macro in include/openssl/macros.h to counteract exactly this:
>
>     /*
>      * Sometimes OPENSSL_NO_xxx ends up with an empty file and some
>        compilers
>      * don't like that.  This will hopefully silence them.
>      */
>     #define NON_EMPTY_TRANSLATION_UNIT static void *dummy = &dummy;
>
> They insert it in the otherwise empty "#else" branch of conditionally
> complied code.

Between my "I recall some compilers and linkers had trouble with an
totally empty object files, and we'd better be prepared for them"
and "I didn't see any system that had such a problem during my
tests", I still lean toward "it merely shows that the problem is
rarer than the set of systems you tried", even without further
input.

But "It is a problem for which a real workaround is used in a system
that is used more widely than we are" is a clear enough evidence
that we should have one, especially the solution is so trivial.

https://github.com/openssl/openssl/pull/10425 seems to indicate that
they are moving in a direction that removes the necessity to use
this macro, by switching to tell the build system to not compile and
build a file that would become empty due to conditionals, but nobody
in the discussion seems to question the need for the macro for
portability if an otherwise empty file need to be dealt with.

Thanks.

[PATCH v4] compat: auto-detect if zlib has uncompress2()

[Junio C Hamano]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

We have a copy of uncompress2() implementation in compat/ so that we
can build with an older version of zlib that lack the function, and
the build procedure selects if it is used via the NO_UNCOMPRESS2
$(MAKE) variable.  This is yet another "annoying" knob the porters
need to tweak on platforms that are not common enough to have the
default set in the config.mak.uname file.

Ævar came up with an idea to instead ask the system header <zlib.h>
to decide if we need the compatibility implementation.  We can
compile and link compat/zlib-uncompress2.c file unconditionally, but
conditionally hide the implementation behind #if/#endif when zlib
version is 1.2.9 or newer, and unconditionally archive the resulting
object file in the libgit.a to be picked up by the linker.

There are a few things to note in the shape of the code base after
this change:

 - We no longer use NO_UNCOMPRESS2 knob; if the system header
   <zlib.h> claims a version that is more cent than the library
   actually is, this would break, but it is easy to add it back when
   we find such a system.

 - The object file compat/zlib-uncompress2.o is always compiled and
   archived in libgit.a, just like a few other compat/ object files
   already are.

 - The inclusion of <zlib.h> is done in <git-compat-util.h>; we used
   to do so from <cache.h> which includes <git-compat-util.h> as the
   first thing it does, so from the *.c codes, there is no practical
   change.

 - Beat found a trick used by OpenSSL to avoid making the
   conditionally-compiled object truly empty (apparently because
   they had to deal with compilers that do not want to see an
   effectively empty input file).  Our compat/zlib-uncompress2.c
   file borrows the same trick for portabilty.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So, here is an updated one, still retaining the authorship but
   adjusting for the "no empty source" trick.  v3 seems to fail
   "win+VS build" due to the use of an extra $(ZLIB_COMPAT_OBJS)
   macro, and this iteration, which just uses LIB_OBJS as everybody
   else does, should be sufficient to avoid introducing such an
   issue.

   One thing that I found a bit iffy is the use of "force z_const to
   an empty string before including <zlib.h>".  This trick to work
   around too old a version of zlib (according to Carlo) was used
   only when compat/zlib-uncompress2.c included <zlib.h> via
   <reftable/system.h>, but never done when <cache.h> included
   <zlib.h>, which means that the two parts of the code could have
   been using incompatible definitions of the same structs (many
   struct definitions zlib uses have const members).  I opted to be
   "conservative" and choose to cast away z_const before
   <git-compat-util.> includes <zlib.h>, but we may want to drop it
   to see if anybody screams.

   In any case, I think it is way too late to put this in the final.
   We may want to reroll it once more to reintroduce NO_UNCOMPRESS2
   as an escape hatch for those with zlib.h headers that lie, but
   all of that can wait until the end of the next week.

 Makefile                  |  8 +-------
 cache.h                   |  1 -
 ci/lib.sh                 |  1 -
 compat/zlib-uncompress2.c | 11 ++++++-----
 config.mak.uname          |  7 -------
 configure.ac              | 13 -------------
 git-compat-util.h         | 12 ++++++++++++
 reftable/system.h         | 11 -----------
 8 files changed, 19 insertions(+), 45 deletions(-)

diff --git c/Makefile w/Makefile
index 5580859afd..a32d278700 100644
--- c/Makefile
+++ w/Makefile
@@ -256,8 +256,6 @@ all::
 #
 # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
 #
-# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
-#
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
 #
@@ -862,6 +860,7 @@ LIB_OBJS += commit-reach.o
 LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
+LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
@@ -1726,11 +1725,6 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
 
-ifdef NO_UNCOMPRESS2
-	BASIC_CFLAGS += -DNO_UNCOMPRESS2
-	REFTABLE_OBJS += compat/zlib-uncompress2.o
-endif
-
 ifdef NO_POSIX_GOODIES
 	BASIC_CFLAGS += -DNO_POSIX_GOODIES
 endif
diff --git c/cache.h w/cache.h
index 281f00ab1b..3a0142aa56 100644
--- c/cache.h
+++ w/cache.h
@@ -18,7 +18,6 @@
 #include "repository.h"
 #include "mem-pool.h"
 
-#include <zlib.h>
 typedef struct git_zstream {
 	z_stream z;
 	unsigned long avail_in;
diff --git c/ci/lib.sh w/ci/lib.sh
index 9d28ab50fb..cbc2f8f1ca 100755
--- c/ci/lib.sh
+++ w/ci/lib.sh
@@ -197,7 +197,6 @@ esac
 case "$jobname" in
 linux32)
 	CC=gcc
-	MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1"
 	;;
 linux-musl)
 	CC=gcc
diff --git c/compat/zlib-uncompress2.c w/compat/zlib-uncompress2.c
index 722610b971..77a1b08048 100644
--- c/compat/zlib-uncompress2.c
+++ w/compat/zlib-uncompress2.c
@@ -1,3 +1,6 @@
+#include "git-compat-util.h"
+
+#if ZLIB_VERNUM < 0x1290
 /* taken from zlib's uncompr.c
 
    commit cacf7f1d4e3d44d871b605da3b647f07d718623f
@@ -8,16 +11,11 @@
 
 */
 
-#include "../reftable/system.h"
-#define z_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>
-
 /* clang-format off */
 
 /* ===========================================================================
@@ -93,3 +91,6 @@ int ZEXPORT uncompress2 (
 	   err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR :
 	   err;
 }
+#else
+static void *dummy_variable = &dummy_variable;
+#endif
diff --git c/config.mak.uname w/config.mak.uname
index c48db45106..92ea00c219 100644
--- c/config.mak.uname
+++ w/config.mak.uname
@@ -66,7 +66,6 @@ ifeq ($(uname_S),Linux)
 	# centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7.
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
-		NO_UNCOMPRESS2 = YesPlease
 	endif
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
@@ -266,10 +265,6 @@ ifeq ($(uname_S),FreeBSD)
 	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
-	# Versions < 7.0 need compatibility layer
-	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
-		NO_UNCOMPRESS2 = UnfortunatelyYes
-	endif
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
@@ -525,7 +520,6 @@ ifeq ($(uname_S),Interix)
 	endif
 endif
 ifeq ($(uname_S),Minix)
-	NO_UNCOMPRESS2 = YesPlease
 	NO_IPV6 = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
@@ -581,7 +575,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_UNCOMPRESS2 = YesPlease
 	# Currently libiconv-1.9.1.
 	OLD_ICONV = UnfortunatelyYes
 	NO_REGEX = NeedsStartEnd
diff --git c/configure.ac w/configure.ac
index d60d494ee4..5ee25ec95c 100644
--- c/configure.ac
+++ w/configure.ac
@@ -664,22 +664,9 @@ AC_LINK_IFELSE([ZLIBTEST_SRC],
 	NO_DEFLATE_BOUND=yes])
 LIBS="$old_LIBS"
 
-AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [
-AC_LANG_PROGRAM([#include <zlib.h>],
- [uncompress2(NULL,NULL,NULL,NULL);])])
-AC_MSG_CHECKING([for uncompress2 in -lz])
-old_LIBS="$LIBS"
-LIBS="$LIBS -lz"
-AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC],
-	[AC_MSG_RESULT([yes])],
-	[AC_MSG_RESULT([no])
-	NO_UNCOMPRESS2=yes])
-LIBS="$old_LIBS"
-
 GIT_UNSTASH_FLAGS($ZLIB_PATH)
 
 GIT_CONF_SUBST([NO_DEFLATE_BOUND])
-GIT_CONF_SUBST([NO_UNCOMPRESS2])
 
 #
 # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
diff --git c/git-compat-util.h w/git-compat-util.h
index 1229c8296b..ea111a7b48 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -1386,6 +1386,18 @@ void unleak_memory(const void *ptr, size_t len);
 #define UNLEAK(var) do {} while (0)
 #endif
 
+#define z_const
+#include <zlib.h>
+
+#if ZLIB_VERNUM < 0x1290
+/*
+ * This is uncompress2, which is only available in zlib >= 1.2.9
+ * (released as of early 2017). See compat/zlib-uncompress2.c.
+ */
+int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
+		uLong *sourceLen);
+#endif
+
 /*
  * This include must come after system headers, since it introduces macros that
  * replace system names.
diff --git c/reftable/system.h w/reftable/system.h
index 4907306c0c..18f9207dfe 100644
--- c/reftable/system.h
+++ w/reftable/system.h
@@ -16,17 +16,6 @@ license that can be found in the LICENSE file or at
 #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
-
 int hash_size(uint32_t id);
 
 #endif

Re: [PATCH v4] compat: auto-detect if zlib has uncompress2()

[Ævar Arnfjörð Bjarmason]

On Sun, Jan 23 2022, Junio C Hamano wrote:

> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> We have a copy of uncompress2() implementation in compat/ so that we
> can build with an older version of zlib that lack the function, and
> the build procedure selects if it is used via the NO_UNCOMPRESS2
> $(MAKE) variable.  This is yet another "annoying" knob the porters
> need to tweak on platforms that are not common enough to have the
> default set in the config.mak.uname file.
>
> Ævar came up with an idea to instead ask the system header <zlib.h>
> to decide if we need the compatibility implementation.  We can
> compile and link compat/zlib-uncompress2.c file unconditionally, but
> conditionally hide the implementation behind #if/#endif when zlib
> version is 1.2.9 or newer, and unconditionally archive the resulting
> object file in the libgit.a to be picked up by the linker.
>
> There are a few things to note in the shape of the code base after
> this change:
>
>  - We no longer use NO_UNCOMPRESS2 knob; if the system header
>    <zlib.h> claims a version that is more cent than the library
>    actually is, this would break, but it is easy to add it back when
>    we find such a system.
>
>  - The object file compat/zlib-uncompress2.o is always compiled and
>    archived in libgit.a, just like a few other compat/ object files
>    already are.
>
>  - The inclusion of <zlib.h> is done in <git-compat-util.h>; we used
>    to do so from <cache.h> which includes <git-compat-util.h> as the
>    first thing it does, so from the *.c codes, there is no practical
>    change.
>
>  - Beat found a trick used by OpenSSL to avoid making the
>    conditionally-compiled object truly empty (apparently because
>    they had to deal with compilers that do not want to see an
>    effectively empty input file).  Our compat/zlib-uncompress2.c
>    file borrows the same trick for portabilty.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Beat Bolli <dev+git@drbeat.li>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * So, here is an updated one, still retaining the authorship but
>    adjusting for the "no empty source" trick.  v3 seems to fail
>    "win+VS build" due to the use of an extra $(ZLIB_COMPAT_OBJS)
>    macro, [...]

I hadn't noticed that "win+VS build" issue, but it's another issue with
the CMake.txt scraping of the Makefile. I.e. it scrapes $(LIB_OBJS) with
a regex, and as soon as another variable is used to append to it it
fails. It has special-casing for the $(COMPAT_OBJS) seen in the v3 diff
context.

All of which b.t.w. would be avoided with the approach I suggested in in
https://lore.kernel.org/git/patch-v2-3.3-cd62d8f92d1-20211101T191231Z-avarab@gmail.com/
of having it ask "make" for these values rather than scraping them.

>    and this iteration, which just uses LIB_OBJS as everybody
>    else does, should be sufficient to avoid introducing such an
>    issue.

The change you have here doesn't work because in relying on $(LIB_OBJS)
you broke the linking of libreftable.a, which doesn't link using
$(LIB_OBJS), but requires uncompress2().

I think you didn't test this on a system that doesn't have
uncompress3(). Testing it can be emulated by just naming it
uncompress3() or something, and making the "#if" macro check an "#if 1".

That linking issue can also be fixed, but overall I really don't see the
point of this complexity of insisting that this fallback function must
arrive via a compat object.

The approach I had in the v2 of just including these sources in the
top-level zlib.c just works, without any of the added build system
complexity.

So just taking the v2 would also work (perhaps with the config.mak.uname
changes), or this v4 with compat/zlib-uncompress2.o hardcoded in both
LIB_OBJS and REFTABLE_OBJS, so that the scraping in CMake.txt can
understand it.

>    One thing that I found a bit iffy is the use of "force z_const to
>    an empty string before including <zlib.h>".  This trick to work
>    around too old a version of zlib (according to Carlo) was used
>    only when compat/zlib-uncompress2.c included <zlib.h> via
>    <reftable/system.h>, but never done when <cache.h> included
>    <zlib.h>, which means that the two parts of the code could have
>    been using incompatible definitions of the same structs (many
>    struct definitions zlib uses have const members).  I opted to be
>    "conservative" and choose to cast away z_const before
>    <git-compat-util.> includes <zlib.h>, but we may want to drop it
>    to see if anybody screams.

Weren't any issues with this avoided because we didn't include the
reftable/* sources, but compiled them and then linked libreftable.a?

[PATCH v5] compat: auto-detect if zlib has uncompress2()

[Junio C Hamano]

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

We have a copy of uncompress2() implementation in compat/ so that we
can build with an older version of zlib that lack the function, and
the build procedure selects if it is used via the NO_UNCOMPRESS2
$(MAKE) variable.  This is yet another "annoying" knob the porters
need to tweak on platforms that are not common enough to have the
default set in the config.mak.uname file.

Attempt to instead ask the system header <zlib.h> to decide if we
need the compatibility implementation.  This is a deviation from the
way we have been handling the "compatiblity" features so far, and if
it can be done cleanly enough, it could work as a model for features
that need compatibility definition we discover in the future.  With
that goal in mind, avoid expedient but ugly hacks, like shoving the
code that is conditionally compiled into an unrelated .c file, which
may not work in future cases---instead, take an approach that uses a
file that is independently compiled and stands on its own.

Compile and link compat/zlib-uncompress2.c file unconditionally, but
conditionally hide the implementation behind #if/#endif when zlib
version is 1.2.9 or newer, and unconditionally archive the resulting
object file in the libgit.a to be picked up by the linker.

There are a few things to note in the shape of the code base after
this change:

 - We no longer use NO_UNCOMPRESS2 knob; if the system header
   <zlib.h> claims a version that is more cent than the library
   actually is, this would break, but it is easy to add it back when
   we find such a system.

 - The object file compat/zlib-uncompress2.o is always compiled and
   archived in libgit.a, just like a few other compat/ object files
   already are.

 - The inclusion of <zlib.h> is done in <git-compat-util.h>; we used
   to do so from <cache.h> which includes <git-compat-util.h> as the
   first thing it does, so from the *.c codes, there is no practical
   change.

 - Until objects in libgit.a that is already used gains a reference
   to the function, the reftable code will be the only one that
   wants it, so libgit.a on the linker command line needs to appear
   once more at the end to satisify the mutual dependency.

 - Beat found a trick used by OpenSSL to avoid making the
   conditionally-compiled object truly empty (apparently because
   they had to deal with compilers that do not want to see an
   effectively empty input file).  Our compat/zlib-uncompress2.c
   file borrows the same trick for portabilty.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * With a single-liner update to the Makefile with an updated log
   message that explains the change.  I am not sure if this version
   can become the model of future "compat" support, or we should
   just discard the new approach and use the Makefile macro approach
   that has worked well for all of our existing compat support
   already.

Range-diff against v4:
1:  d26c9073bf ! 1:  63c5753c6f compat: auto-detect if zlib has uncompress2()
    @@ Commit message
         need to tweak on platforms that are not common enough to have the
         default set in the config.mak.uname file.
     
    -    Ævar came up with an idea to instead ask the system header <zlib.h>
    -    to decide if we need the compatibility implementation.  We can
    -    compile and link compat/zlib-uncompress2.c file unconditionally, but
    +    Attempt to instead ask the system header <zlib.h> to decide if we
    +    need the compatibility implementation.  This is a deviation from the
    +    way we have been handling the "compatiblity" features so far, and if
    +    it can be done cleanly enough, it could work as a model for features
    +    that need compatibility definition we discover in the future.  With
    +    that goal in mind, avoid expedient but ugly hacks, like shoving the
    +    code that is conditionally compiled into an unrelated .c file, which
    +    may not work in future cases---instead, take an approach that uses a
    +    file that is independently compiled and stands on its own.
    +
    +    Compile and link compat/zlib-uncompress2.c file unconditionally, but
         conditionally hide the implementation behind #if/#endif when zlib
         version is 1.2.9 or newer, and unconditionally archive the resulting
         object file in the libgit.a to be picked up by the linker.
    @@ Commit message
            first thing it does, so from the *.c codes, there is no practical
            change.
     
    +     - Until objects in libgit.a that is already used gains a reference
    +       to the function, the reftable code will be the only one that
    +       wants it, so libgit.a on the linker command line needs to appear
    +       once more at the end to satisify the mutual dependency.
    +
          - Beat found a trick used by OpenSSL to avoid making the
            conditionally-compiled object truly empty (apparently because
            they had to deal with compilers that do not want to see an
    @@ Makefile: THIRD_PARTY_SOURCES += compat/regex/%
      THIRD_PARTY_SOURCES += sha1dc/%
      
     -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
    ++# xdiff and reftable libs may in turn depend on what is in libgit.a
     +GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
      EXTLIBS =
      

 Makefile                  | 11 +++--------
 cache.h                   |  1 -
 ci/lib.sh                 |  1 -
 compat/zlib-uncompress2.c | 11 ++++++-----
 config.mak.uname          |  7 -------
 configure.ac              | 13 -------------
 git-compat-util.h         | 12 ++++++++++++
 reftable/system.h         | 11 -----------
 8 files changed, 21 insertions(+), 46 deletions(-)

diff --git a/Makefile b/Makefile
index 5580859afd..f194b2afc6 100644
--- a/Makefile
+++ b/Makefile
@@ -256,8 +256,6 @@ all::
 #
 # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
 #
-# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
-#
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
 #
@@ -862,6 +860,7 @@ LIB_OBJS += commit-reach.o
 LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
+LIB_OBJS += compat/zlib-uncompress2.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
@@ -1194,7 +1193,8 @@ THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
+# xdiff and reftable libs may in turn depend on what is in libgit.a
+GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
 EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
@@ -1726,11 +1726,6 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
 
-ifdef NO_UNCOMPRESS2
-	BASIC_CFLAGS += -DNO_UNCOMPRESS2
-	REFTABLE_OBJS += compat/zlib-uncompress2.o
-endif
-
 ifdef NO_POSIX_GOODIES
 	BASIC_CFLAGS += -DNO_POSIX_GOODIES
 endif
diff --git a/cache.h b/cache.h
index 281f00ab1b..3a0142aa56 100644
--- a/cache.h
+++ b/cache.h
@@ -18,7 +18,6 @@
 #include "repository.h"
 #include "mem-pool.h"
 
-#include <zlib.h>
 typedef struct git_zstream {
 	z_stream z;
 	unsigned long avail_in;
diff --git a/ci/lib.sh b/ci/lib.sh
index 9d28ab50fb..cbc2f8f1ca 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -197,7 +197,6 @@ esac
 case "$jobname" in
 linux32)
 	CC=gcc
-	MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1"
 	;;
 linux-musl)
 	CC=gcc
diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
index 722610b971..77a1b08048 100644
--- a/compat/zlib-uncompress2.c
+++ b/compat/zlib-uncompress2.c
@@ -1,3 +1,6 @@
+#include "git-compat-util.h"
+
+#if ZLIB_VERNUM < 0x1290
 /* taken from zlib's uncompr.c
 
    commit cacf7f1d4e3d44d871b605da3b647f07d718623f
@@ -8,16 +11,11 @@
 
 */
 
-#include "../reftable/system.h"
-#define z_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>
-
 /* clang-format off */
 
 /* ===========================================================================
@@ -93,3 +91,6 @@ int ZEXPORT uncompress2 (
 	   err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR :
 	   err;
 }
+#else
+static void *dummy_variable = &dummy_variable;
+#endif
diff --git a/config.mak.uname b/config.mak.uname
index c48db45106..92ea00c219 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -66,7 +66,6 @@ ifeq ($(uname_S),Linux)
 	# centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7.
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
-		NO_UNCOMPRESS2 = YesPlease
 	endif
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
@@ -266,10 +265,6 @@ ifeq ($(uname_S),FreeBSD)
 	FILENO_IS_A_MACRO = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
-	# Versions < 7.0 need compatibility layer
-	ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2)
-		NO_UNCOMPRESS2 = UnfortunatelyYes
-	endif
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
@@ -525,7 +520,6 @@ ifeq ($(uname_S),Interix)
 	endif
 endif
 ifeq ($(uname_S),Minix)
-	NO_UNCOMPRESS2 = YesPlease
 	NO_IPV6 = YesPlease
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
 	NO_NSEC = YesPlease
@@ -581,7 +575,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
 	NO_SETENV = YesPlease
 	NO_UNSETENV = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_UNCOMPRESS2 = YesPlease
 	# Currently libiconv-1.9.1.
 	OLD_ICONV = UnfortunatelyYes
 	NO_REGEX = NeedsStartEnd
diff --git a/configure.ac b/configure.ac
index d60d494ee4..5ee25ec95c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -664,22 +664,9 @@ AC_LINK_IFELSE([ZLIBTEST_SRC],
 	NO_DEFLATE_BOUND=yes])
 LIBS="$old_LIBS"
 
-AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [
-AC_LANG_PROGRAM([#include <zlib.h>],
- [uncompress2(NULL,NULL,NULL,NULL);])])
-AC_MSG_CHECKING([for uncompress2 in -lz])
-old_LIBS="$LIBS"
-LIBS="$LIBS -lz"
-AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC],
-	[AC_MSG_RESULT([yes])],
-	[AC_MSG_RESULT([no])
-	NO_UNCOMPRESS2=yes])
-LIBS="$old_LIBS"
-
 GIT_UNSTASH_FLAGS($ZLIB_PATH)
 
 GIT_CONF_SUBST([NO_DEFLATE_BOUND])
-GIT_CONF_SUBST([NO_UNCOMPRESS2])
 
 #
 # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
diff --git a/git-compat-util.h b/git-compat-util.h
index 1229c8296b..ea111a7b48 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1386,6 +1386,18 @@ void unleak_memory(const void *ptr, size_t len);
 #define UNLEAK(var) do {} while (0)
 #endif
 
+#define z_const
+#include <zlib.h>
+
+#if ZLIB_VERNUM < 0x1290
+/*
+ * This is uncompress2, which is only available in zlib >= 1.2.9
+ * (released as of early 2017). See compat/zlib-uncompress2.c.
+ */
+int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
+		uLong *sourceLen);
+#endif
+
 /*
  * This include must come after system headers, since it introduces macros that
  * replace system names.
diff --git a/reftable/system.h b/reftable/system.h
index 4907306c0c..18f9207dfe 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -16,17 +16,6 @@ license that can be found in the LICENSE file or at
 #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
-
 int hash_size(uint32_t id);
 
 #endif
-- 
2.35.0-155-g0eb5153edc

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 24 2022, Junio C Hamano wrote:

> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> [...]
> Attempt to instead ask the system header <zlib.h> to decide if we
> need the compatibility implementation.  This is a deviation from the
> way we have been handling the "compatiblity" features so far, and if
> it can be done cleanly enough, it could work as a model for features
> that need compatibility definition we discover in the future.  With
> that goal in mind, avoid expedient but ugly hacks, like shoving the
> code that is conditionally compiled into an unrelated .c file, which
> may not work in future cases---instead, take an approach that uses a
> file that is independently compiled and stands on its own.
> [...]

I think so, we do auto-probes on version (e.g. for libcurl and libpcre),
but I don't think we've outright defined some missing functions on that
basis, we have defined some missing macros though.

FWIW I did some experiments the other day with handling this via the
Makefile.

I.e. similar to how we generate the .depend/* dirs to optionally inject
the building of various "configure" programs into the build step, so
that e.g. git.c would depend on trying to compile a probe/uncompress2.c
or whatever. We'd then write NO_UNCOMPRESS2=$(FOUND_IT) to a file we'd
"include".

I think it's a generally nice thing to move towards, is relatively
light, and would eventually allow us to get rid of the optional
configure.ac (it would become redundant). We could also use it for
e.g. a more accurate "detect-compiler" shellscript (instead of trying to
guess from clang/gcc versions).

>  - Beat found a trick used by OpenSSL to avoid making the
>    conditionally-compiled object truly empty (apparently because
>    they had to deal with compilers that do not want to see an
>    effectively empty input file).  Our compat/zlib-uncompress2.c
>    file borrows the same trick for portabilty.

Aside: I have not yet found such a compiler, does anyone know of one
that breaks? In any case doing this for good measure seems fine, just
wondering if we're cargo-culting a needless workaround or not.

>  * With a single-liner update to the Makefile with an updated log
>    message that explains the change.  I am not sure if this version
>    can become the model of future "compat" support, or we should
>    just discard the new approach and use the Makefile macro approach
>    that has worked well for all of our existing compat support
>    already.
> [...]
>      -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
>     ++# xdiff and reftable libs may in turn depend on what is in libgit.a
>      +GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
>       EXTLIBS =

As noted I'm rather "who cares? :)" on the question of how we get the
uncompress2() compatibility function over to its eventual location
(directly or via *.o linker indirection), but this approch in v5 works &
addresses the issue I noted in v4. So this looks good to me!

FWIW I've tested this with this ad-hoc patch to simulate not having
uncompress2, and on systems that genuinely don't have it:
    
    diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
    index 77a1b080484..539c14c2b54 100644
    --- a/compat/zlib-uncompress2.c
    +++ b/compat/zlib-uncompress2.c
    @@ -3 +3 @@
    -#if ZLIB_VERNUM < 0x1290
    +#if 1
    @@ -37 +37 @@
    -int ZEXPORT uncompress2 (
    +int ZEXPORT uncompress3 (
    diff --git a/git-compat-util.h b/git-compat-util.h
    index ea111a7b481..a6df48d76d5 100644
    --- a/git-compat-util.h
    +++ b/git-compat-util.h
    @@ -1392 +1392 @@ void unleak_memory(const void *ptr, size_t len);
    -#if ZLIB_VERNUM < 0x1290
    +#if 1
    @@ -1397 +1397 @@ void unleak_memory(const void *ptr, size_t len);
    -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
    +int uncompress3(Bytef *dest, uLongf *destLen, const Bytef *source,
    diff --git a/reftable/block.c b/reftable/block.c
    index 855e3f5c947..ee341eb65fe 100644
    --- a/reftable/block.c
    +++ b/reftable/block.c
    @@ -213 +213 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
    -		    uncompress2(uncompressed + block_header_skip, &dst_len,
    +		    uncompress3(uncompressed + block_header_skip, &dst_len,
    

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Aside: I have not yet found such a compiler, does anyone know of one
> that breaks? In any case doing this for good measure seems fine, just
> wondering if we're cargo-culting a needless workaround or not.

Before I started Git, I had to deal with quite a many variations of
UNIX, all of which looked alike but behaved slightly differently,
and I do recall seeing this exact breakage, so it is a real solution
to a real problem, and I can see OpenSSL folks had seen the same one.

If you find my experience is not Enough, I have no further words for
you on this topic.

If the question is "name a compiler that breaks and is *still* in
active use", then the answer would be fuzzy (it depends on the
definition of "in active use"), but is useful to find out.

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Carlo Arenas]

On Mon, Jan 24, 2022 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> If the question is "name a compiler that breaks and is *still* in
> active use", then the answer would be fuzzy (it depends on the
> definition of "in active use"), but is useful to find out.

`gcc -pedantic -werror` will abort the build (ISO C forbids an empty
translation unit) because an empty translation unit is not
syntactically correct code per ISO, as well as clang (ISO C requires a
translation unit to contain at least one declaration
[-Wempty-translation-unit]).

Carlo

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Mon, Jan 24, 2022 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If the question is "name a compiler that breaks and is *still* in
>> active use", then the answer would be fuzzy (it depends on the
>> definition of "in active use"), but is useful to find out.
>
> `gcc -pedantic -werror` will abort the build (ISO C forbids an empty
> translation unit) because an empty translation unit is not
> syntactically correct code per ISO, as well as clang (ISO C requires a
> translation unit to contain at least one declaration
> [-Wempty-translation-unit]).

Ah, I remember that one, and it cleanly concludes the thread.
Thanks.

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 24 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Aside: I have not yet found such a compiler, does anyone know of one
>> that breaks? In any case doing this for good measure seems fine, just
>> wondering if we're cargo-culting a needless workaround or not.
>
> Before I started Git, I had to deal with quite a many variations of
> UNIX, all of which looked alike but behaved slightly differently,
> and I do recall seeing this exact breakage, so it is a real solution
> to a real problem, and I can see OpenSSL folks had seen the same one.
>
> If you find my experience is not Enough, I have no further words for
> you on this topic.

I wasn't alleging that this issue was a figment of someone's
imagination, but probing for whether it was a current issue or
not. I.e. it could have been something that only mattered in the GCC 2.x
era, and OpenSSL was still carrying.

But as Carlo notes downthread it's to do with ISO C strictness and
-Wempty-translation-unit. So we'd have caught it under DEVELOPER=1, but
due to the recent over-strictness of DEVELOPER I'd disabled it on the
test boxes involved, so I didn't spot that.

> If the question is "name a compiler that breaks and is *still* in
> active use", then the answer would be fuzzy (it depends on the
> definition of "in active use"), but is useful to find out.

But as to how to deal with it this is good enough for now, but perhaps
we'll consider something like this for a future compat/*.c addition:
    
    $ file compat/blah.c
    compat/blah.c: empty
    $ diff --git a/Makefile b/Makefile
    index 5da099e8a16..62ec13c72fd 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -571 +571 @@ COMPAT_CFLAGS =
    -COMPAT_OBJS =
    +COMPAT_OBJS = compat/blah.o
    @@ -2604,0 +2605 @@ endif
    +compat/blah.o: EXTRA_CPPFLAGS += -Wno-empty-translation-unit

That will compile cleanly on GCC and Clang with DEVELOPER=1, which of
course may leave some other strictly ISO C compiler unsatisfied.

Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> That will compile cleanly on GCC and Clang with DEVELOPER=1, which of
> course may leave some other strictly ISO C compiler unsatisfied.

I agree with you that, compared to the solution we borrow from
OpenSSL, it would be of limited utility.  Let's not go there.