[PATCH 0/6] reftable: some more portability improvements

[PATCH 0/6] reftable: some more portability improvements

[Patrick Steinhardt]

Hi,

this patch series contains the last set of portability improvements
that currently sits in the reftable implementation of libgit2. With
these patches merged lbigit2 is able to fully reuse the reftable library
while only having to provide its own system headers.

I've got a test run with libgit2 at [1], the code in Git is tested at
[2]. Overall we're quite close -- the pull requests to implement the
repository extension and to adjust handling of pseudo-refs have been
merged. Still missing is a couple of test fixes, but once those are
merged the reftable backend itself will be in review.

Thanks!

Patrick

[1]: https://github.com/libgit2/libgit2/pull/7117
[2]: https://gitlab.com/gitlab-org/git/-/merge_requests/535

---
Patrick Steinhardt (6):
      reftable/system: provide `REFTABLE_INLINE()` macro
      reftable/stack: don't call fsync(3p) unless provided
      reftable/fsck: use REFTABLE_UNUSED instead of UNUSED
      reftable/system: add abstraction to retrieve time in milliseconds
      reftable/system: add abstraction to mmap files
      reftable: introduce "reftable-system.h" header

 reftable/basics.h               | 20 ++++++++++----------
 reftable/blocksource.c          | 19 +++++++------------
 reftable/fsck.c                 |  2 +-
 reftable/pq.h                   |  4 ++--
 reftable/record.h               |  4 ++--
 reftable/reftable-basics.h      |  2 +-
 reftable/reftable-block.h       |  3 +--
 reftable/reftable-blocksource.h |  2 +-
 reftable/reftable-error.h       |  2 ++
 reftable/reftable-fsck.h        |  1 +
 reftable/reftable-iterator.h    |  1 +
 reftable/reftable-merged.h      |  1 +
 reftable/reftable-record.h      |  2 +-
 reftable/reftable-stack.h       |  1 +
 reftable/reftable-system.h      |  7 +++++++
 reftable/reftable-table.h       |  1 +
 reftable/reftable-writer.h      |  4 +---
 reftable/stack.c                | 29 +++++------------------------
 reftable/system.c               | 26 ++++++++++++++++++++++++++
 reftable/system.h               | 26 ++++++++++++++++++++++++--
 20 files changed, 96 insertions(+), 61 deletions(-)


---
base-commit: 270e10ad6dda3379ea0da7efd11e4fbf2cd7a325
change-id: 20260330-pks-reftable-portability-fixes-36ebf9f227c2

[PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[Patrick Steinhardt]

Not every compiler knows about the `inline` annotation for functions.
Consequently, Git knows to define `inline` as an empty macro in case
it's not available.

In the reftable library though we cannot assume the macro to be
available as it is usable as a standalone library. Fix this by
introducing a `REFTABLE_INLINE()` macro via "reftable/system.h" that
allows the project to use their own definition.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/basics.h | 20 ++++++++++----------
 reftable/pq.h     |  4 ++--
 reftable/record.h |  4 ++--
 reftable/system.h |  2 ++
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/reftable/basics.h b/reftable/basics.h
index e4b83b2b03..ebbcec2ac3 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -75,14 +75,14 @@ char *reftable_buf_detach(struct reftable_buf *buf);
 
 /* Bigendian en/decoding of integers */
 
-static inline void reftable_put_be16(void *out, uint16_t i)
+REFTABLE_INLINE(void) reftable_put_be16(void *out, uint16_t i)
 {
 	unsigned char *p = out;
 	p[0] = (uint8_t)((i >> 8) & 0xff);
 	p[1] = (uint8_t)((i >> 0) & 0xff);
 }
 
-static inline void reftable_put_be24(void *out, uint32_t i)
+REFTABLE_INLINE(void) reftable_put_be24(void *out, uint32_t i)
 {
 	unsigned char *p = out;
 	p[0] = (uint8_t)((i >> 16) & 0xff);
@@ -90,7 +90,7 @@ static inline void reftable_put_be24(void *out, uint32_t i)
 	p[2] = (uint8_t)((i >>  0) & 0xff);
 }
 
-static inline void reftable_put_be32(void *out, uint32_t i)
+REFTABLE_INLINE(void) reftable_put_be32(void *out, uint32_t i)
 {
 	unsigned char *p = out;
 	p[0] = (uint8_t)((i >> 24) & 0xff);
@@ -99,7 +99,7 @@ static inline void reftable_put_be32(void *out, uint32_t i)
 	p[3] = (uint8_t)((i >>  0) & 0xff);
 }
 
-static inline void reftable_put_be64(void *out, uint64_t i)
+REFTABLE_INLINE(void) reftable_put_be64(void *out, uint64_t i)
 {
 	unsigned char *p = out;
 	p[0] = (uint8_t)((i >> 56) & 0xff);
@@ -112,14 +112,14 @@ static inline void reftable_put_be64(void *out, uint64_t i)
 	p[7] = (uint8_t)((i >>  0) & 0xff);
 }
 
-static inline uint16_t reftable_get_be16(const void *in)
+REFTABLE_INLINE(uint16_t) reftable_get_be16(const void *in)
 {
 	const unsigned char *p = in;
 	return (uint16_t)(p[0]) << 8 |
 	       (uint16_t)(p[1]) << 0;
 }
 
-static inline uint32_t reftable_get_be24(const void *in)
+REFTABLE_INLINE(uint32_t) reftable_get_be24(const void *in)
 {
 	const unsigned char *p = in;
 	return (uint32_t)(p[0]) << 16 |
@@ -127,7 +127,7 @@ static inline uint32_t reftable_get_be24(const void *in)
 	       (uint32_t)(p[2]) << 0;
 }
 
-static inline uint32_t reftable_get_be32(const void *in)
+REFTABLE_INLINE(uint32_t) reftable_get_be32(const void *in)
 {
 	const unsigned char *p = in;
 	return (uint32_t)(p[0]) << 24 |
@@ -136,7 +136,7 @@ static inline uint32_t reftable_get_be32(const void *in)
 	       (uint32_t)(p[3]) <<  0;
 }
 
-static inline uint64_t reftable_get_be64(const void *in)
+REFTABLE_INLINE(uint64_t) reftable_get_be64(const void *in)
 {
 	const unsigned char *p = in;
 	return (uint64_t)(p[0]) << 56 |
@@ -187,7 +187,7 @@ void reftable_free(void *p);
 void *reftable_calloc(size_t nelem, size_t elsize);
 char *reftable_strdup(const char *str);
 
-static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out)
+REFTABLE_INLINE(int) reftable_alloc_size(size_t nelem, size_t elsize, size_t *out)
 {
 	if (nelem && elsize > SIZE_MAX / nelem)
 		return -1;
@@ -215,7 +215,7 @@ static inline int reftable_alloc_size(size_t nelem, size_t elsize, size_t *out)
 		} \
 	} while (0)
 
