Re: [PATCH 6/6] reftable: introduce "reftable-system.h" header

[Patrick Steinhardt <ps@pks.im>]

On Tue, Mar 31, 2026 at 11:26:35AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We're including a couple of standard headers like <stdint.h> in a bunch
> > of locations, which makes it hard for a project to plug in their own
> > logic for making required functionality available. For us this is for
> > example via "compat/posix.h", which already includes all of the system
> > headers relevant to us.
> 
> Hmmm.  This is interesting.
> 
> > diff --git a/reftable/reftable-system.h b/reftable/reftable-system.h
> > new file mode 100644
> > index 0000000000..f90c415182
> > --- /dev/null
> > +++ b/reftable/reftable-system.h
> > @@ -0,0 +1,7 @@
> > +#ifndef REFTABLE_SYSTEM_H
> > +#define REFTABLE_SYSTEM_H
> > +
> > +#define MINGW_DONT_HANDLE_IN_USE_ERROR
> > +#include "compat/posix.h"
> > +
> > +#endif
> 
> This one is clearly tailored to be used in the context of our
> system.
> 
> > diff --git a/reftable/system.h b/reftable/system.h
> > index dffc717bd4..52f964c04b 100644
> > --- a/reftable/system.h
> > +++ b/reftable/system.h
> > @@ -11,8 +11,7 @@
> >  
> >  /* This header glues the reftable library to the rest of Git */
> >  
> > -#define MINGW_DONT_HANDLE_IN_USE_ERROR
> > -#include "compat/posix.h"
> > +#include "reftable-system.h"
> >  #include "compat/zlib-compat.h"
> >  
> >  #define REFTABLE_INLINE(type) static inline type
> 
> And so far in this series, I was getting the impression that
> reftable/system.c and reftable/system.h are where the target system
> specific definitions are stored.
> 
> The implementation detail of how we obtain the wallclock time at
> millisecond resolution is in reftable/system.c, the implementation
> detail of how our mmap() emulation can work to build reftable_mmap()
> is in reftable/system.c, for example.
> 
> But the corresponding reftable/system.h does not seem to be specific
> to the target system at all---it describes the common abstraction,
> like "reftable code proper is expected call reftable_mmap() on any
> system" and "the way for reftable code is expected to read the
> wallclock is by calling reftable_time_ms()".

It almost isn't. There are a few small parts in here that are specific.
I was also wondering whether I want to try and adapt it so that it can
always remain the exact same.

> So <reftable-system.h>, just like <reftable/system.c>, is expected
> to have a target platform specific "implementation", and not like
> <reftable/system.h> that is expected to be platform neutral (this
> neutrality comes from the fact that <reftable/system.c> will
> implement the interface specified in <reftable/system.h> for the
> target platform).
> 
> Which somehow feels confusing.
> 
> Besides, the definition of "REFTABLE_INLINE(type)" being "static
> inline type", according to the explanation in [1/6], is valid only
> in the context of this project, so shouldn't it be done inside
> <reftable-system.h>, not <reftable/system.h>"?  For that matter,
> what about inclusion of "compat/zlib-compat.h"?  Is it widely
> applicable across target platforms, or very specific to our codebase
> where this library is used/embedded in?

It overall is a tiny bit confusing, agreed. The reftable interfaces are
split into two parts:

  - "reftable/foo.h" contains the library-internal API surface.

  - "reftable/reftable-foo.h" contains the external API surface as it
    should be consumed by the project that embeds the reftable library.

Now for most of the part, headers in the "reftable/reftable-*.h"
namespace are self-contained. But naturally, we also use some types
there that require us to include headers, like `uint32_t` et al.
The requirements that we have here are significantly smaller though than
what we expose via "reftable/system.h".

So ultimately, the idea was to have "reftable/reftable-system.h" expose
the POSIX-like environment that is project-specific to make the other
public headers compile as standalone units. And then have the
implementeation sit in "reftable/system.c". But I agree that
"reftable/system.h" itself still sits somewhere in between of being
platform specific and containing project-specific stuff, which isn't
great.

I'm overall not a 100% happy myself with the split and agree that it's
somewhat confusing. An alternative would be to say that it's the
caller's responsibility to ensure that our public-facing headers have
all dependencies satisfied, which I think is in practice only <stdint.h>.

I'm very open to alternative suggestions though.

Thanks!

Patrick