Re: [PATCH] cache.h: auto-detect if zlib has uncompress2()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 17, 2022, 18:27 UTC
- Message-ID
- <xmqq7dayw732.fsf@gitster.g>
- In-Reply-To
- <patch-1.1-9cea01a1395-20220117T170457Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 13 quoted lines
> 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.
Show 5 quoted lines
> @@ -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).
Show 13 quoted lines
> 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);
> +#endifI 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 *);
Show 9 quoted lines
> 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".
Show 7 quoted lines
> /* > * 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 ;-)
Show 10 quoted lines
> 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.
Show 20 quoted lines
> 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.
Show 11 quoted lines
> 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.