-static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
+REFTABLE_INLINE(void) *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
 					size_t *allocp)
 {
 	void *new_p;
diff --git a/reftable/pq.h b/reftable/pq.h
index 42310670b0..9210ede273 100644
--- a/reftable/pq.h
+++ b/reftable/pq.h
@@ -27,12 +27,12 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
 void merged_iter_pqueue_release(struct merged_iter_pqueue *pq);
 int pq_less(struct pq_entry *a, struct pq_entry *b);
 
-static inline struct pq_entry merged_iter_pqueue_top(struct merged_iter_pqueue pq)
+REFTABLE_INLINE(struct) pq_entry merged_iter_pqueue_top(struct merged_iter_pqueue pq)
 {
 	return pq.heap[0];
 }
 
-static inline int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
+REFTABLE_INLINE(int) merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
 {
 	return pq.len == 0;
 }
diff --git a/reftable/record.h b/reftable/record.h
index 7953f352a3..20c9091371 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -26,7 +26,7 @@ struct string_view {
 };
 
 /* Advance `s.buf` by `n`, and decrease length. */
-static inline void string_view_consume(struct string_view *s, int n)
+REFTABLE_INLINE(void) string_view_consume(struct string_view *s, int n)
 {
 	s->buf += n;
 	s->len -= n;
@@ -147,7 +147,7 @@ int reftable_record_decode(struct reftable_record *rec, struct reftable_buf key,
 			   uint32_t hash_size, struct reftable_buf *scratch);
 int reftable_record_is_deletion(struct reftable_record *rec);
 
-static inline uint8_t reftable_record_type(struct reftable_record *rec)
+REFTABLE_INLINE(uint8_t) reftable_record_type(struct reftable_record *rec)
 {
 	return rec->type;
 }
diff --git a/reftable/system.h b/reftable/system.h
index c54ed4cad6..b15768dbdb 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -15,6 +15,8 @@
 #include "compat/posix.h"
 #include "compat/zlib-compat.h"
 
+#define REFTABLE_INLINE(type) static inline type
+
 /*
  * Return a random 32 bit integer. This function is expected to return
  * pre-seeded data.

-- 
2.53.0.1185.g05d4b7b318.dirty

[PATCH 2/6] reftable/stack: don't call fsync(3p) unless provided

[Patrick Steinhardt]

Users of the reftable library are expected to provide their own function
callback in cases they want to sync(3p) data to disk via the reftable
write options. But if no such function was provided we end up calling
fsync(3p) directly, which may not even be available on some systems.

Drop the call to fsync(3p) and rely on the callback function
exclusively.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 1c9f21dfe1..f9ae832e3a 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -33,7 +33,7 @@ static int stack_fsync(const struct reftable_write_options *opts, int fd)
 {
 	if (opts->fsync)
 		return opts->fsync(fd);
-	return fsync(fd);
+	return 0;
 }
 
 static ssize_t reftable_write_data(int fd, const void *data, size_t size)

-- 
2.53.0.1185.g05d4b7b318.dirty

[PATCH 3/6] reftable/fsck: use REFTABLE_UNUSED instead of UNUSED

[Patrick Steinhardt]

While we have the reftable-specific `REFTABLE_UNUSED` header, we
accidentally introduced a new usage of the Git-specific `UNUSED` header
into the reftable library in 9051638519 (reftable: add code to
facilitate consistency checks, 2025-10-07).

Convert the site to use `REFTABLE_UNUSED`.

Ideally, we'd move the definition of `UNUSED` into "git-compat-util.h"
so that it becomes in accessible to the reftable library. But this is
unfortunately not easily possible as "compat/mingw-posix.h" requires
this macro, and this header is included by "compat/posix.h".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/fsck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reftable/fsck.c b/reftable/fsck.c
index 26b9115b14..8e73fc83f2 100644
--- a/reftable/fsck.c
+++ b/reftable/fsck.c
@@ -63,7 +63,7 @@ static int table_check_name(struct reftable_table *table,
 
 static int table_checks(struct reftable_table *table,
 			reftable_fsck_report_fn report_fn,
-			reftable_fsck_verbose_fn verbose_fn UNUSED,
+			reftable_fsck_verbose_fn verbose_fn REFTABLE_UNUSED,
 			void *cb_data)
 {
 	table_check_fn table_check_fns[] = {

-- 
2.53.0.1185.g05d4b7b318.dirty

[PATCH 4/6] reftable/system: add abstraction to retrieve time in milliseconds

[Patrick Steinhardt]

We directly call gettimeofday(3p), which may not be available on some
platforms. Provide the infrastructure to let projects easily use their
own implementations of this function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c  | 27 ++++-----------------------
 reftable/system.c |  6 ++++++
 reftable/system.h |  3 +++
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index f9ae832e3a..2d2c9b1f84 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -372,45 +372,26 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 	return err;
 }
 
-/* return negative if a before b. */
-static int tv_cmp(struct timeval *a, struct timeval *b)
-{
-	time_t diff = a->tv_sec - b->tv_sec;
-	int udiff = a->tv_usec - b->tv_usec;
-
-	if (diff != 0)
-		return diff;
-
-	return udiff;
-}
-
 static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 					     int reuse_open)
 {
 	char **names = NULL, **names_after = NULL;
-	struct timeval deadline;
+	uint64_t deadline;
 	int64_t delay = 0;
 	int tries = 0, err;
 	int fd = -1;
 
-	err = gettimeofday(&deadline, NULL);
-	if (err < 0)
-		goto out;
-	deadline.tv_sec += 3;
+	deadline = reftable_time_ms() + 3000;
 
 	while (1) {
-		struct timeval now;
-
-		err = gettimeofday(&now, NULL);
-		if (err < 0)
-			goto out;
+		uint64_t now = reftable_time_ms();
 
 		/*
 		 * Only look at deadlines after the first few times. This
 		 * simplifies debugging in GDB.
 		 */
 		tries++;
-		if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
+		if (tries > 3 && now >= deadline)
 			goto out;
 
 		fd = open(st->list_file, O_RDONLY);
diff --git a/reftable/system.c b/reftable/system.c
index 725a25844e..7aecd3859d 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -4,6 +4,7 @@
 #include "basics.h"
 #include "reftable-error.h"
 #include "../lockfile.h"
+#include "../trace.h"
 #include "../tempfile.h"
 
 uint32_t reftable_rand(void)
@@ -131,3 +132,8 @@ int flock_commit(struct reftable_flock *l)
 
 	return 0;
 }
+
+uint64_t reftable_time_ms(void)
+{
+	return getnanotime() / 1000000;
+}
diff --git a/reftable/system.h b/reftable/system.h
index b15768dbdb..6e00cd32a3 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -110,4 +110,7 @@ int flock_release(struct reftable_flock *l);
  */
 int flock_commit(struct reftable_flock *l);
 
+/* Report the time in milliseconds. */
+uint64_t reftable_time_ms(void);
+
 #endif

-- 
2.53.0.1185.g05d4b7b318.dirty

[PATCH 5/6] reftable/system: add abstraction to mmap files

[Patrick Steinhardt]

In our codebase we have a couple of wrappers around mmap(3p) that allow
us to reimplement the syscall on platforms that don't have it natively,
like for example Windows. Other projects that embed the reftable library
may have a different infra though to hook up mmap wrappers, but these
are currently hard to integrate.

Provide the infrastructure to let projects easily define the mmap
interface with a custom struct and custom functions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/blocksource.c | 19 +++++++------------
 reftable/system.c      | 20 ++++++++++++++++++++
 reftable/system.h      | 18 ++++++++++++++++++
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index 573c81287f..7f7441f751 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -93,13 +93,12 @@ void block_source_from_buf(struct reftable_block_source *bs,
 }
 
 struct file_block_source {
-	uint64_t size;
-	unsigned char *data;
+	struct reftable_mmap mmap;
 };
 
 static uint64_t file_size(void *b)
 {
-	return ((struct file_block_source *)b)->size;
+	return ((struct file_block_source *)b)->mmap.size;
 }
 
 static void file_release_data(void *b REFTABLE_UNUSED, struct reftable_block_data *dest REFTABLE_UNUSED)
@@ -109,7 +108,7 @@ static void file_release_data(void *b REFTABLE_UNUSED, struct reftable_block_dat
 static void file_close(void *v)
 {
 	struct file_block_source *b = v;
-	munmap(b->data, b->size);
+	reftable_munmap(&b->mmap);
 	reftable_free(b);
 }
 
@@ -117,8 +116,8 @@ static ssize_t file_read_data(void *v, struct reftable_block_data *dest, uint64_
 			      uint32_t size)
 {
 	struct file_block_source *b = v;
-	assert(off + size <= b->size);
-	dest->data = b->data + off;
+	assert(off + size <= b->mmap.size);
+	dest->data = (unsigned char *) b->mmap.data + off;
 	dest->len = size;
 	return size;
 }
@@ -156,13 +155,9 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
 		goto out;
 	}
 
-	p->size = st.st_size;
-	p->data = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (p->data == MAP_FAILED) {
-		err = REFTABLE_IO_ERROR;
-		p->data = NULL;
+	err = reftable_mmap(&p->mmap, fd, st.st_size);
+	if (err < 0)
 		goto out;
-	}
 
 	assert(!bs->ops);
 	bs->ops = &file_vtable;
diff --git a/reftable/system.c b/reftable/system.c
index 7aecd3859d..a5603f5f61 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -137,3 +137,23 @@ uint64_t reftable_time_ms(void)
 {
 	return getnanotime() / 1000000;
 }
+
+int reftable_mmap(struct reftable_mmap *out, int fd, size_t len)
+{
+	void *data = xmmap_gently(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (data == MAP_FAILED)
+		return REFTABLE_IO_ERROR;
+
+	out->data = data;
+	out->size = len;
+
+	return 0;
+}
+
+int reftable_munmap(struct reftable_mmap *mmap)
+{
+	if (munmap(mmap->data, mmap->size) < 0)
+		return REFTABLE_IO_ERROR;
+	memset(mmap, 0, sizeof(*mmap));
+	return 0;
+}
diff --git a/reftable/system.h b/reftable/system.h
index 6e00cd32a3..dffc717bd4 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -113,4 +113,22 @@ int flock_commit(struct reftable_flock *l);
 /* Report the time in milliseconds. */
 uint64_t reftable_time_ms(void);
 
+struct reftable_mmap {
+       void *data;
+       size_t size;
+       void *priv;
+};
+
+/*
+ * Map the file into memory. Returns 0 on success, a reftable error code on
+ * error.
+ */
+int reftable_mmap(struct reftable_mmap *out, int fd, size_t len);
+
+/*
+ * Unmap the file from memory. Returns 0 on success, a reftable error code on
+ * error.
+ */
+int reftable_munmap(struct reftable_mmap *mmap);
+
 #endif

-- 
2.53.0.1185.g05d4b7b318.dirty

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

[Patrick Steinhardt]

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.

Introduce a new "reftable-system.h" header that allows projects to
provide their own headers.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-basics.h      | 2 +-
 reftable/reftable-block.h       | 3 +--
 reftable/reftable-blocksource.h | 2 +-
 reftable/reftable-error.h       | 2 ++
 reftable/reftable-fsck.h        | 1 +
 reftable/reftable-iterator.h    | 1 +
 reftable/reftable-merged.h      | 1 +
 reftable/reftable-record.h      | 2 +-
 reftable/reftable-stack.h       | 1 +
 reftable/reftable-system.h      | 7 +++++++
 reftable/reftable-table.h       | 1 +
 reftable/reftable-writer.h      | 4 +---
 reftable/system.h               | 3 +--
 13 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/reftable/reftable-basics.h b/reftable/reftable-basics.h
index 6d73f19c85..dc8622682d 100644
--- a/reftable/reftable-basics.h
+++ b/reftable/reftable-basics.h
@@ -9,7 +9,7 @@
 #ifndef REFTABLE_BASICS_H
 #define REFTABLE_BASICS_H
 
-#include <stddef.h>
+#include "reftable-system.h"
 
 /* A buffer that contains arbitrary byte slices. */
 struct reftable_buf {
diff --git a/reftable/reftable-block.h b/reftable/reftable-block.h
index 0b05a8f7e3..94c79b5c58 100644
--- a/reftable/reftable-block.h
+++ b/reftable/reftable-block.h
@@ -9,8 +9,7 @@
 #ifndef REFTABLE_BLOCK_H
 #define REFTABLE_BLOCK_H
 
-#include <stdint.h>
-
+#include "reftable-system.h"
 #include "reftable-basics.h"
 #include "reftable-blocksource.h"
 #include "reftable-iterator.h"
diff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h
index f5ba867bd6..40c1e94646 100644
--- a/reftable/reftable-blocksource.h
+++ b/reftable/reftable-blocksource.h
@@ -9,7 +9,7 @@
 #ifndef REFTABLE_BLOCKSOURCE_H
 #define REFTABLE_BLOCKSOURCE_H
 
-#include <stdint.h>
+#include "reftable-system.h"
 
 /*
  * Generic wrapper for a seekable readable file.
diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h
index d100e0df92..0535e1478b 100644
--- a/reftable/reftable-error.h
+++ b/reftable/reftable-error.h
@@ -9,6 +9,8 @@
 #ifndef REFTABLE_ERROR_H
 #define REFTABLE_ERROR_H
 
+#include "reftable-system.h"
+
 /*
  * Errors in reftable calls are signaled with negative integer return values. 0
  * means success.
diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
index 007a392cf9..340fc7762e 100644
--- a/reftable/reftable-fsck.h
+++ b/reftable/reftable-fsck.h
@@ -1,6 +1,7 @@
 #ifndef REFTABLE_FSCK_H
 #define REFTABLE_FSCK_H
 
+#include "reftable-system.h"
 #include "reftable-stack.h"
 
 enum reftable_fsck_error {
diff --git a/reftable/reftable-iterator.h b/reftable/reftable-iterator.h
index af582028c2..a050cc153b 100644
--- a/reftable/reftable-iterator.h
+++ b/reftable/reftable-iterator.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_ITERATOR_H
 #define REFTABLE_ITERATOR_H
 
+#include "reftable-system.h"
 #include "reftable-record.h"
 
 struct reftable_iterator_vtable;
diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h
index e5af846b32..02a9966835 100644
--- a/reftable/reftable-merged.h
+++ b/reftable/reftable-merged.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_MERGED_H
 #define REFTABLE_MERGED_H
 
+#include "reftable-system.h"
 #include "reftable-iterator.h"
 
 /*
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index 385a74cc86..e18c538238 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -9,8 +9,8 @@
 #ifndef REFTABLE_RECORD_H
 #define REFTABLE_RECORD_H
 
+#include "reftable-system.h"
 #include "reftable-basics.h"
-#include <stdint.h>
 
 /*
  * Basic data types
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index c2415cbc6e..5f7be573fa 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_STACK_H
 #define REFTABLE_STACK_H
 
+#include "reftable-system.h"
 #include "reftable-writer.h"
 
 /*
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
diff --git a/reftable/reftable-table.h b/reftable/reftable-table.h
index 5f935d02e3..d7666b53a1 100644
--- a/reftable/reftable-table.h
+++ b/reftable/reftable-table.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_TABLE_H
 #define REFTABLE_TABLE_H
 
+#include "reftable-system.h"
 #include "reftable-iterator.h"
 #include "reftable-block.h"
 #include "reftable-blocksource.h"
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 1e7003cd69..065dd93dc6 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -9,11 +9,9 @@
 #ifndef REFTABLE_WRITER_H
 #define REFTABLE_WRITER_H
 
+#include "reftable-system.h"
 #include "reftable-record.h"
 
-#include <stdint.h>
-#include <unistd.h> /* ssize_t */
-
 /* Writing single reftables */
 
 /* reftable_write_options sets options for writing a single reftable. */
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

-- 
2.53.0.1185.g05d4b7b318.dirty

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Not every compiler knows about the `inline` annotation for functions.
> Consequently, Git knows to define `inline` as an empty macro in case
> it's not available.
>
> In the reftable library though we cannot assume the macro to be
> available as it is usable as a standalone library. Fix this by
> introducing a `REFTABLE_INLINE()` macro via "reftable/system.h" that
> allows the project to use their own definition.

And our `inline` would be used to define REFTABLE_INLINE() in the
context of this project?  Makes sense to me.

> diff --git a/reftable/system.h b/reftable/system.h
> index c54ed4cad6..b15768dbdb 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -15,6 +15,8 @@
>  #include "compat/posix.h"
>  #include "compat/zlib-compat.h"
>  
> +#define REFTABLE_INLINE(type) static inline type
> +
>  /*
>   * Return a random 32 bit integer. This function is expected to return
>   * pre-seeded data.

Re: [PATCH 2/6] reftable/stack: don't call fsync(3p) unless provided

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Users of the reftable library are expected to provide their own function
> callback in cases they want to sync(3p) data to disk via the reftable
> write options. But if no such function was provided we end up calling
> fsync(3p) directly, which may not even be available on some systems.
>
> Drop the call to fsync(3p) and rely on the callback function
> exclusively.

Hmph, reftable-backend.c seems to do

	refs->write_options.fsync = reftable_be_fsync;

in its _be_init(), so this change is a no-op in the context of our
system, so this may be _safe_, but for a caller that wanted a fsync
to happen, returning to it without doing anything may be a bit
unexpected.  I am wondering if it should be more like

	if (!opts->fsync)
		/* BUG("whoa where is your fsync callback???") */
		reutrn -1;
	return opts->fsync(fd);

instead.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 1c9f21dfe1..f9ae832e3a 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -33,7 +33,7 @@ static int stack_fsync(const struct reftable_write_options *opts, int fd)
>  {
>  	if (opts->fsync)
>  		return opts->fsync(fd);
> -	return fsync(fd);
> +	return 0;
>  }
>  
>  static ssize_t reftable_write_data(int fd, const void *data, size_t size)

Re: [PATCH 3/6] reftable/fsck: use REFTABLE_UNUSED instead of UNUSED

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

Patrick Steinhardt <ps@pks.im> writes:

> While we have the reftable-specific `REFTABLE_UNUSED` header, we
> accidentally introduced a new usage of the Git-specific `UNUSED` header
> into the reftable library in 9051638519 (reftable: add code to
> facilitate consistency checks, 2025-10-07).
>
> Convert the site to use `REFTABLE_UNUSED`.

Good eyes.  Being self-contained is good.

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

[Junio C Hamano]

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()".

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?

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[René Scharfe]

On 3/31/26 1:26 PM, Patrick Steinhardt wrote:
> Not every compiler knows about the `inline` annotation for functions.
> Consequently, Git knows to define `inline` as an empty macro in case
> it's not available.

Does it?  Only in compat/regex/regex_internal.h, which does not leak
to other code, no?

René

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 3/31/26 1:26 PM, Patrick Steinhardt wrote:
>> Not every compiler knows about the `inline` annotation for functions.
>> Consequently, Git knows to define `inline` as an empty macro in case
>> it's not available.
>
> Does it?  Only in compat/regex/regex_internal.h, which does not leak
> to other code, no?

As we also do 

ifneq (,$(INLINE))
        BASIC_CFLAGS += -Dinline=$(INLINE)
endif

in the Makefile so we cannot tell what people do with their
config.mak ;-).

And obviously other projects do not share our Makefile, so it is not
too much of stretch to say Git "knows to define", even though it may
be more precise to say "knows to let users redefine", perhaps?

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On 2026-03-31 at 11:26:47, Patrick Steinhardt wrote:
> Not every compiler knows about the `inline` annotation for functions.
> Consequently, Git knows to define `inline` as an empty macro in case
> it's not available.

I thought `inline` was in C99, which would mean that it's been required
in C for over 26 years old—it's older than some of my colleagues.  What
compilers are people using in 2026 that don't know about `inline`?  Or
more importantly, what platforms are people using in 2026 that lack a
usable compiler with at least C99?
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[René Scharfe]

On 3/31/26 11:23 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> On 3/31/26 1:26 PM, Patrick Steinhardt wrote:
>>> Not every compiler knows about the `inline` annotation for functions.
>>> Consequently, Git knows to define `inline` as an empty macro in case
>>> it's not available.
>>
>> Does it?  Only in compat/regex/regex_internal.h, which does not leak
>> to other code, no?
> 
> As we also do 
> 
> ifneq (,$(INLINE))
>         BASIC_CFLAGS += -Dinline=$(INLINE)
> endif
> 
> in the Makefile so we cannot tell what people do with their
> config.mak ;-).

Ah, missed that.

So setting INLINE to an empty string does nothing.  We do that for an
ancient versions of HP-UX in config.mak.uname.

When I set it to '', like we do in config.mak.uname for an ancient
version of AIX, I get lots of warnings about unused functions and
linking errors due to duplicate symbols.  I can only hope that the
pre-C99 compilers targeted by this measure can better deal with that
issue somehow.

> And obviously other projects do not share our Makefile, so it is not
> too much of stretch to say Git "knows to define", even though it may
> be more precise to say "knows to let users redefine", perhaps?

Fair enough.

René

Re: [PATCH 1/6] reftable/system: provide `REFTABLE_INLINE()` macro

[Patrick Steinhardt]

On Tue, Mar 31, 2026 at 10:08:17PM +0000, brian m. carlson wrote:
> On 2026-03-31 at 11:26:47, Patrick Steinhardt wrote:
> > Not every compiler knows about the `inline` annotation for functions.
> > Consequently, Git knows to define `inline` as an empty macro in case
> > it's not available.
> 
> I thought `inline` was in C99, which would mean that it's been required
> in C for over 26 years old—it's older than some of my colleagues.  What
> compilers are people using in 2026 that don't know about `inline`?  Or
> more importantly, what platforms are people using in 2026 that lack a
> usable compiler with at least C99?

Surprisingly there are still projects out there that explicitly use C90,
and libgit2 is one of them. it prouds itself with still compiling on
Amiga OS. Whether it actually does may be a different question though.
I kind of doubt it.

Patrick

Re: [PATCH 2/6] reftable/stack: don't call fsync(3p) unless provided

[Patrick Steinhardt]

On Tue, Mar 31, 2026 at 11:09:31AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Users of the reftable library are expected to provide their own function
> > callback in cases they want to sync(3p) data to disk via the reftable
> > write options. But if no such function was provided we end up calling
> > fsync(3p) directly, which may not even be available on some systems.
> >
> > Drop the call to fsync(3p) and rely on the callback function
> > exclusively.
> 
> Hmph, reftable-backend.c seems to do
> 
> 	refs->write_options.fsync = reftable_be_fsync;
> 
> in its _be_init(), so this change is a no-op in the context of our
> system, so this may be _safe_, but for a caller that wanted a fsync
> to happen, returning to it without doing anything may be a bit
> unexpected.  I am wondering if it should be more like
> 
> 	if (!opts->fsync)
> 		/* BUG("whoa where is your fsync callback???") */
> 		reutrn -1;
> 	return opts->fsync(fd);
> 
> instead.

Hm, I guess that's a fair concern. Anyone who doesn't want to fsync can
just provide a no-op function, so it's easy enough for others to stub
out while being safe by default.

I guess the only question in that case is whether it even makes sense to
specify this as an option, or whether we'd rather want to make this part
of the "system.{c,h}" interface, as well.

Patrick

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

[Patrick Steinhardt]

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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

So your assessment starts as "tiny bit" and in the end ends ujp with
"somewhat" confusing ;-)?

I am OK with two level:

 - A C source file that will be customized for platform and the
   project that embeds the library to implement a neutral interface.

 - A C header file that defines what that neutral interface looks
   like, without having any platform/project specific implementation
   details.

But that header file, in order to define an interface in a neutral
way, would need to be able to see common types and things like
"inline", so it is inevitable to have the third thing that is a
header file that will be customzed for platform and the project that
embeds the library.

And in that context making reftable/system.c in this series the C
source that implements the neutral API for the platform and the
project, reftable/system.h the C header that declares the neutral
API, and reftable-system.h the platform specific shim to show a
common definition to reftable/system.h, may be a good division of
labor.  So, while I found the _naming_ confusing initially, at least
between reftable/system.[ch], I no longer see the naming of the
files a problem.

The division of labor is not quite honored in the current
implementation, though, as I pointed out that just like MINGW
specific things that are moved out of reftable/system.h and to the
reftable-system.h, other things like inline and compat/zlib-compat.h
are quite specific to our project and belong to reftable-system.h at
the layer that exposes a certain system services to reftable/system.h
and other "more common" parts of the library.

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

[Patrick Steinhardt]

On Wed, Apr 01, 2026 at 09:37:11AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> 
> So your assessment starts as "tiny bit" and in the end ends ujp with
> "somewhat" confusing ;-)?
> 
> I am OK with two level:
> 
>  - A C source file that will be customized for platform and the
>    project that embeds the library to implement a neutral interface.
> 
>  - A C header file that defines what that neutral interface looks
>    like, without having any platform/project specific implementation
>    details.
> 
> But that header file, in order to define an interface in a neutral
> way, would need to be able to see common types and things like
> "inline", so it is inevitable to have the third thing that is a
> header file that will be customzed for platform and the project that
> embeds the library.
> 
> And in that context making reftable/system.c in this series the C
> source that implements the neutral API for the platform and the
> project, reftable/system.h the C header that declares the neutral
> API, and reftable-system.h the platform specific shim to show a
> common definition to reftable/system.h, may be a good division of
> labor.  So, while I found the _naming_ confusing initially, at least
> between reftable/system.[ch], I no longer see the naming of the
> files a problem.
> 
> The division of labor is not quite honored in the current
> implementation, though, as I pointed out that just like MINGW
> specific things that are moved out of reftable/system.h and to the
> reftable-system.h, other things like inline and compat/zlib-compat.h
> are quite specific to our project and belong to reftable-system.h at
> the layer that exposes a certain system services to reftable/system.h
> and other "more common" parts of the library.

