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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2024, 21:43 UTC
Message-ID: <xmqqh6i0cgyo.fsf@gitster.g>
In-Reply-To: <20240222175033.1489723-2-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 26 quoted lines

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