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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 24, 2024, 07:58 UTC
Message-ID: <xmqqv86eqonh.fsf@gitster.g>
In-Reply-To: <CAO_smVh6PyxbnXfo0K1aDjEFPc3jTF4X_grerkxNZJdQe8V3sg@mail.gmail.com>

Kyle Lippincott <spectral@google.com> writes:

Show 7 quoted lines

> As far as I can tell, we need pager.h because of the `pager_in_use`
> symbol. We need that symbol because of its use in date.c's
> `parse_date_format`. I wonder if we can side step the `#include
> <stdint.h>` concerns by splitting pager.h into pager.h and
> pager_in_use.h, and have pager.h include pager_in_use.h instead. This
> way pager.h (and its [unused] forward declarations) aren't part of
> git-std-lib at all.

Step back a bit. Why do you even need to touch pager.h in the first place? Whatever thing that needs to define a mock version of pager_in_use() would need to be able to find out that it is supposed to take nothing as arguments and return an integer, and it can include <pager.h> without modification. Just like everybody else, it has to include <git-compat-util.h> so that the system header that gives us uintmax_t gets include appropriately in platform-dependent way, no? Why do we even need to butcher pager.h into two pieces in the first place?

If you just include <git-compat-util.h> and then <pager.h> in stubs/pager.c and you're OK, no?

If anything, as I already said, I think it is more reasonable to tweak what <git-compat-util.h> does. For example, it might be unwieldy for gitstdlib's purpose that it unconditionally overrides exit(), in which case it may be OK to introduce some conditional compilation macros to omit that override when building stub code. Or even split parts of the <git-compat-util.h> that both Git's use and gitstdlib's purpose are OK with into a separate header file <git-compat-core.h>, while leaving (hopefully a very minor) other parts in <git-compat-util.h> *and* include <git-compat-core.h> in <git-compat-util.h>. That way, the sources of Git can continue including <git-compat-util.h> while stub code can include <git-compat-core.h>, and we will get system library symbols and system defined types like uintmax_t in a consistent way, both in Git itself and in gitstdlib.

But once such a sanitization is done on the compat-util header, other "ordinary" header files that should not have to care about portability (because they can assume that inclusion of git-compat-util.h will give them access to system types and symbols without having to worry about portability issues) and should not have to include system header files themselves.

At least, that is the idea behind <git-compat-util.h> in the first place. Including any system headers directly in ordinary headers, or splitting ordinary headers at an arbitrary and artificial boundary, should not be necessary. I'd have to say that such changes are tail wagging the dog.

I do not have sufficient cycles to spend actually splitting git-compat-util.h into two myself, but as an illustration, here is how I would tweak cw/git-std-lib topic to make it build without breaking our headers and including system header files directly.

 git-compat-util.h | 2 ++
 pager.h           | 2 --
 stubs/misc.c      | 4 ++--
 stubs/pager.c     | 1 +
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git c/git-compat-util.h w/git-compat-util.h
index 7c2a6538e5..981d526d18 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -1475,12 +1475,14 @@ static inline int is_missing_file_error(int errno_)
 
 int cmd_main(int, const char **);
 
+#ifndef _GIT_NO_OVERRIDE_EXIT
 /*
  * Intercept all calls to exit() and route them to trace2 to
  * optionally emit a message before calling the real exit().
  */
 int common_exit(const char *file, int line, int code);
 #define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
+#endif
 
 /*
  * You can mark a stack variable with UNLEAK(var) to avoid it being
diff --git c/pager.h w/pager.h
index 015bca95e3..b77433026d 100644
--- c/pager.h
+++ w/pager.h
@@ -1,8 +1,6 @@
 #ifndef PAGER_H
 #define PAGER_H
 
-#include <stdint.h>
-
 struct child_process;
 
 const char *git_pager(int stdout_is_tty);
diff --git c/stubs/misc.c w/stubs/misc.c
index 8d80581e39..d0379dcb69 100644
--- c/stubs/misc.c
+++ w/stubs/misc.c
@@ -1,5 +1,5 @@
-#include <assert.h>
-#include <stdlib.h>
+#define _GIT_NO_OVERRIDE_EXIT
+#include <git-compat-util.h>
 
 #ifndef NO_GETTEXT
 /*
diff --git c/stubs/pager.c w/stubs/pager.c
index 4f575cada7..04517aad4c 100644
--- c/stubs/pager.c
+++ w/stubs/pager.c
@@ -1,3 +1,4 @@
+#include <git-compat-util.h>
 #include "pager.h"
 
 int pager_in_use(void)