Makes sense, thanks for your input! Will send a new version soonish.

Patrick

[PATCH v2 0/5] reftable: some more portability improvements

[Patrick Steinhardt]

Hi,

this patch series contains the last set of portability improvements
that currently sits in the reftable implementation of libgit2. With
these patches merged lbigit2 is able to fully reuse the reftable library
while only having to provide its own system headers.

I've got a test run with libgit2 at [1], the code in Git is tested at
[2]. Overall we're quite close -- the pull requests to implement the
repository extension and to adjust handling of pseudo-refs have been
merged. Still missing is a couple of test fixes, but once those are
merged the reftable backend itself will be in review.

Changes in v2:
  - Reorder commits a bit so that we introduce "reftable-system.h" early
    on.
  - Make the split between "reftable-system.h" and "system.h" cleaner
    and explain it better.
  - Drop the `REFTABLE_INLINE()` patch. I can work around it in libgit2
    itself.
  - Drop the fsync callback and instead require it to exist, so that we
    can do some macro magic to use our own `fsync_component()` instead.
  - Rebased the libgit2 changes on top of this series to verify that
    we're still moving into the right direction.
  - Link to v1: https://patch.msgid.link/20260331-pks-reftable-portability-fixes-v1-0-46bfae55c68c@pks.im

Thanks!

