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

[Junio C Hamano <gitster@pobox.com>]

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