Re: [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used

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

Calvin Wan <calvinwan@google.com> writes:

> From: Jonathan Tan <jonathantanmy@google.com>
>
> pager.h uses uintmax_t but does not include stdint.h. Therefore, add
> this include statement.
>
> This was discovered when writing a stub pager.c file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  pager.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/pager.h b/pager.h
> index b77433026d..015bca95e3 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -1,6 +1,8 @@
>  #ifndef PAGER_H
>  #define PAGER_H
>  
> +#include <stdint.h>
> +
>  struct child_process;
>  
>  const char *git_pager(int stdout_is_tty);

This is not going in a sensible direction from our portability
standard's point of view.

The reason why we do not include these system headers directly to
our source files, and instead make it a rule to include
<git-compat-util.h> as the first header instead, is exactly because
there are curiosities in various platforms that Git wants to run on
which system include headers give us the declarations for types and
functions we rely on, in what order they must be included, and after
what feature macros (the ones that give adjustment to what the
system headers do, like _POSIX_C_SOURCE) are defined, etc.

Given that in <git-compat-util.h>, inclusion of <stdint.h> is
conditional behind some #ifdef's, it does not look like a sensible
change.  It is not very likely for <inttypes.h> and <stdint.h> to
declare uintmax_t in an incompatible way, but on a platform where
<git-compat-util.h> decides to include <inttypes.h> and use its
definition of what uintmax_t is, we should follow the same choice
and be consistent.

If there is a feature macro that affects sizes of the integer on a
platform, this patch will break it even more badly.  Perhaps there
is a platform whose C-library header requires you to define a
feature macro to use 64-bit, and we may define that feature macro
in <git-compat-util.h> before including either <inttypes.h> or
<stdint.h>, but by including <stdint.h> directly like the above
patch does, only this file and the sources that include only this
file, refusing to include <git-compat-util.h> as everybody in the
Git source tree should, will end up using different notion of what
the integral type with maximum width is from everybody else.

What this patch _wants_ to do is of course sympathizable, and we
have "make hdr-check" rule to enforce "a header must include the
headers that declare what it uses", except that it lets the header
files being tested assume that the things made available by
including <git-compat-util.h> are always available.

I think a sensible direction to go for libification purposes is to
also make sure that sources that are compiled into gitstdlib.a, and
the headers that makes what is in gitstdlib.a available, include the
<git-compat-util.h> header file.  There may be things declared in
the <git-compat-util.h> header that are _too_ specific to what ought
to be linked into the final "git" binary and unwanted by library
clients that are not "git" binary, and the right way to deal with it
is to split <git-compat-util.h> into two parts, i.e. what makes
system services available like its conditional inclusion of
<stdint.h> vs <inttypes.h>, definition of feature macros, order in
which the current <git-compat-util.h> includes system headers, etc.,
excluding those that made you write this patch to avoid assuming
that the client code would have included <git-compat-util.h> before
<pager.h>, would be the new <git-compat-core.h>.  And everything
else will remain in <git-compat-util.h>, which will include the
<git-compat-core.h>.  The <pager.h> header for library clients would
include <git-compat-core.h> instead, to still allow them to use the
same types as "git" binary itself that way.