Re: [PATCH 02/16] treewide: remove unnecessary git-compat-util.h includes in headers

[Elijah Newren <newren@gmail.com>]

On Thu, Feb 23, 2023 at 11:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Since git-compat-util.h needs to be included first in C files, having it
> > appear in header files is unnecessary.  More importantly, having it
> > included in header files seems to lead to folks leaving it out of C
> > files, which makes it harder to verify that the rule is being followed.
> > Remove it from header files, other than the ones that have been approved
> > as alternate first includes.
>
> Hmph, doesn't this cut both ways?
>
> I like the idea that the removal of compat-util from other
> header files may increase the likelihood that a C file that includes
> such header files without including compat-util fail to compile,
> because it would fail to find what is defined or declared in
> compat-util.
>
> But from "include what you use" point of view, shouldn't a header
> that defines or declares its own stuff using what is defined or
> declared in compat-util be including compat-util itself?
>
> Or do I misunderstand what "make hdr-check" is trying to achieve?
>
> Granted that the check does not fail with this patch in place, but I
> suspect that it is by accident (i.e. there happens to be nobody who
> depends on what is defined/declared in compat-util for their own
> definition or declaration).  Also I am not sure how to interpret
> the fact that "make hdr-check" succeeds with this patch.  Does it
> mean C files that include these header files while forgetting to
> include compat-util may not be caught by the compiler after all?
>
> So, I dunno.

I did something like that before, and Peff objected; see
https://lore.kernel.org/git/20180811173406.GA9119@sigill.intra.peff.net/
and https://lore.kernel.org/git/20180811174301.GA9287@sigill.intra.peff.net/.

I think for sanity we should do one of the following:

(a) make C and header files both depend upon everything they need
(b) consistently exclude git-compat-util.h from headers and require it
be the first include in C files

I think things get really messy if we let half the headers follow (a)
and the other half are forced to do (b).  I was pushed towards (b)
before, but now that I've worked on this series, I think there is even
more reason to go this direction: this work I did during this series
shows that if we allow a mixture of (a) and (b), then empirically we
end up with C files that don't include git-compat-util.h directly, and
those same C files likely include some headers that don't include
git-compat-util.h at all, and if the other headers are included before
the indirect inclusion of git-compat-util.h then there are risks that
things will break in very subtle ways (as pointed out by Peff in the
above-linked emails).  So, I'm inclined to go towards (b).