Patrick

[1]: https://github.com/libgit2/libgit2/pull/7117
[2]: https://gitlab.com/gitlab-org/git/-/merge_requests/535

---
Patrick Steinhardt (5):
      reftable: introduce "reftable-system.h" header
      reftable/stack: provide fsync(3p) via system header
      reftable/fsck: use REFTABLE_UNUSED instead of UNUSED
      reftable/system: add abstraction to retrieve time in milliseconds
      reftable/system: add abstraction to mmap files

 refs/reftable-backend.c         |  6 ------
 reftable/blocksource.c          | 19 +++++++------------
 reftable/fsck.c                 |  2 +-
 reftable/reftable-basics.h      |  2 +-
 reftable/reftable-block.h       |  3 +--
 reftable/reftable-blocksource.h |  2 +-
 reftable/reftable-error.h       |  2 ++
 reftable/reftable-fsck.h        |  1 +
 reftable/reftable-iterator.h    |  1 +
 reftable/reftable-merged.h      |  1 +
 reftable/reftable-record.h      |  2 +-
 reftable/reftable-stack.h       |  1 +
 reftable/reftable-system.h      | 18 ++++++++++++++++++
 reftable/reftable-table.h       |  1 +
 reftable/reftable-writer.h      | 10 +---------
 reftable/stack.c                | 40 +++++++---------------------------------
 reftable/system.c               | 32 ++++++++++++++++++++++++++++++++
 reftable/system.h               | 32 ++++++++++++++++++++++++++++----
 18 files changed, 105 insertions(+), 70 deletions(-)

