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

From: Kyle Lippincott <spectral@google.com>
Date: Feb 27, 2024, 22:29 UTC
Message-ID: <CAO_smVgmaXvyZZ7zp6RCFD_6kpL2pHKC9gMDeg+yXBb9R4rR5w@mail.gmail.com>
In-Reply-To: <xmqqzfvmhbfs.fsf@gitster.g>

On Mon, Feb 26, 2024 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

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

Show 12 quoted lines

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

Show 18 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 11 quoted lines

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