[PATCH] compat: drop inclusion of <git-compat-util.h>

Junio C Hamano· Feb 24, 2024, 20:32 UTC · lore

These two header files are included from ordinary source files that already include <git-compat-util.h> as the first header file as they should. There is no need to include the compat-util in these headers.

"make hdr-check" is not affected, as it is designed to assume that what <git-compat-util.h> offers is available to everybody without being included.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * There is an obvious alternative that goes in the complete
   opposite direction possible, to update "make hdr-check" to ensure
   that things that are depended upon in each header file
   (e.g. pager.h refers to uintmax_t) are brought in by the header
   file to include the compat-util in it, i.e.

	diff --git c/Makefile w/Makefile
	index 78e874099d..d7b360f15e 100644
	--- c/Makefile
	+++ w/Makefile
	@@ -3259,7 +3259,7 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
	 HCC = $(HCO:hco=hcc)
	 
	 %.hcc: %.h
	-	@echo '#include "git-compat-util.h"' >$@
	+	@echo '/* #include "git-compat-util.h" */' >$@
	 	@echo '#include "$<"' >>$@
	 
	 $(HCO): %.hco: %.hcc FORCE

   which would require in a noisy diff to add inclusion of
   git-compat-util.h to many header files.  For purposes of folks
   who may want to carve out only pieces of our source tree, such an
   approach might work better, but for that to happen and yield any
   useful result, I suspect that compat-util header needs to be
   split into "compatibility essentials" and other "it is convenient
   if these are available everywhere, even though they do not have
   much to do with hiding system dependencies from the sources"
   parts first.

 compat/compiler.h | 1 -
 compat/disk.h     | 1 -
 2 files changed, 2 deletions(-)

Show changes to diff +0 −2

diff --git c/compat/compiler.h w/compat/compiler.h
index 10dbb65937..e9ad9db84f 100644
--- c/compat/compiler.h
+++ w/compat/compiler.h
@@ -1,7 +1,6 @@
 #ifndef COMPILER_H
 #define COMPILER_H
 
-#include "git-compat-util.h"
 #include "strbuf.h"
 
 #ifdef __GLIBC__
diff --git c/compat/disk.h w/compat/disk.h
index 6c979c27d8..23bc1bef86 100644
--- c/compat/disk.h
+++ w/compat/disk.h
@@ -1,7 +1,6 @@
 #ifndef COMPAT_DISK_H
 #define COMPAT_DISK_H
 
-#include "git-compat-util.h"
 #include "abspath.h"
 #include "gettext.h"

Kyle Lippincott· Feb 24, 2024, 22:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] compat: drop inclusion of <git-compat-util.h>

On Sat, Feb 24, 2024 at 12:33 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 46 quoted lines

>
> These two header files are included from ordinary source files that
> already include <git-compat-util.h> as the first header file as they
> should.  There is no need to include the compat-util in these
> headers.
>
> "make hdr-check" is not affected, as it is designed to assume that
> what <git-compat-util.h> offers is available to everybody without
> being included.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * There is an obvious alternative that goes in the complete
>    opposite direction possible, to update "make hdr-check" to ensure
>    that things that are depended upon in each header file
>    (e.g. pager.h refers to uintmax_t) are brought in by the header
>    file to include the compat-util in it, i.e.
>
>         diff --git c/Makefile w/Makefile
>         index 78e874099d..d7b360f15e 100644
>         --- c/Makefile
>         +++ w/Makefile
>         @@ -3259,7 +3259,7 @@ HCO = $(patsubst %.h,%.hco,$(CHK_HDRS))
>          HCC = $(HCO:hco=hcc)
>
>          %.hcc: %.h
>         -       @echo '#include "git-compat-util.h"' >$@
>         +       @echo '/* #include "git-compat-util.h" */' >$@
>                 @echo '#include "$<"' >>$@
>
>          $(HCO): %.hco: %.hcc FORCE
>
>    which would require in a noisy diff to add inclusion of
>    git-compat-util.h to many header files.  For purposes of folks
>    who may want to carve out only pieces of our source tree, such an
>    approach might work better, but for that to happen and yield any
>    useful result, I suspect that compat-util header needs to be
>    split into "compatibility essentials" and other "it is convenient
>    if these are available everywhere, even though they do not have
>    much to do with hiding system dependencies from the sources"
>    parts first.
>
>  compat/compiler.h | 1 -
>  compat/disk.h     | 1 -
>  2 files changed, 2 deletions(-)

LG, thanks. I agree with the direction from this patch: I think we should not be including git-compat-util.h in header files, whether they're "internal only" or part of the "external interface" of a library. It does far too much and makes too many assumptions about when it's being included. I plan on writing up some more thoughts on this on a different thread, but it's taking some time to get those into a shareable state.

Show 26 quoted lines

>
> diff --git c/compat/compiler.h w/compat/compiler.h
> index 10dbb65937..e9ad9db84f 100644
> --- c/compat/compiler.h
> +++ w/compat/compiler.h
> @@ -1,7 +1,6 @@
>  #ifndef COMPILER_H
>  #define COMPILER_H
>
> -#include "git-compat-util.h"
>  #include "strbuf.h"
>
>  #ifdef __GLIBC__
> diff --git c/compat/disk.h w/compat/disk.h
> index 6c979c27d8..23bc1bef86 100644
> --- c/compat/disk.h
> +++ w/compat/disk.h
> @@ -1,7 +1,6 @@
>  #ifndef COMPAT_DISK_H
>  #define COMPAT_DISK_H
>
> -#include "git-compat-util.h"
>  #include "abspath.h"
>  #include "gettext.h"
>
>

patchcompat: drop inclusion of <git-compat-util.h>