Range-diff versus v1:

1:  77c23e530a < -:  ---------- reftable/system: provide `REFTABLE_INLINE()` macro
2:  a32aeffa92 < -:  ---------- reftable/stack: don't call fsync(3p) unless provided
6:  5413397e77 ! 1:  11f69c228e reftable: introduce "reftable-system.h" header
    @@ Commit message
         headers relevant to us.
     
         Introduce a new "reftable-system.h" header that allows projects to
    -    provide their own headers.
    +    provide their own headers. This new header is supposed to contain all
    +    the project-specific bits to provide the POSIX-like environment, and some
    +    additional supporting code. With this change, we thus have the following
    +    split in our system-specific code:
    +
    +      - "reftable/reftable-system.h" is the project-specific header that
    +        provides a POSIX-like environment. Every project is expected to
    +        provide their own implementation.
    +
    +      - "reftable/system.h" contains the project-independent definition of
    +        the interfaces that a project needs to implement. This file should
    +        not be touched by a project.
    +
    +      - "reftable/system.c" contains the project-specific implementation of
    +        the interfaces defined in "system.h". Again, every project is
    +        expected to provide their own implementation.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ reftable/reftable-system.h (new)
     +#ifndef REFTABLE_SYSTEM_H
     +#define REFTABLE_SYSTEM_H
     +
    ++/*
    ++ * This header defines the platform-specific bits required to compile the
    ++ * reftable library. It should provide an environment that bridges over the
    ++ * gaps between POSIX and your system, as well as the zlib interfaces. This
    ++ * header is expected to be changed by the individual project.
    ++ */
    ++
     +#define MINGW_DONT_HANDLE_IN_USE_ERROR
     +#include "compat/posix.h"
    ++#include "compat/zlib-compat.h"
     +
     +#endif
     
    @@ reftable/reftable-writer.h
     
      ## reftable/system.h ##
     @@
    + #ifndef SYSTEM_H
    + #define SYSTEM_H
      
    - /* This header glues the reftable library to the rest of Git */
    +-/* This header glues the reftable library to the rest of Git */
    ++/*
    ++ * This header defines the platform-agnostic interface that is to be
    ++ * implemented by the project to make it work on their respective supported
    ++ * systems, and to integrate it into the project itself. This header is not
    ++ * expected to be changed by the individual project.
    ++ */
      
     -#define MINGW_DONT_HANDLE_IN_USE_ERROR
     -#include "compat/posix.h"
    +-#include "compat/zlib-compat.h"
     +#include "reftable-system.h"
    - #include "compat/zlib-compat.h"
      
    - #define REFTABLE_INLINE(type) static inline type
    + /*
    +  * Return a random 32 bit integer. This function is expected to return
-:  ---------- > 2:  422c12955e reftable/stack: provide fsync(3p) via system header
3:  94bd6cae41 = 3:  73fa205b5a reftable/fsck: use REFTABLE_UNUSED instead of UNUSED
4:  ea5aa25f71 ! 4:  139583e68a reftable/system: add abstraction to retrieve time in milliseconds
    @@ reftable/system.c
      #include "../lockfile.h"
     +#include "../trace.h"
      #include "../tempfile.h"
    + #include "../write-or-die.h"
      
    - uint32_t reftable_rand(void)
    -@@ reftable/system.c: int flock_commit(struct reftable_flock *l)
    - 
    - 	return 0;
    +@@ reftable/system.c: int reftable_fsync(int fd)
    + {
    + 	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
      }
     +
     +uint64_t reftable_time_ms(void)
5:  27d055eecd = 5:  a445a6e6eb reftable/system: add abstraction to mmap files

---
base-commit: 270e10ad6dda3379ea0da7efd11e4fbf2cd7a325
change-id: 20260330-pks-reftable-portability-fixes-36ebf9f227c2

[PATCH v2 1/5] reftable: introduce "reftable-system.h" header

[Patrick Steinhardt]

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.

Introduce a new "reftable-system.h" header that allows projects to
provide their own headers. This new header is supposed to contain all
the project-specific bits to provide the POSIX-like environment, and some
additional supporting code. With this change, we thus have the following
split in our system-specific code:

  - "reftable/reftable-system.h" is the project-specific header that
    provides a POSIX-like environment. Every project is expected to
    provide their own implementation.

  - "reftable/system.h" contains the project-independent definition of
    the interfaces that a project needs to implement. This file should
    not be touched by a project.

  - "reftable/system.c" contains the project-specific implementation of
    the interfaces defined in "system.h". Again, every project is
    expected to provide their own implementation.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-basics.h      |  2 +-
 reftable/reftable-block.h       |  3 +--
 reftable/reftable-blocksource.h |  2 +-
 reftable/reftable-error.h       |  2 ++
 reftable/reftable-fsck.h        |  1 +
 reftable/reftable-iterator.h    |  1 +
 reftable/reftable-merged.h      |  1 +
 reftable/reftable-record.h      |  2 +-
 reftable/reftable-stack.h       |  1 +
 reftable/reftable-system.h      | 15 +++++++++++++++
 reftable/reftable-table.h       |  1 +
 reftable/reftable-writer.h      |  4 +---
 reftable/system.h               | 11 +++++++----
 13 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/reftable/reftable-basics.h b/reftable/reftable-basics.h
index 6d73f19c85..dc8622682d 100644
--- a/reftable/reftable-basics.h
+++ b/reftable/reftable-basics.h
@@ -9,7 +9,7 @@
 #ifndef REFTABLE_BASICS_H
 #define REFTABLE_BASICS_H
 
-#include <stddef.h>
+#include "reftable-system.h"
 
 /* A buffer that contains arbitrary byte slices. */
 struct reftable_buf {
diff --git a/reftable/reftable-block.h b/reftable/reftable-block.h
index 0b05a8f7e3..94c79b5c58 100644
--- a/reftable/reftable-block.h
+++ b/reftable/reftable-block.h
@@ -9,8 +9,7 @@
 #ifndef REFTABLE_BLOCK_H
 #define REFTABLE_BLOCK_H
 
-#include <stdint.h>
-
+#include "reftable-system.h"
 #include "reftable-basics.h"
 #include "reftable-blocksource.h"
 #include "reftable-iterator.h"
diff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h
index f5ba867bd6..40c1e94646 100644
--- a/reftable/reftable-blocksource.h
+++ b/reftable/reftable-blocksource.h
@@ -9,7 +9,7 @@
 #ifndef REFTABLE_BLOCKSOURCE_H
 #define REFTABLE_BLOCKSOURCE_H
 
-#include <stdint.h>
+#include "reftable-system.h"
 
 /*
  * Generic wrapper for a seekable readable file.
diff --git a/reftable/reftable-error.h b/reftable/reftable-error.h
index d100e0df92..0535e1478b 100644
--- a/reftable/reftable-error.h
+++ b/reftable/reftable-error.h
@@ -9,6 +9,8 @@
 #ifndef REFTABLE_ERROR_H
 #define REFTABLE_ERROR_H
 
+#include "reftable-system.h"
+
 /*
  * Errors in reftable calls are signaled with negative integer return values. 0
  * means success.
diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
index 007a392cf9..340fc7762e 100644
--- a/reftable/reftable-fsck.h
+++ b/reftable/reftable-fsck.h
@@ -1,6 +1,7 @@
 #ifndef REFTABLE_FSCK_H
 #define REFTABLE_FSCK_H
 
+#include "reftable-system.h"
 #include "reftable-stack.h"
 
 enum reftable_fsck_error {
diff --git a/reftable/reftable-iterator.h b/reftable/reftable-iterator.h
index af582028c2..a050cc153b 100644
--- a/reftable/reftable-iterator.h
+++ b/reftable/reftable-iterator.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_ITERATOR_H
 #define REFTABLE_ITERATOR_H
 
+#include "reftable-system.h"
 #include "reftable-record.h"
 
 struct reftable_iterator_vtable;
diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h
index e5af846b32..02a9966835 100644
--- a/reftable/reftable-merged.h
+++ b/reftable/reftable-merged.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_MERGED_H
 #define REFTABLE_MERGED_H
 
+#include "reftable-system.h"
 #include "reftable-iterator.h"
 
 /*
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index 385a74cc86..e18c538238 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -9,8 +9,8 @@
 #ifndef REFTABLE_RECORD_H
 #define REFTABLE_RECORD_H
 
+#include "reftable-system.h"
 #include "reftable-basics.h"
-#include <stdint.h>
 
 /*
  * Basic data types
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index c2415cbc6e..5f7be573fa 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_STACK_H
 #define REFTABLE_STACK_H
 
+#include "reftable-system.h"
 #include "reftable-writer.h"
 
 /*
diff --git a/reftable/reftable-system.h b/reftable/reftable-system.h
new file mode 100644
index 0000000000..4a18a6a790
--- /dev/null
+++ b/reftable/reftable-system.h
@@ -0,0 +1,15 @@
+#ifndef REFTABLE_SYSTEM_H
+#define REFTABLE_SYSTEM_H
+
+/*
+ * This header defines the platform-specific bits required to compile the
+ * reftable library. It should provide an environment that bridges over the
+ * gaps between POSIX and your system, as well as the zlib interfaces. This
+ * header is expected to be changed by the individual project.
+ */
+
+#define MINGW_DONT_HANDLE_IN_USE_ERROR
+#include "compat/posix.h"
+#include "compat/zlib-compat.h"
+
+#endif
diff --git a/reftable/reftable-table.h b/reftable/reftable-table.h
index 5f935d02e3..d7666b53a1 100644
--- a/reftable/reftable-table.h
+++ b/reftable/reftable-table.h
@@ -9,6 +9,7 @@
 #ifndef REFTABLE_TABLE_H
 #define REFTABLE_TABLE_H
 
+#include "reftable-system.h"
 #include "reftable-iterator.h"
 #include "reftable-block.h"
 #include "reftable-blocksource.h"
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 1e7003cd69..065dd93dc6 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -9,11 +9,9 @@
 #ifndef REFTABLE_WRITER_H
 #define REFTABLE_WRITER_H
 
+#include "reftable-system.h"
 #include "reftable-record.h"
 
-#include <stdint.h>
-#include <unistd.h> /* ssize_t */
-
 /* Writing single reftables */
 
 /* reftable_write_options sets options for writing a single reftable. */
diff --git a/reftable/system.h b/reftable/system.h
index c54ed4cad6..a7eb6acd4a 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -9,11 +9,14 @@
 #ifndef SYSTEM_H
 #define SYSTEM_H
 
-/* This header glues the reftable library to the rest of Git */
+/*
+ * This header defines the platform-agnostic interface that is to be
+ * implemented by the project to make it work on their respective supported
+ * systems, and to integrate it into the project itself. This header is not
+ * expected to be changed by the individual project.
+ */
 
-#define MINGW_DONT_HANDLE_IN_USE_ERROR
-#include "compat/posix.h"
-#include "compat/zlib-compat.h"
+#include "reftable-system.h"
 
 /*
  * Return a random 32 bit integer. This function is expected to return

-- 
2.53.0.1323.g189a785ab5.dirty

[PATCH v2 2/5] reftable/stack: provide fsync(3p) via system header

[Patrick Steinhardt]

Users of the reftable library are expected to provide their own function
callback in cases they want to sync(3p) data to disk via the reftable
write options. But if no such function was provided we end up calling
fsync(3p) directly, which may not even be available on some systems.

While dropping the explicit call to fsync(3p) would work, it would lead
to an unsafe default behaviour where a project may have forgotten to set
up the callback function, and that could lead to potential data loss. So
this is not a great solution.

Instead, drop the callback function and make it mandatory for the
project to define fsync(3p). In the case of Git, we can then easily
inject our custom implementation via the "reftable-system.h" header so
that we continue to use `fsync_component()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c    |  6 ------
 reftable/reftable-system.h |  3 +++
 reftable/reftable-writer.h |  6 ------
 reftable/stack.c           | 13 +++----------
 reftable/system.c          |  6 ++++++
 5 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index b124404663..daea30a5b4 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -366,11 +366,6 @@ static int reftable_be_config(const char *var, const char *value,
 	return 0;
 }
 
-static int reftable_be_fsync(int fd)
-{
-	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
-}
-
 static struct ref_store *reftable_be_init(struct repository *repo,
 					  const char *payload,
 					  const char *gitdir,
@@ -408,7 +403,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	refs->write_options.disable_auto_compact =
 		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
 	refs->write_options.lock_timeout_ms = 100;
-	refs->write_options.fsync = reftable_be_fsync;
 
 	repo_config(the_repository, reftable_be_config, &refs->write_options);
 
diff --git a/reftable/reftable-system.h b/reftable/reftable-system.h
index 4a18a6a790..76f3e33e90 100644
--- a/reftable/reftable-system.h
+++ b/reftable/reftable-system.h
@@ -12,4 +12,7 @@
 #include "compat/posix.h"
 #include "compat/zlib-compat.h"
 
+int reftable_fsync(int fd);
+#define fsync(fd) reftable_fsync(fd)
+
 #endif
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 065dd93dc6..a66db415c8 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -61,12 +61,6 @@ struct reftable_write_options {
 	 */
 	long lock_timeout_ms;
 
-	/*
-	 * Optional callback used to fsync files to disk. Falls back to using
-	 * fsync(3P) when unset.
-	 */
-	int (*fsync)(int fd);
-
 	/*
 	 * Callback function to execute whenever the stack is being reloaded.
 	 * This can be used e.g. to discard cached information that relies on
diff --git a/reftable/stack.c b/reftable/stack.c
index 1c9f21dfe1..fa87b46c37 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -29,13 +29,6 @@ static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 	return 0;
 }
 
-static int stack_fsync(const struct reftable_write_options *opts, int fd)
-{
-	if (opts->fsync)
-		return opts->fsync(fd);
-	return fsync(fd);
-}
-
 static ssize_t reftable_write_data(int fd, const void *data, size_t size)
 {
 	size_t total_written = 0;
@@ -69,7 +62,7 @@ static ssize_t fd_writer_write(void *arg, const void *data, size_t sz)
 static int fd_writer_flush(void *arg)
 {
 	struct fd_writer *writer = arg;
-	return stack_fsync(writer->opts, writer->fd);
+	return fsync(writer->fd);
 }
 
 static int fd_read_lines(int fd, char ***namesp)
@@ -812,7 +805,7 @@ int reftable_addition_commit(struct reftable_addition *add)
 		goto done;
 	}
 
-	err = stack_fsync(&add->stack->opts, add->tables_list_lock.fd);
+	err = fsync(add->tables_list_lock.fd);
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		goto done;
@@ -1480,7 +1473,7 @@ static int stack_compact_range(struct reftable_stack *st,
 		goto done;
 	}
 
-	err = stack_fsync(&st->opts, tables_list_lock.fd);
+	err = fsync(tables_list_lock.fd);
 	if (err < 0) {
 		err = REFTABLE_IO_ERROR;
 		unlink(new_table_path.buf);
diff --git a/reftable/system.c b/reftable/system.c
index 725a25844e..4d7e366b55 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -5,6 +5,7 @@
 #include "reftable-error.h"
 #include "../lockfile.h"
 #include "../tempfile.h"
+#include "../write-or-die.h"
 
 uint32_t reftable_rand(void)
 {
@@ -131,3 +132,8 @@ int flock_commit(struct reftable_flock *l)
 
 	return 0;
 }
+
+int reftable_fsync(int fd)
+{
+	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
+}

-- 
2.53.0.1323.g189a785ab5.dirty

[PATCH v2 3/5] reftable/fsck: use REFTABLE_UNUSED instead of UNUSED

[Patrick Steinhardt]

While we have the reftable-specific `REFTABLE_UNUSED` header, we
accidentally introduced a new usage of the Git-specific `UNUSED` header
into the reftable library in 9051638519 (reftable: add code to
facilitate consistency checks, 2025-10-07).

Convert the site to use `REFTABLE_UNUSED`.

Ideally, we'd move the definition of `UNUSED` into "git-compat-util.h"
so that it becomes in accessible to the reftable library. But this is
unfortunately not easily possible as "compat/mingw-posix.h" requires
this macro, and this header is included by "compat/posix.h".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/fsck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reftable/fsck.c b/reftable/fsck.c
index 26b9115b14..8e73fc83f2 100644
--- a/reftable/fsck.c
+++ b/reftable/fsck.c
@@ -63,7 +63,7 @@ static int table_check_name(struct reftable_table *table,
 
 static int table_checks(struct reftable_table *table,
 			reftable_fsck_report_fn report_fn,
-			reftable_fsck_verbose_fn verbose_fn UNUSED,
+			reftable_fsck_verbose_fn verbose_fn REFTABLE_UNUSED,
 			void *cb_data)
 {
 	table_check_fn table_check_fns[] = {

-- 
2.53.0.1323.g189a785ab5.dirty

[PATCH v2 4/5] reftable/system: add abstraction to retrieve time in milliseconds

[Patrick Steinhardt]

We directly call gettimeofday(3p), which may not be available on some
platforms. Provide the infrastructure to let projects easily use their
own implementations of this function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c  | 27 ++++-----------------------
 reftable/system.c |  6 ++++++
 reftable/system.h |  3 +++
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index fa87b46c37..1fba96ddb3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -365,45 +365,26 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 	return err;
 }
 
-/* return negative if a before b. */
-static int tv_cmp(struct timeval *a, struct timeval *b)
-{
-	time_t diff = a->tv_sec - b->tv_sec;
-	int udiff = a->tv_usec - b->tv_usec;
-
-	if (diff != 0)
-		return diff;
-
-	return udiff;
-}
-
 static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 					     int reuse_open)
 {
 	char **names = NULL, **names_after = NULL;
-	struct timeval deadline;
+	uint64_t deadline;
 	int64_t delay = 0;
 	int tries = 0, err;
 	int fd = -1;
 
-	err = gettimeofday(&deadline, NULL);
-	if (err < 0)
-		goto out;
-	deadline.tv_sec += 3;
+	deadline = reftable_time_ms() + 3000;
 
 	while (1) {
-		struct timeval now;
-
-		err = gettimeofday(&now, NULL);
-		if (err < 0)
-			goto out;
+		uint64_t now = reftable_time_ms();
 
 		/*
 		 * Only look at deadlines after the first few times. This
 		 * simplifies debugging in GDB.
 		 */
 		tries++;
-		if (tries > 3 && tv_cmp(&now, &deadline) >= 0)
+		if (tries > 3 && now >= deadline)
 			goto out;
 
 		fd = open(st->list_file, O_RDONLY);
diff --git a/reftable/system.c b/reftable/system.c
index 4d7e366b55..cd76e56be8 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -4,6 +4,7 @@
 #include "basics.h"
 #include "reftable-error.h"
 #include "../lockfile.h"
+#include "../trace.h"
 #include "../tempfile.h"
 #include "../write-or-die.h"
 
@@ -137,3 +138,8 @@ int reftable_fsync(int fd)
 {
 	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
 }
+
+uint64_t reftable_time_ms(void)
+{
+	return getnanotime() / 1000000;
+}
diff --git a/reftable/system.h b/reftable/system.h
index a7eb6acd4a..071bfa3d58 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -111,4 +111,7 @@ int flock_release(struct reftable_flock *l);
  */
 int flock_commit(struct reftable_flock *l);
 
+/* Report the time in milliseconds. */
+uint64_t reftable_time_ms(void);
+
 #endif

-- 
2.53.0.1323.g189a785ab5.dirty

[PATCH v2 5/5] reftable/system: add abstraction to mmap files

[Patrick Steinhardt]

In our codebase we have a couple of wrappers around mmap(3p) that allow
us to reimplement the syscall on platforms that don't have it natively,
like for example Windows. Other projects that embed the reftable library
may have a different infra though to hook up mmap wrappers, but these
are currently hard to integrate.

Provide the infrastructure to let projects easily define the mmap
interface with a custom struct and custom functions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/blocksource.c | 19 +++++++------------
 reftable/system.c      | 20 ++++++++++++++++++++
 reftable/system.h      | 18 ++++++++++++++++++
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index 573c81287f..7f7441f751 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -93,13 +93,12 @@ void block_source_from_buf(struct reftable_block_source *bs,
 }
 
 struct file_block_source {
-	uint64_t size;
-	unsigned char *data;
+	struct reftable_mmap mmap;
 };
 
 static uint64_t file_size(void *b)
 {
-	return ((struct file_block_source *)b)->size;
+	return ((struct file_block_source *)b)->mmap.size;
 }
 
 static void file_release_data(void *b REFTABLE_UNUSED, struct reftable_block_data *dest REFTABLE_UNUSED)
@@ -109,7 +108,7 @@ static void file_release_data(void *b REFTABLE_UNUSED, struct reftable_block_dat
 static void file_close(void *v)
 {
 	struct file_block_source *b = v;
-	munmap(b->data, b->size);
+	reftable_munmap(&b->mmap);
 	reftable_free(b);
 }
 
@@ -117,8 +116,8 @@ static ssize_t file_read_data(void *v, struct reftable_block_data *dest, uint64_
 			      uint32_t size)
 {
 	struct file_block_source *b = v;
-	assert(off + size <= b->size);
-	dest->data = b->data + off;
+	assert(off + size <= b->mmap.size);
+	dest->data = (unsigned char *) b->mmap.data + off;
 	dest->len = size;
 	return size;
 }
@@ -156,13 +155,9 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
 		goto out;
 	}
 
-	p->size = st.st_size;
-	p->data = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (p->data == MAP_FAILED) {
-		err = REFTABLE_IO_ERROR;
-		p->data = NULL;
+	err = reftable_mmap(&p->mmap, fd, st.st_size);
+	if (err < 0)
 		goto out;
-	}
 
 	assert(!bs->ops);
 	bs->ops = &file_vtable;
diff --git a/reftable/system.c b/reftable/system.c
index cd76e56be8..9063641f30 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -143,3 +143,23 @@ uint64_t reftable_time_ms(void)
 {
 	return getnanotime() / 1000000;
 }
+
+int reftable_mmap(struct reftable_mmap *out, int fd, size_t len)
+{
+	void *data = xmmap_gently(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (data == MAP_FAILED)
+		return REFTABLE_IO_ERROR;
+
+	out->data = data;
+	out->size = len;
+
+	return 0;
+}
+
+int reftable_munmap(struct reftable_mmap *mmap)
+{
+	if (munmap(mmap->data, mmap->size) < 0)
+		return REFTABLE_IO_ERROR;
+	memset(mmap, 0, sizeof(*mmap));
+	return 0;
+}
diff --git a/reftable/system.h b/reftable/system.h
index 071bfa3d58..c0e2cbe0ff 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -114,4 +114,22 @@ int flock_commit(struct reftable_flock *l);
 /* Report the time in milliseconds. */
 uint64_t reftable_time_ms(void);
 
+struct reftable_mmap {
+       void *data;
+       size_t size;
+       void *priv;
+};
+
+/*
+ * Map the file into memory. Returns 0 on success, a reftable error code on
+ * error.
+ */
+int reftable_mmap(struct reftable_mmap *out, int fd, size_t len);
+
+/*
+ * Unmap the file from memory. Returns 0 on success, a reftable error code on
+ * error.
+ */
+int reftable_munmap(struct reftable_mmap *mmap);
+
 #endif

-- 
2.53.0.1323.g189a785ab5.dirty

Re: [PATCH v2 2/5] reftable/stack: provide fsync(3p) via system header

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Users of the reftable library are expected to provide their own function
> callback in cases they want to sync(3p) data to disk via the reftable
> write options. But if no such function was provided we end up calling
> fsync(3p) directly, which may not even be available on some systems.
>
> While dropping the explicit call to fsync(3p) would work, it would lead
> to an unsafe default behaviour where a project may have forgotten to set
> up the callback function, and that could lead to potential data loss. So
> this is not a great solution.
>
> Instead, drop the callback function and make it mandatory for the
> project to define fsync(3p). In the case of Git, we can then easily
> inject our custom implementation via the "reftable-system.h" header so
> that we continue to use `fsync_component()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c    |  6 ------
>  reftable/reftable-system.h |  3 +++
>  reftable/reftable-writer.h |  6 ------
>  reftable/stack.c           | 13 +++----------
>  reftable/system.c          |  6 ++++++
>  5 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index b124404663..daea30a5b4 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -366,11 +366,6 @@ static int reftable_be_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> -static int reftable_be_fsync(int fd)
> -{
> -	return fsync_component(FSYNC_COMPONENT_REFERENCE, fd);
> -}
> -
>  static struct ref_store *reftable_be_init(struct repository *repo,
>  					  const char *payload,
>  					  const char *gitdir,
> @@ -408,7 +403,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
>  	refs->write_options.disable_auto_compact =
>  		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
>  	refs->write_options.lock_timeout_ms = 100;
> -	refs->write_options.fsync = reftable_be_fsync;


It used to be that by swapping the write_options settings the
project can choose to perform its fsync in different ways depending
on what they are writing, but now we have a chance to specify a
single fsync() in <reftable/system.c>?  

The project code does not set up write_options and the project code
has no say in the choice of the kind of fsync used for different
data files the reftable library uses, so it is not a problem.

OK.

Re: [PATCH v2 4/5] reftable/system: add abstraction to retrieve time in milliseconds

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We directly call gettimeofday(3p), which may not be available on some
> platforms. Provide the infrastructure to let projects easily use their
> own implementations of this function.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c  | 27 ++++-----------------------
>  reftable/system.c |  6 ++++++
>  reftable/system.h |  3 +++
>  3 files changed, 13 insertions(+), 23 deletions(-)
> ...
> +uint64_t reftable_time_ms(void)
> +{
> +	return getnanotime() / 1000000;
> +}
> diff --git a/reftable/system.h b/reftable/system.h
> index a7eb6acd4a..071bfa3d58 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -111,4 +111,7 @@ int flock_release(struct reftable_flock *l);
>   */
>  int flock_commit(struct reftable_flock *l);
>  
> +/* Report the time in milliseconds. */
> +uint64_t reftable_time_ms(void);

This must be give the current time in milliseconds, measured from
some fixed point in time.  It is up to the implementation to choose
what absolute time as the epoch, since we only use this to compare
one timestamp returned by a call to this function with another.

The "we do not care what epoch you choose, but you have to be
consistent" requirement may want to be written down here, though.