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

From: Kyle Lippincott <spectral@google.com>
Date: Feb 24, 2024, 22:25 UTC
Message-ID: <CAO_smViGixoV+mWVye5xVhNtC28Yhdvf3Cq4xAXOtF0VKCQEzg@mail.gmail.com>
In-Reply-To: <xmqqwmqtli18.fsf@gitster.g>

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