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

[Kyle Lippincott <spectral@google.com>]

On Mon, Feb 26, 2024 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> >> In any case, your sources should not include a standard library
> >> header directly yourself, period.  Instead let <git-compat-util.h>
> >> take care of the details of how we need to obtain what we need out
> >> of the system on various platforms.
> >
> > I disagree with this statement. We _can't_ use a magic compatibility
> > header file in the library interfaces, for the reasons I outlined
> > further below in my previous message. For those headers, the ones that
> > might be included by code that's not under the Git project's control,
> > they need to be self-contained, minimal, and maximally compatible.
>
> Note that I am not talking about your random outside program that
> happens to link with gitstdlib.a; it would want to include a header
> file <gitstdlib.h> that comes with the library.

I agree with this.

>
> Earlier I suggested that you may want to take a subset of
> <git-compat-util.h>, because <git-compat-util.h> may have a lot more
> than what is minimally necessary to allow our sources to be
> insulated from details of platform dependence.  You can think of
> that subset as a good starting point to build the <gitstdlib.h>
> header file to be given to the library customers.
>
> But the sources that go to the library, as gitstdlib.a is supposed
> to serve as a subset of gitlib.a to our internal codebase when
> building the git binary, should still follow our header inclusion
> rules.

If I'm understanding this correctly, I agree with it. The .c files
still include <git-compat-util.h>, and don't change. The internal-only
.h files (ones that a pre-built-library consumer doesn't need to even
have in the filesystem) still assume that <git-compat-util.h> was
included, and don't change. <pager.h> falls into this category.

>
> Because we would want to make sure that the sources that are made
> into gitstdlib.a, the sources to the rest of libgit.a, and the
> sources to the rest of git, all agree on what system features we ask
> from the system, feature macros that must be defined to certain
> values before we include system library files (like _XOPEN_SOURCE
> and _FILE_OFFSET_BITS) must be defined consistently across all of
> these three pieces.  One way to do so may be to ensure that the
> definition of them would be migrated to <gitstdlib.h> when we
> separate a subset out of <git-compat-util.h> to it (and of course,
> we make <git-compat-util.h> to include <gitstdlib.h> so that it
> would be still sufficient for our in-tree users to include the
> <git-compat-util.h>)
>
> <gitstdlib.h> may have to expose an API function that uses some
> extended types only available by including system header files,
> e.g. some function may return ssize_t as its value or take an off_t
> value as its argument.

I agree that these types will be necessary (specifically ssize_t and
int##_t, but less so off_t) in the "external" (used by projects other
than Git) library interfaces.

>
> If our header should include system headers to make these types
> available to our definitions is probably open to discussion.  It is
> harder to do so portably, unless your world is limited to POSIX.1
> and ISO C, than making it the responsibility of library users.

I think I'm probably missing the nuance here, and may be making this
discussion much harder because of it. My understanding is that Git is
using C99; is that different from ISO C? There's something at the top
of <git-compat-util.h> that enforces that we're using C99. Therefore,
I'm assuming that any compiler that claims to be C99 and passes that
check at the top of <git-compat-util.h> will support inttypes.h,
stdint.h, stdbool.h, and other files defined by the C99 standard to
include types that we need in our .h files are able to be included
without reservation. To flip it around: any compiler/platform that's
missing inttypes.h, or is missing stdint.h, or raises errors if both
are included, or requires other headers to be included before them
_isn't a C99 compiler_, and _isn't supported_. I'm picking on these
files because I think they will be necessary for the external library
interfaces. I'm intentionally ignoring any file not mentioned in the
C99 standard, because those are platform specific. I acknowledge that
there may be some functionality in these files that's only enabled if
certain #defines are set. Our external interfaces should strive to not
use that functionality, and only do so if we are able to test for this
functionality and refuse to compile if it's not available. I have an
example with uintmax_t below.

>
> But if the platform headers and libraries support feature macros
> that allows you to tweak these sizes (e.g. the size of off_t may be
> controlled by setting the _FILE_OFFSET_BITS to an appropriate
> value), it may be irresponsible to leave that to the library users,
> as they MUST make sure to define such feature macros exactly the
> same way as we define for our code, which currently is done in
> <git-compat-util.h>, before they include their system headers to
> obtain off_t so that they can use <gitstdlib.h>.

I think the only viable solution to this is to not use these types
that depend on #defines in the interface available to non-git
projects. We can't set _FILE_OFFSET_BITS in the library's external
(used by non-Git projects) interface header, as there's a high
likelihood that it's either too late (external project #included
something that relies on _FILE_OFFSET_BITS already), or, if not, we
create the "off_t is a different size" problem for their code.

This means that we can't use off_t in these external interface headers
(and in the .c files that support them, if any). We can't use `struct
stat`. We likely need to limit ourselves to just the typedefs from
stdint.h, and probably will need some additional checks that enforce
that we have the types and sizes we expect (ex: I could imagine that
some platforms define uintmax_t as 32-bit. or 128-bit. Either we can't
use it in these external interfaces, or we have to enforce somehow
that the simplest file we can imagine (#include <stdint.h>) gets a
definition of uintmax_t that is the exact same as the one we'd get if
we included <git-compat-util.h>). The external interface headers don't
need to be as platform-compatible as the rest of the git code base,
because not every platform is going to be a supported target for using
the library in non-git projects, especially at first. The external
interface headers _do_ need to be as tolerant and well behaved as
possible when being included by external projects, which I'm asserting
means they need to be self-contained and minimal. If that means these
external interfaces don't get to use off_t at all, so be it. If it
means they can only be included if sizeof(off_t) == 64, and we have a
way of enforcing that at compile time, that's fine with me too. But we
can't #define _FILE_OFFSET_BITS ourselves in this external interface
to get that behavior, because it just doesn't work.

I'm making some assumptions here. I'm assuming that the git binary
uses a different interface to a hypothetical libgitobjstore.a than an
external project would (i.e. that there'd be some
git-obj-store-interface.h that gets included by non-Git projects, but
not by git itself). Is git-std-lib an obvious counterexample to this
assumption? Yes and no. No one (besides Git itself) is going to
include libgitstdlib.a in their project any time soon, so there's no
real "external interface" to define right now. Eventually, having
git-std-lib types in the hypothetical git-obj-store-interface.h _may_
happen, or it may not. I don't know.

...

But I think we're in agreement that pager.h isn't part of
git-std-lib's (currently undefined/non-existent) external interface,
and so doesn't need to be self-contained, and this patch should
probably be dropped?
>
> So the rules for library clients (random outside programs that
> happen to link with gitstdlib.a) may not be that they must include
> <git-compat-util.h> as the first thing, but they probably still have
> to include <gitstdlib.h> fairly early before including any of their
> system headers, I would suspect, unless they are willing to accept
> such responsibility fully to ensure they compile the same way as the
> gitstdlib library, I would think.
>
>
>