[PATCH 0/5] reftable: a couple of improvements for libgit2

[PATCH 0/5] reftable: a couple of improvements for libgit2

[Patrick Steinhardt]

Hi,

this small patch series contains a couple of improvements I required for
libgit2. With those changes libgit2 is now able to run its full test
suite with reftable-enabled repositories. I still need to invest a bit
of work to make it memory-leak free and compile on Windows, but overall
I think that support for reftables is almost ready.

Thanks!

Patrick

---
Patrick Steinhardt (5):
      reftable/writer: fix type used for number of records
      reftable/writer: drop Git-specific `QSORT()` macro
      reftable/stack: fix compiler warning due to missing braces
      reftable/stack: reorder code to avoid forward declarations
      reftable/stack: allow passing flags to `reftable_stack_add()`

 refs/reftable-backend.c         |   8 +-
 reftable/reftable-stack.h       |   9 +-
 reftable/reftable-writer.h      |   4 +-
 reftable/stack.c                | 366 +++++++++++++++++++---------------------
 reftable/writer.c               |  23 +--
 t/unit-tests/t-reftable-stack.c |  50 +++---
 6 files changed, 227 insertions(+), 233 deletions(-)


---
base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c
change-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603

[PATCH 1/5] reftable/writer: fix type used for number of records

[Patrick Steinhardt]

Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`
accept an array of records that should be added to the new table.
Callers of this function are expected to also pass the number of such
records to the function to tell it how many such records it is supposed
to write.

But while all callers pass in a `size_t`, which is a sensible choice,
the function in fact accepts an `int` as argument, which is less so. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-writer.h |  4 ++--
 reftable/writer.c          | 17 +++++++++--------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 0fbeff17f4..1e7003cd69 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
   the records before adding them, reordering the records array passed in.
 */
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n);
+			     struct reftable_ref_record *refs, size_t n);
 
 /*
   adds reftable_log_records. Log records are keyed by (refname, decreasing
@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,
   the records before adding them, reordering records array passed in.
 */
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n);
+			     struct reftable_log_record *logs, size_t n);
 
 /* reftable_writer_close finalizes the reftable. The writer is retained so
  * statistics can be inspected. */
diff --git a/reftable/writer.c b/reftable/writer.c
index 3b4ebdd6dc..5bad130c7e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 }
 
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n)
+			     struct reftable_ref_record *refs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(refs, n, reftable_ref_record_compare_name);
-	for (i = 0; err == 0 && i < n; i++) {
+
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
-	}
+
 	return err;
 }
 
@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,
 }
 
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n)
+			     struct reftable_log_record *logs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(logs, n, reftable_log_record_compare_key);
 
-	for (i = 0; err == 0 && i < n; i++) {
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);
-	}
+
 	return err;
 }
 

-- 
2.50.1

[PATCH 2/5] reftable/writer: drop Git-specific `QSORT()` macro

[Patrick Steinhardt]

The reftable writer accidentally uses the Git-specific `QSORT()` macro.
This macro removes the need for the caller to provide the element size,
but other than that it's mostly equivalent to `qsort()`.

Replace the macro accordingly to make the library usable outside of Git.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/writer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index 5bad130c7e..0133b64975 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(refs, n, reftable_ref_record_compare_name);
+	if (n)
+		qsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(logs, n, reftable_log_record_compare_key);
+	if (n)
+		qsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);

-- 
2.50.1

[PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

While perfectly legal, older compiler toolchains complain when
zero-initializing structs that contain nested structs with `{0}`:

    /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
            struct reftable_addition empty = REFTABLE_ADDITION_INIT;
                                             ^~~~~~~~~~~~~~~~~~~~~~
    /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
    #define REFTABLE_ADDITION_INIT {0}
                                    ^

Silence this warning by using `{{0}}` 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 4caf96aa1d..3480ad21c3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -704,7 +704,7 @@ struct reftable_addition {
 	uint64_t next_update_index;
 };
 
-#define REFTABLE_ADDITION_INIT {0}
+#define REFTABLE_ADDITION_INIT {{0}}
 
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,

-- 
2.50.1

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Eric Sunshine]

On Fri, Aug 1, 2025 at 10:48 AM Patrick Steinhardt <ps@pks.im> wrote:
> While perfectly legal, older compiler toolchains complain when
> zero-initializing structs that contain nested structs with `{0}`:
> [...]
> Silence this warning by using `{{0}}` instead.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/reftable/stack.c b/reftable/stack.c
> -#define REFTABLE_ADDITION_INIT {0}
> +#define REFTABLE_ADDITION_INIT {{0}}

For what it's worth, the last time this sort of issue came up[*], I
believe the ultimate response was that the project ought not pander to
this particular instance of buggy compiler and that, instead, an
individual developer should, if bothered by the warning, use the
appropriate compiler option to suppress the warning. Whether or not
that attitude should apply also to the reftable code which is meant to
be shareable, is a separate question, but at least that previous
discussion thread provides some background regarding how different
developers feel or felt about the issue.

[*]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

On Fri, Aug 01, 2025 at 03:19:52PM -0400, Eric Sunshine wrote:
> On Fri, Aug 1, 2025 at 10:48 AM Patrick Steinhardt <ps@pks.im> wrote:
> > While perfectly legal, older compiler toolchains complain when
> > zero-initializing structs that contain nested structs with `{0}`:
> > [...]
> > Silence this warning by using `{{0}}` instead.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > -#define REFTABLE_ADDITION_INIT {0}
> > +#define REFTABLE_ADDITION_INIT {{0}}
> 
> For what it's worth, the last time this sort of issue came up[*], I
> believe the ultimate response was that the project ought not pander to
> this particular instance of buggy compiler and that, instead, an
> individual developer should, if bothered by the warning, use the
> appropriate compiler option to suppress the warning. Whether or not
> that attitude should apply also to the reftable code which is meant to
> be shareable, is a separate question, but at least that previous
> discussion thread provides some background regarding how different
> developers feel or felt about the issue.
> 
> [*]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/

Yeah, in general I'm also of the opinion that we shouldn't bother. But
in libgit2 we have pipelines that use such older compilers, and we don't
want to drop those for now. So I think we should treat the reftable
library specially, doubly so as this is the only instance that causes
problems.

Thanks for digging up that old thread!

Patrick

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Yeah, in general I'm also of the opinion that we shouldn't bother. But
> in libgit2 we have pipelines that use such older compilers, and we don't
> want to drop those for now. So I think we should treat the reftable
> library specially, doubly so as this is the only instance that causes
> problems.

Hmph.  Shouldn't there be some kind of "shim" layer where these
things are defined per project convention and/or toolchain being
used?  So when building for git proper, you'd use {0} just as
everybody else do, but for others your include file supplied by that
project would use something else (like {{0}} in this case)?  That
kind of approach would be a better solution than open coding QSORT()
in the longer term, for example.

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Yeah, in general I'm also of the opinion that we shouldn't bother. But
> > in libgit2 we have pipelines that use such older compilers, and we don't
> > want to drop those for now. So I think we should treat the reftable
> > library specially, doubly so as this is the only instance that causes
> > problems.
> 
> Hmph.  Shouldn't there be some kind of "shim" layer where these
> things are defined per project convention and/or toolchain being
> used?  So when building for git proper, you'd use {0} just as
> everybody else do, but for others your include file supplied by that
> project would use something else (like {{0}} in this case)?  That
> kind of approach would be a better solution than open coding QSORT()
> in the longer term, for example.

We do have a shim layer, but I don't think it makes sense to use it for
every small piece. The intent of that layer is to paper over platform
differences that we cannot easily hide away in a generic fashion. So
things like mmap, random numbers, handling includes or registering
lockfiles via atexit(3p).

But I don't think it makes sense to use the shim layer for things like
`{0}` vs `{{0}}` or `QSORT()`. It doesn't really have any downside to
change to `{{0}}`, and the one invocation of `QSORT()` is trivial to
convert to qsort(3p) and unlikely to cause any more maintenance in the
long run than the invocation of `QSORT()` would.

On the other hand, it does have a downside if we moved all of this into
the shim layer, because now every user of the reftable library will have
to reinvent the wheel. The other downside is that the more we move into
the shim layer, the more the different implementations of the reftable
backend may diverge in behaviour. Reversely, the more code is the exact
same across implementations the more we can benefit from the tests that
those other implementations use and the more solid the reftable library
becomes.

Patrick

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Carlo Arenas]

On Tue, Aug 05, 2025 at 06:49:45AM -0800, Patrick Steinhardt wrote:
> On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > Yeah, in general I'm also of the opinion that we shouldn't bother. But
> > > in libgit2 we have pipelines that use such older compilers, and we don't
> > > want to drop those for now. So I think we should treat the reftable
> > > library specially, doubly so as this is the only instance that causes
> > > problems.
> > 
> > Hmph.  Shouldn't there be some kind of "shim" layer where these
> > things are defined per project convention and/or toolchain being
> > used?  So when building for git proper, you'd use {0} just as
> > everybody else do, but for others your include file supplied by that
> > project would use something else (like {{0}} in this case)?  That
> > kind of approach would be a better solution than open coding QSORT()
> > in the longer term, for example.
> 
> We do have a shim layer, but I don't think it makes sense to use it for
> every small piece. The intent of that layer is to paper over platform
> differences that we cannot easily hide away in a generic fashion. So
> things like mmap, random numbers, handling includes or registering
> lockfiles via atexit(3p).
> 
> But I don't think it makes sense to use the shim layer for things like
> `{0}` vs `{{0}}`

I think the suggestion for using a shim layer solution is relevant, because
additionally to the compatibility issues of the zero initializer, you also
need to take into consideration that the proposed solution will still trigger
warnings when compiled as C++ (where {0} should be instead {}).

Why not do instead something like?

diff --git a/reftable/stack.c b/reftable/stack.c
index 4caf96aa1d..80ce8a7083 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -704,8 +704,6 @@ struct reftable_addition {
 	uint64_t next_update_index;
 };
 
-#define REFTABLE_ADDITION_INIT {0}
-
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
 					unsigned int flags)
@@ -859,7 +857,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 				unsigned int flags)
 {
 	int err = 0;
-	struct reftable_addition empty = REFTABLE_ADDITION_INIT;
+	static const struct reftable_addition empty;
 
 	REFTABLE_CALLOC_ARRAY(*dest, 1);
 	if (!*dest)
@@ -879,8 +877,11 @@ static int stack_try_add(struct reftable_stack *st,
 					    void *arg),
 			 void *arg)
 {
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
+	struct reftable_addition add;
+	int err;
+
+	memset(&add, 0, sizeof(add));
+	err = reftable_stack_init_addition(&add, st, 0);
 	if (err < 0)
 		goto done;
 
Carlo

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

On Mon, Aug 11, 2025 at 09:18:09PM -0700, Carlo Arenas wrote:
> On Tue, Aug 05, 2025 at 06:49:45AM -0800, Patrick Steinhardt wrote:
> > On Mon, Aug 04, 2025 at 12:14:22PM -0700, Junio C Hamano wrote:
> > > Patrick Steinhardt <ps@pks.im> writes:
> > > 
> > > > Yeah, in general I'm also of the opinion that we shouldn't bother. But
> > > > in libgit2 we have pipelines that use such older compilers, and we don't
> > > > want to drop those for now. So I think we should treat the reftable
> > > > library specially, doubly so as this is the only instance that causes
> > > > problems.
> > > 
> > > Hmph.  Shouldn't there be some kind of "shim" layer where these
> > > things are defined per project convention and/or toolchain being
> > > used?  So when building for git proper, you'd use {0} just as
> > > everybody else do, but for others your include file supplied by that
> > > project would use something else (like {{0}} in this case)?  That
> > > kind of approach would be a better solution than open coding QSORT()
> > > in the longer term, for example.
> > 
> > We do have a shim layer, but I don't think it makes sense to use it for
> > every small piece. The intent of that layer is to paper over platform
> > differences that we cannot easily hide away in a generic fashion. So
> > things like mmap, random numbers, handling includes or registering
> > lockfiles via atexit(3p).
> > 
> > But I don't think it makes sense to use the shim layer for things like
> > `{0}` vs `{{0}}`
> 
> I think the suggestion for using a shim layer solution is relevant, because
> additionally to the compatibility issues of the zero initializer, you also
> need to take into consideration that the proposed solution will still trigger
> warnings when compiled as C++ (where {0} should be instead {}).

Do we even support compiling Git as C++?

> Why not do instead something like?

But this is fair indeed -- the definition is internal anyway and not
exposed to outside callers, so we can just as well use `memset()`.

Patrick

> diff --git a/reftable/stack.c b/reftable/stack.c
> index 4caf96aa1d..80ce8a7083 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -704,8 +704,6 @@ struct reftable_addition {
>  	uint64_t next_update_index;
>  };
>  
> -#define REFTABLE_ADDITION_INIT {0}
> -
>  static int reftable_stack_init_addition(struct reftable_addition *add,
>  					struct reftable_stack *st,
>  					unsigned int flags)
> @@ -859,7 +857,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
>  				unsigned int flags)
>  {
>  	int err = 0;
> -	struct reftable_addition empty = REFTABLE_ADDITION_INIT;
> +	static const struct reftable_addition empty;
>  
>  	REFTABLE_CALLOC_ARRAY(*dest, 1);
>  	if (!*dest)
> @@ -879,8 +877,11 @@ static int stack_try_add(struct reftable_stack *st,
>  					    void *arg),
>  			 void *arg)
>  {
> -	struct reftable_addition add = REFTABLE_ADDITION_INIT;
> -	int err = reftable_stack_init_addition(&add, st, 0);
> +	struct reftable_addition add;
> +	int err;
> +
> +	memset(&add, 0, sizeof(add));
> +	err = reftable_stack_init_addition(&add, st, 0);
>  	if (err < 0)
>  		goto done;
>  
> Carlo

Re: [PATCH 3/5] reftable/stack: fix compiler warning due to missing braces

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> I think the suggestion for using a shim layer solution is relevant, because
>> additionally to the compatibility issues of the zero initializer, you also
>> need to take into consideration that the proposed solution will still trigger
>> warnings when compiled as C++ (where {0} should be instead {}).
>
> Do we even support compiling Git as C++?

There was an earlier effort to rename variables like "new", but I
think the motivation was more like some folks wanted to try building
with C++ compilers.  I do not know what the outcome of it was.

But people like to disect and include pieces of useful and/or
popular software in their programs within the limitation of how the
original is licensed.  Especially a part well libified like reftable
may be a good candidate.

[PATCH 4/5] reftable/stack: reorder code to avoid forward declarations

[Patrick Steinhardt]

We have a couple of forward declarations in the stack-related code of
the reftable library. These declarations aren't really required, but are
simply caused by unfortunate ordering.

Reorder the code and remove the forward declarations.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------
 1 file changed, 176 insertions(+), 188 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 3480ad21c3..d6e4ea93a3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,18 +17,6 @@
 #include "table.h"
 #include "writer.h"
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg);
-static int stack_write_compact(struct reftable_stack *st,
-			       struct reftable_writer *wr,
-			       size_t first, size_t last,
-			       struct reftable_log_expiry_config *config);
-static void reftable_addition_close(struct reftable_addition *add);
-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
-					     int reuse_open);
-
 static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 			  const char *name)
 {
@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)
 	return stack_fsync(writer->opts, writer->fd);
 }
 
-int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       const struct reftable_write_options *_opts)
-{
-	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
-	struct reftable_write_options opts = { 0 };
-	struct reftable_stack *p;
-	int err;
-
-	p = reftable_calloc(1, sizeof(*p));
-	if (!p) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	if (_opts)
-		opts = *_opts;
-	if (opts.hash_id == 0)
-		opts.hash_id = REFTABLE_HASH_SHA1;
-
-	*dest = NULL;
-
-	reftable_buf_reset(&list_file_name);
-	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
-	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
-		goto out;
-
-	p->list_file = reftable_buf_detach(&list_file_name);
-	p->list_fd = -1;
-	p->opts = opts;
-	p->reftable_dir = reftable_strdup(dir);
-	if (!p->reftable_dir) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	err = reftable_stack_reload_maybe_reuse(p, 1);
-	if (err < 0)
-		goto out;
-
-	*dest = p;
-	err = 0;
-
-out:
-	if (err < 0)
-		reftable_stack_destroy(p);
-	return err;
-}
-
 static int fd_read_lines(int fd, char ***namesp)
 {
 	char *buf = NULL;
@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 	return err;
 }
 
+int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+		       const struct reftable_write_options *_opts)
+{
+	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
+	struct reftable_write_options opts = { 0 };
+	struct reftable_stack *p;
+	int err;
+
+	p = reftable_calloc(1, sizeof(*p));
+	if (!p) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	if (_opts)
+		opts = *_opts;
+	if (opts.hash_id == 0)
+		opts.hash_id = REFTABLE_HASH_SHA1;
+
+	*dest = NULL;
+
+	reftable_buf_reset(&list_file_name);
+	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
+	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
+		goto out;
+
+	p->list_file = reftable_buf_detach(&list_file_name);
+	p->list_fd = -1;
+	p->opts = opts;
+	p->reftable_dir = reftable_strdup(dir);
+	if (!p->reftable_dir) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	err = reftable_stack_reload_maybe_reuse(p, 1);
+	if (err < 0)
+		goto out;
+
+	*dest = p;
+	err = 0;
+
+out:
+	if (err < 0)
+		reftable_stack_destroy(p);
+	return err;
+}
+
 /* -1 = error
  0 = up to date
  1 = changed. */
@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)
 	return err;
 }
 
-int reftable_stack_add(struct reftable_stack *st,
-		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
-{
-	int err = stack_try_add(st, write, arg);
-	if (err < 0) {
-		if (err == REFTABLE_OUTDATED_ERROR) {
-			/* Ignore error return, we want to propagate
-			   REFTABLE_OUTDATED_ERROR.
-			*/
-			reftable_stack_reload(st);
-		}
-		return err;
-	}
-
-	return 0;
-}
-
-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
-{
-	char buf[100];
-	uint32_t rnd = reftable_rand();
-	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
-		 min, max, rnd);
-	reftable_buf_reset(dest);
-	return reftable_buf_addstr(dest, buf);
-}
-
 struct reftable_addition {
 	struct reftable_flock tables_list_lock;
 	struct reftable_stack *stack;
@@ -706,6 +666,26 @@ struct reftable_addition {
 
 #define REFTABLE_ADDITION_INIT {{0}}
 
+static void reftable_addition_close(struct reftable_addition *add)
+{
+	struct reftable_buf nm = REFTABLE_BUF_INIT;
+	size_t i;
+
+	for (i = 0; i < add->new_tables_len; i++) {
+		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
+			unlink(nm.buf);
+		reftable_free(add->new_tables[i]);
+		add->new_tables[i] = NULL;
+	}
+	reftable_free(add->new_tables);
+	add->new_tables = NULL;
+	add->new_tables_len = 0;
+	add->new_tables_cap = 0;
+
+	flock_release(&add->tables_list_lock);
+	reftable_buf_release(&nm);
+}
+
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
 					unsigned int flags)
@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	return err;
 }
 
-static void reftable_addition_close(struct reftable_addition *add)
+static int stack_try_add(struct reftable_stack *st,
+			 int (*write_table)(struct reftable_writer *wr,
+					    void *arg),
+			 void *arg)
 {
-	struct reftable_buf nm = REFTABLE_BUF_INIT;
-	size_t i;
+	struct reftable_addition add = REFTABLE_ADDITION_INIT;
+	int err = reftable_stack_init_addition(&add, st, 0);
+	if (err < 0)
+		goto done;
 
-	for (i = 0; i < add->new_tables_len; i++) {
-		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
-			unlink(nm.buf);
-		reftable_free(add->new_tables[i]);
-		add->new_tables[i] = NULL;
+	err = reftable_addition_add(&add, write_table, arg);
+	if (err < 0)
+		goto done;
+
+	err = reftable_addition_commit(&add);
+done:
+	reftable_addition_close(&add);
+	return err;
+}
+
+int reftable_stack_add(struct reftable_stack *st,
+		       int (*write)(struct reftable_writer *wr, void *arg),
+		       void *arg)
+{
+	int err = stack_try_add(st, write, arg);
+	if (err < 0) {
+		if (err == REFTABLE_OUTDATED_ERROR) {
+			/* Ignore error return, we want to propagate
+			   REFTABLE_OUTDATED_ERROR.
+			*/
+			reftable_stack_reload(st);
+		}
+		return err;
 	}
-	reftable_free(add->new_tables);
-	add->new_tables = NULL;
-	add->new_tables_len = 0;
-	add->new_tables_cap = 0;
 
-	flock_release(&add->tables_list_lock);
-	reftable_buf_release(&nm);
+	return 0;
+}
+
+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
+{
+	char buf[100];
+	uint32_t rnd = reftable_rand();
+	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
+		 min, max, rnd);
+	reftable_buf_reset(dest);
+	return reftable_buf_addstr(dest, buf);
 }
 
 void reftable_addition_destroy(struct reftable_addition *add)
@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 	return err;
 }
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg)
-{
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_add(&add, write_table, arg);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_commit(&add);
-done:
-	reftable_addition_close(&add);
-	return err;
-}
-
 int reftable_addition_add(struct reftable_addition *add,
 			  int (*write_table)(struct reftable_writer *wr,
 					     void *arg),
@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
 	return 1;
 }
 
-static int stack_compact_locked(struct reftable_stack *st,
-				size_t first, size_t last,
-				struct reftable_log_expiry_config *config,
-				struct reftable_tmpfile *tab_file_out)
-{
-	struct reftable_buf next_name = REFTABLE_BUF_INIT;
-	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
-	struct reftable_writer *wr = NULL;
-	struct fd_writer writer=  {
-		.opts = &st->opts,
-	};
-	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
-	int err = 0;
-
-	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
-			  reftable_table_max_update_index(st->tables[last]));
-	if (err < 0)
-		goto done;
-
-	err = stack_filename(&tab_file_path, st, next_name.buf);
-	if (err < 0)
-		goto done;
-
-	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
-	if (err < 0)
-		goto done;
-
-	if (st->opts.default_permissions &&
-	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
-		err = REFTABLE_IO_ERROR;
-		goto done;
-	}
-
-	writer.fd = tab_file.fd;
-	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
-				  &writer, &st->opts);
-	if (err < 0)
-		goto done;
-
-	err = stack_write_compact(st, wr, first, last, config);
-	if (err < 0)
-		goto done;
-
-	err = reftable_writer_close(wr);
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_close(&tab_file);
-	if (err < 0)
-		goto done;
-
-	*tab_file_out = tab_file;
-	tab_file = REFTABLE_TMPFILE_INIT;
-
-done:
-	tmpfile_delete(&tab_file);
-	reftable_writer_free(wr);
-	reftable_buf_release(&next_name);
-	reftable_buf_release(&tab_file_path);
-	return err;
-}
-
 static int stack_write_compact(struct reftable_stack *st,
 			       struct reftable_writer *wr,
 			       size_t first, size_t last,
@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,
 	return err;
 }
 
+static int stack_compact_locked(struct reftable_stack *st,
+				size_t first, size_t last,
+				struct reftable_log_expiry_config *config,
+				struct reftable_tmpfile *tab_file_out)
+{
+	struct reftable_buf next_name = REFTABLE_BUF_INIT;
+	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
+	struct reftable_writer *wr = NULL;
+	struct fd_writer writer=  {
+		.opts = &st->opts,
+	};
+	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
+	int err = 0;
+
+	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
+			  reftable_table_max_update_index(st->tables[last]));
+	if (err < 0)
+		goto done;
+
+	err = stack_filename(&tab_file_path, st, next_name.buf);
+	if (err < 0)
+		goto done;
+
+	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
+	if (err < 0)
+		goto done;
+
+	if (st->opts.default_permissions &&
+	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
+	writer.fd = tab_file.fd;
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &st->opts);
+	if (err < 0)
+		goto done;
+
+	err = stack_write_compact(st, wr, first, last, config);
+	if (err < 0)
+		goto done;
+
+	err = reftable_writer_close(wr);
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_close(&tab_file);
+	if (err < 0)
+		goto done;
+
+	*tab_file_out = tab_file;
+	tab_file = REFTABLE_TMPFILE_INIT;
+
+done:
+	tmpfile_delete(&tab_file);
+	reftable_writer_free(wr);
+	reftable_buf_release(&next_name);
+	reftable_buf_release(&tab_file_path);
+	return err;
+}
+
 enum stack_compact_range_flags {
 	/*
 	 * Perform a best-effort compaction. That is, even if we cannot lock

-- 
2.50.1

[PATCH 5/5] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

The `reftable_stack_add()` function is a simple wrapper to lock the
stack, add records to it via a callback and then commit the
result. One problem with it though is that it doesn't accept any flags
for creating the addition. This makes it impossible to automatically
reload the stack in case it was modified before we managed to lock the
stack.

Add a `flags` field to plug this gap and pass it through accordingly.
For now this new flag won't be used by us, but it will be used by
libgit2.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c         |  8 +++----
 reftable/reftable-stack.h       |  9 +++++---
 reftable/stack.c                |  8 +++----
 t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4c3817f4ec..3f0deab338 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
 
 done:
 	return ret;
@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 910ec6ef3a..d70fcb705d 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);
  * transaction. Releases the lock if held. */
 void reftable_addition_destroy(struct reftable_addition *add);
 
-/* add a new table to the stack. The write_table function must call
- * reftable_writer_set_limits, add refs and return an error value. */
+/*
+ * Add a new table to the stack. The write_table function must call
+ * reftable_writer_set_limits, add refs and return an error value.
+ * The flags are passed through to `reftable_stack_new_addition()`.
+ */
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write_table)(struct reftable_writer *wr,
 					  void *write_arg),
-		       void *write_arg);
+		       void *write_arg, unsigned flags);
 
 struct reftable_iterator;
 
diff --git a/reftable/stack.c b/reftable/stack.c
index d6e4ea93a3..f77d7f58e8 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 static int stack_try_add(struct reftable_stack *st,
 			 int (*write_table)(struct reftable_writer *wr,
 					    void *arg),
-			 void *arg)
+			 void *arg, unsigned flags)
 {
 	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
+	int err = reftable_stack_init_addition(&add, st, flags);
 	if (err < 0)
 		goto done;
 
@@ -756,9 +756,9 @@ static int stack_try_add(struct reftable_stack *st,
 
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
+		       void *arg, unsigned flags)
 {
-	int err = stack_try_add(st, write, arg);
+	int err = stack_try_add(st, write, arg, flags);
 	if (err < 0) {
 		if (err == REFTABLE_OUTDATED_ERROR) {
 			/* Ignore error return, we want to propagate
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 2f49c97519..ce10247903 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,
 		ref.refname = buf;
 		t_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 	}
 
@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	err = reftable_stack_read_ref(st, ref.refname, &dest);
@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)
 	err = reftable_new_stack(&st2, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st1, write_test_ref, &ref1);
+	err = reftable_stack_add(st1, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_OUTDATED_ERROR);
 
 	err = reftable_stack_reload(st2);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check(!err);
 	reftable_stack_destroy(st1);
 	reftable_stack_destroy(st2);
@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 1);
 	check_int(st->stats.attempts, ==, 0);
@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 2);
 	check_int(st->stats.attempts, ==, 1);
@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref1);
+	err = reftable_stack_add(st, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref2);
+	err = reftable_stack_add(st, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_stack_destroy(st);
 	clear_dir(dir);
@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 	for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {
-		err = reftable_stack_add(st, write_error, &i);
+		err = reftable_stack_add(st, write_error, &i, 0);
 		check_int(err, ==, i);
 	}
 
@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 
-		err = reftable_stack_add(st, write_test_log, &arg);
+		err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)
 	check(!err);
 
 	input.value.update.message = (char *) "one\ntwo";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	input.value.update.message = (char *) "one";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 
 	err = reftable_stack_read_log(st, input.refname, &dest);
@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)
 
 	input.value.update.message = (char *) "two\n";
 	arg.update_index = 2;
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 	err = reftable_stack_read_log(st, input.refname, &dest);
 	check(!err);
@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)
 		}
 	}
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	/* can't read it with the wrong hash ID. */
@@ -927,7 +927,7 @@ static void t_reflog_expire(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -978,7 +978,7 @@ static void t_empty_add(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_nothing, NULL);
+	err = reftable_stack_add(st, write_nothing, NULL, 0);
 	check(!err);
 
 	err = reftable_new_stack(&st2, dir, &opts);
@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)
 		};
 		snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		err = reftable_stack_auto_compact(st);
@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)
 		};
 		xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 
 		check(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));
@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)
 		snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);
 		ref.refname = buf;
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		/*

-- 
2.50.1

[PATCH v2 0/6] reftable: a couple of improvements for libgit2

[Patrick Steinhardt]

Hi,

this small patch series contains a couple of improvements I required for
libgit2. With those changes libgit2 is now able to run its full test
suite with reftable-enabled repositories. I still need to invest a bit
of work to make it memory-leak free and compile on Windows, but overall
I think that support for reftables is almost ready.

Changes in v2:
  - Another commit that fixes handling of outdated stacks when doing
    compaction. This issue is hit in libgit2, which has a test that
    performs writes with a couple dozen concurrent threads.
  - Add a link to past discussions around `{0}` vs `{{0}}` as provided
    by Eric.
  - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (6):
      reftable/writer: fix type used for number of records
      reftable/writer: drop Git-specific `QSORT()` macro
      reftable/stack: fix compiler warning due to missing braces
      reftable/stack: reorder code to avoid forward declarations
      reftable/stack: allow passing flags to `reftable_stack_add()`
      reftable/stack: handle outdated stacks when compacting

 refs/reftable-backend.c         |   8 +-
 reftable/reftable-stack.h       |   9 +-
 reftable/reftable-writer.h      |   4 +-
 reftable/stack.c                | 398 ++++++++++++++++++++--------------------
 reftable/writer.c               |  23 ++-
 t/unit-tests/t-reftable-stack.c |  50 ++---
 6 files changed, 253 insertions(+), 239 deletions(-)

Range-diff versus v1:

1:  e8aacccd51 = 1:  c8bba1f88b reftable/writer: fix type used for number of records
2:  b1a331bc78 = 2:  6c20e9a874 reftable/writer: drop Git-specific `QSORT()` macro
3:  1b3556685d ! 3:  c2ed4eab6a reftable/stack: fix compiler warning due to missing braces
    @@ Commit message
     
         Silence this warning by using `{{0}}` instead.
     
    +    Note that we had the discussion around whether or not we want to handle
    +    such errors in the past already [1], where we basically decided that we
    +    do not care about such old-and-buggy compilers. But the reftable library
    +    is a special case because it is used by projects other than Git, and
    +    libgit2 for example hits the above issue in its pipeline. As there is
    +    only a single problematic instance of this issue we do the pragmatic
    +    thing and simply make the compiler happy.
    +
    +    [1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## reftable/stack.c ##
4:  c1fb437781 = 4:  0f1bb446ce reftable/stack: reorder code to avoid forward declarations
5:  33dcf7ca70 = 5:  5aaff713dd reftable/stack: allow passing flags to `reftable_stack_add()`
-:  ---------- > 6:  ccfa5b59a0 reftable/stack: handle outdated stacks when compacting

---
base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c
change-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603

[PATCH v2 1/6] reftable/writer: fix type used for number of records

[Patrick Steinhardt]

Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`
accept an array of records that should be added to the new table.
Callers of this function are expected to also pass the number of such
records to the function to tell it how many such records it is supposed
to write.

But while all callers pass in a `size_t`, which is a sensible choice,
the function in fact accepts an `int` as argument, which is less so. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-writer.h |  4 ++--
 reftable/writer.c          | 17 +++++++++--------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 0fbeff17f4..1e7003cd69 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
   the records before adding them, reordering the records array passed in.
 */
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n);
+			     struct reftable_ref_record *refs, size_t n);
 
 /*
   adds reftable_log_records. Log records are keyed by (refname, decreasing
@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,
   the records before adding them, reordering records array passed in.
 */
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n);
+			     struct reftable_log_record *logs, size_t n);
 
 /* reftable_writer_close finalizes the reftable. The writer is retained so
  * statistics can be inspected. */
diff --git a/reftable/writer.c b/reftable/writer.c
index 3b4ebdd6dc..5bad130c7e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 }
 
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n)
+			     struct reftable_ref_record *refs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(refs, n, reftable_ref_record_compare_name);
-	for (i = 0; err == 0 && i < n; i++) {
+
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
-	}
+
 	return err;
 }
 
@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,
 }
 
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n)
+			     struct reftable_log_record *logs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(logs, n, reftable_log_record_compare_key);
 
-	for (i = 0; err == 0 && i < n; i++) {
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);
-	}
+
 	return err;
 }
 

-- 
2.50.1.723.g3e08bea96f.dirty

[PATCH v2 2/6] reftable/writer: drop Git-specific `QSORT()` macro

[Patrick Steinhardt]

The reftable writer accidentally uses the Git-specific `QSORT()` macro.
This macro removes the need for the caller to provide the element size,
but other than that it's mostly equivalent to `qsort()`.

Replace the macro accordingly to make the library usable outside of Git.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/writer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index 5bad130c7e..0133b64975 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(refs, n, reftable_ref_record_compare_name);
+	if (n)
+		qsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(logs, n, reftable_log_record_compare_key);
+	if (n)
+		qsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);

-- 
2.50.1.723.g3e08bea96f.dirty

[PATCH v2 3/6] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

While perfectly legal, older compiler toolchains complain when
zero-initializing structs that contain nested structs with `{0}`:

    /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
            struct reftable_addition empty = REFTABLE_ADDITION_INIT;
                                             ^~~~~~~~~~~~~~~~~~~~~~
    /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
    #define REFTABLE_ADDITION_INIT {0}
                                    ^

Silence this warning by using `{{0}}` instead.

Note that we had the discussion around whether or not we want to handle
such errors in the past already [1], where we basically decided that we
do not care about such old-and-buggy compilers. But the reftable library
is a special case because it is used by projects other than Git, and
libgit2 for example hits the above issue in its pipeline. As there is
only a single problematic instance of this issue we do the pragmatic
thing and simply make the compiler happy.

[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/

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 4caf96aa1d..3480ad21c3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -704,7 +704,7 @@ struct reftable_addition {
 	uint64_t next_update_index;
 };
 
-#define REFTABLE_ADDITION_INIT {0}
+#define REFTABLE_ADDITION_INIT {{0}}
 
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,

-- 
2.50.1.723.g3e08bea96f.dirty

[PATCH v2 4/6] reftable/stack: reorder code to avoid forward declarations

[Patrick Steinhardt]

We have a couple of forward declarations in the stack-related code of
the reftable library. These declarations aren't really required, but are
simply caused by unfortunate ordering.

Reorder the code and remove the forward declarations.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------
 1 file changed, 176 insertions(+), 188 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 3480ad21c3..d6e4ea93a3 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,18 +17,6 @@
 #include "table.h"
 #include "writer.h"
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg);
-static int stack_write_compact(struct reftable_stack *st,
-			       struct reftable_writer *wr,
-			       size_t first, size_t last,
-			       struct reftable_log_expiry_config *config);
-static void reftable_addition_close(struct reftable_addition *add);
-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
-					     int reuse_open);
-
 static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 			  const char *name)
 {
@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)
 	return stack_fsync(writer->opts, writer->fd);
 }
 
-int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       const struct reftable_write_options *_opts)
-{
-	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
-	struct reftable_write_options opts = { 0 };
-	struct reftable_stack *p;
-	int err;
-
-	p = reftable_calloc(1, sizeof(*p));
-	if (!p) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	if (_opts)
-		opts = *_opts;
-	if (opts.hash_id == 0)
-		opts.hash_id = REFTABLE_HASH_SHA1;
-
-	*dest = NULL;
-
-	reftable_buf_reset(&list_file_name);
-	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
-	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
-		goto out;
-
-	p->list_file = reftable_buf_detach(&list_file_name);
-	p->list_fd = -1;
-	p->opts = opts;
-	p->reftable_dir = reftable_strdup(dir);
-	if (!p->reftable_dir) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	err = reftable_stack_reload_maybe_reuse(p, 1);
-	if (err < 0)
-		goto out;
-
-	*dest = p;
-	err = 0;
-
-out:
-	if (err < 0)
-		reftable_stack_destroy(p);
-	return err;
-}
-
 static int fd_read_lines(int fd, char ***namesp)
 {
 	char *buf = NULL;
@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 	return err;
 }
 
+int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+		       const struct reftable_write_options *_opts)
+{
+	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
+	struct reftable_write_options opts = { 0 };
+	struct reftable_stack *p;
+	int err;
+
+	p = reftable_calloc(1, sizeof(*p));
+	if (!p) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	if (_opts)
+		opts = *_opts;
+	if (opts.hash_id == 0)
+		opts.hash_id = REFTABLE_HASH_SHA1;
+
+	*dest = NULL;
+
+	reftable_buf_reset(&list_file_name);
+	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
+	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
+		goto out;
+
+	p->list_file = reftable_buf_detach(&list_file_name);
+	p->list_fd = -1;
+	p->opts = opts;
+	p->reftable_dir = reftable_strdup(dir);
+	if (!p->reftable_dir) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	err = reftable_stack_reload_maybe_reuse(p, 1);
+	if (err < 0)
+		goto out;
+
+	*dest = p;
+	err = 0;
+
+out:
+	if (err < 0)
+		reftable_stack_destroy(p);
+	return err;
+}
+
 /* -1 = error
  0 = up to date
  1 = changed. */
@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)
 	return err;
 }
 
-int reftable_stack_add(struct reftable_stack *st,
-		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
-{
-	int err = stack_try_add(st, write, arg);
-	if (err < 0) {
-		if (err == REFTABLE_OUTDATED_ERROR) {
-			/* Ignore error return, we want to propagate
-			   REFTABLE_OUTDATED_ERROR.
-			*/
-			reftable_stack_reload(st);
-		}
-		return err;
-	}
-
-	return 0;
-}
-
-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
-{
-	char buf[100];
-	uint32_t rnd = reftable_rand();
-	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
-		 min, max, rnd);
-	reftable_buf_reset(dest);
-	return reftable_buf_addstr(dest, buf);
-}
-
 struct reftable_addition {
 	struct reftable_flock tables_list_lock;
 	struct reftable_stack *stack;
@@ -706,6 +666,26 @@ struct reftable_addition {
 
 #define REFTABLE_ADDITION_INIT {{0}}
 
+static void reftable_addition_close(struct reftable_addition *add)
+{
+	struct reftable_buf nm = REFTABLE_BUF_INIT;
+	size_t i;
+
+	for (i = 0; i < add->new_tables_len; i++) {
+		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
+			unlink(nm.buf);
+		reftable_free(add->new_tables[i]);
+		add->new_tables[i] = NULL;
+	}
+	reftable_free(add->new_tables);
+	add->new_tables = NULL;
+	add->new_tables_len = 0;
+	add->new_tables_cap = 0;
+
+	flock_release(&add->tables_list_lock);
+	reftable_buf_release(&nm);
+}
+
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
 					unsigned int flags)
@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	return err;
 }
 
-static void reftable_addition_close(struct reftable_addition *add)
+static int stack_try_add(struct reftable_stack *st,
+			 int (*write_table)(struct reftable_writer *wr,
+					    void *arg),
+			 void *arg)
 {
-	struct reftable_buf nm = REFTABLE_BUF_INIT;
-	size_t i;
+	struct reftable_addition add = REFTABLE_ADDITION_INIT;
+	int err = reftable_stack_init_addition(&add, st, 0);
+	if (err < 0)
+		goto done;
 
-	for (i = 0; i < add->new_tables_len; i++) {
-		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
-			unlink(nm.buf);
-		reftable_free(add->new_tables[i]);
-		add->new_tables[i] = NULL;
+	err = reftable_addition_add(&add, write_table, arg);
+	if (err < 0)
+		goto done;
+
+	err = reftable_addition_commit(&add);
+done:
+	reftable_addition_close(&add);
+	return err;
+}
+
+int reftable_stack_add(struct reftable_stack *st,
+		       int (*write)(struct reftable_writer *wr, void *arg),
+		       void *arg)
+{
+	int err = stack_try_add(st, write, arg);
+	if (err < 0) {
+		if (err == REFTABLE_OUTDATED_ERROR) {
+			/* Ignore error return, we want to propagate
+			   REFTABLE_OUTDATED_ERROR.
+			*/
+			reftable_stack_reload(st);
+		}
+		return err;
 	}
-	reftable_free(add->new_tables);
-	add->new_tables = NULL;
-	add->new_tables_len = 0;
-	add->new_tables_cap = 0;
 
-	flock_release(&add->tables_list_lock);
-	reftable_buf_release(&nm);
+	return 0;
+}
+
+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
+{
+	char buf[100];
+	uint32_t rnd = reftable_rand();
+	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
+		 min, max, rnd);
+	reftable_buf_reset(dest);
+	return reftable_buf_addstr(dest, buf);
 }
 
 void reftable_addition_destroy(struct reftable_addition *add)
@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 	return err;
 }
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg)
-{
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_add(&add, write_table, arg);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_commit(&add);
-done:
-	reftable_addition_close(&add);
-	return err;
-}
-
 int reftable_addition_add(struct reftable_addition *add,
 			  int (*write_table)(struct reftable_writer *wr,
 					     void *arg),
@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
 	return 1;
 }
 
-static int stack_compact_locked(struct reftable_stack *st,
-				size_t first, size_t last,
-				struct reftable_log_expiry_config *config,
-				struct reftable_tmpfile *tab_file_out)
-{
-	struct reftable_buf next_name = REFTABLE_BUF_INIT;
-	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
-	struct reftable_writer *wr = NULL;
-	struct fd_writer writer=  {
-		.opts = &st->opts,
-	};
-	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
-	int err = 0;
-
-	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
-			  reftable_table_max_update_index(st->tables[last]));
-	if (err < 0)
-		goto done;
-
-	err = stack_filename(&tab_file_path, st, next_name.buf);
-	if (err < 0)
-		goto done;
-
-	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
-	if (err < 0)
-		goto done;
-
-	if (st->opts.default_permissions &&
-	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
-		err = REFTABLE_IO_ERROR;
-		goto done;
-	}
-
-	writer.fd = tab_file.fd;
-	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
-				  &writer, &st->opts);
-	if (err < 0)
-		goto done;
-
-	err = stack_write_compact(st, wr, first, last, config);
-	if (err < 0)
-		goto done;
-
-	err = reftable_writer_close(wr);
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_close(&tab_file);
-	if (err < 0)
-		goto done;
-
-	*tab_file_out = tab_file;
-	tab_file = REFTABLE_TMPFILE_INIT;
-
-done:
-	tmpfile_delete(&tab_file);
-	reftable_writer_free(wr);
-	reftable_buf_release(&next_name);
-	reftable_buf_release(&tab_file_path);
-	return err;
-}
-
 static int stack_write_compact(struct reftable_stack *st,
 			       struct reftable_writer *wr,
 			       size_t first, size_t last,
@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,
 	return err;
 }
 
+static int stack_compact_locked(struct reftable_stack *st,
+				size_t first, size_t last,
+				struct reftable_log_expiry_config *config,
+				struct reftable_tmpfile *tab_file_out)
+{
+	struct reftable_buf next_name = REFTABLE_BUF_INIT;
+	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
+	struct reftable_writer *wr = NULL;
+	struct fd_writer writer=  {
+		.opts = &st->opts,
+	};
+	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
+	int err = 0;
+
+	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
+			  reftable_table_max_update_index(st->tables[last]));
+	if (err < 0)
+		goto done;
+
+	err = stack_filename(&tab_file_path, st, next_name.buf);
+	if (err < 0)
+		goto done;
+
+	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
+	if (err < 0)
+		goto done;
+
+	if (st->opts.default_permissions &&
+	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
+	writer.fd = tab_file.fd;
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &st->opts);
+	if (err < 0)
+		goto done;
+
+	err = stack_write_compact(st, wr, first, last, config);
+	if (err < 0)
+		goto done;
+
+	err = reftable_writer_close(wr);
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_close(&tab_file);
+	if (err < 0)
+		goto done;
+
+	*tab_file_out = tab_file;
+	tab_file = REFTABLE_TMPFILE_INIT;
+
+done:
+	tmpfile_delete(&tab_file);
+	reftable_writer_free(wr);
+	reftable_buf_release(&next_name);
+	reftable_buf_release(&tab_file_path);
+	return err;
+}
+
 enum stack_compact_range_flags {
 	/*
 	 * Perform a best-effort compaction. That is, even if we cannot lock

-- 
2.50.1.723.g3e08bea96f.dirty

Re: [PATCH v2 4/6] reftable/stack: reorder code to avoid forward declarations

[Justin Tobler]

On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> We have a couple of forward declarations in the stack-related code of
> the reftable library. These declarations aren't really required, but are
> simply caused by unfortunate ordering.
> 
> Reorder the code and remove the forward declarations.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 176 insertions(+), 188 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 3480ad21c3..d6e4ea93a3 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -17,18 +17,6 @@
>  #include "table.h"
>  #include "writer.h"
>  
> -static int stack_try_add(struct reftable_stack *st,
> -			 int (*write_table)(struct reftable_writer *wr,
> -					    void *arg),
> -			 void *arg);
> -static int stack_write_compact(struct reftable_stack *st,
> -			       struct reftable_writer *wr,
> -			       size_t first, size_t last,
> -			       struct reftable_log_expiry_config *config);
> -static void reftable_addition_close(struct reftable_addition *add);
> -static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
> -					     int reuse_open);
> -

Nice cleanup :)

[PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

The `reftable_stack_add()` function is a simple wrapper to lock the
stack, add records to it via a callback and then commit the
result. One problem with it though is that it doesn't accept any flags
for creating the addition. This makes it impossible to automatically
reload the stack in case it was modified before we managed to lock the
stack.

Add a `flags` field to plug this gap and pass it through accordingly.
For now this new flag won't be used by us, but it will be used by
libgit2.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c         |  8 +++----
 reftable/reftable-stack.h       |  9 +++++---
 reftable/stack.c                |  8 +++----
 t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4c3817f4ec1..3f0deab338c 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
 
 done:
 	return ret;
@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 910ec6ef3a2..d70fcb705dc 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);
  * transaction. Releases the lock if held. */
 void reftable_addition_destroy(struct reftable_addition *add);
 
-/* add a new table to the stack. The write_table function must call
- * reftable_writer_set_limits, add refs and return an error value. */
+/*
+ * Add a new table to the stack. The write_table function must call
+ * reftable_writer_set_limits, add refs and return an error value.
+ * The flags are passed through to `reftable_stack_new_addition()`.
+ */
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write_table)(struct reftable_writer *wr,
 					  void *write_arg),
-		       void *write_arg);
+		       void *write_arg, unsigned flags);
 
 struct reftable_iterator;
 
diff --git a/reftable/stack.c b/reftable/stack.c
index d6e4ea93a37..f77d7f58e8e 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 static int stack_try_add(struct reftable_stack *st,
 			 int (*write_table)(struct reftable_writer *wr,
 					    void *arg),
-			 void *arg)
+			 void *arg, unsigned flags)
 {
 	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
+	int err = reftable_stack_init_addition(&add, st, flags);
 	if (err < 0)
 		goto done;
 
@@ -756,9 +756,9 @@ static int stack_try_add(struct reftable_stack *st,
 
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
+		       void *arg, unsigned flags)
 {
-	int err = stack_try_add(st, write, arg);
+	int err = stack_try_add(st, write, arg, flags);
 	if (err < 0) {
 		if (err == REFTABLE_OUTDATED_ERROR) {
 			/* Ignore error return, we want to propagate
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 2f49c975194..ce10247903c 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,
 		ref.refname = buf;
 		t_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 	}
 
@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	err = reftable_stack_read_ref(st, ref.refname, &dest);
@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)
 	err = reftable_new_stack(&st2, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st1, write_test_ref, &ref1);
+	err = reftable_stack_add(st1, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_OUTDATED_ERROR);
 
 	err = reftable_stack_reload(st2);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check(!err);
 	reftable_stack_destroy(st1);
 	reftable_stack_destroy(st2);
@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 1);
 	check_int(st->stats.attempts, ==, 0);
@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 2);
 	check_int(st->stats.attempts, ==, 1);
@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref1);
+	err = reftable_stack_add(st, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref2);
+	err = reftable_stack_add(st, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_stack_destroy(st);
 	clear_dir(dir);
@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 	for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {
-		err = reftable_stack_add(st, write_error, &i);
+		err = reftable_stack_add(st, write_error, &i, 0);
 		check_int(err, ==, i);
 	}
 
@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 
-		err = reftable_stack_add(st, write_test_log, &arg);
+		err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)
 	check(!err);
 
 	input.value.update.message = (char *) "one\ntwo";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	input.value.update.message = (char *) "one";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 
 	err = reftable_stack_read_log(st, input.refname, &dest);
@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)
 
 	input.value.update.message = (char *) "two\n";
 	arg.update_index = 2;
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 	err = reftable_stack_read_log(st, input.refname, &dest);
 	check(!err);
@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)
 		}
 	}
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	/* can't read it with the wrong hash ID. */
@@ -927,7 +927,7 @@ static void t_reflog_expire(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -978,7 +978,7 @@ static void t_empty_add(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_nothing, NULL);
+	err = reftable_stack_add(st, write_nothing, NULL, 0);
 	check(!err);
 
 	err = reftable_new_stack(&st2, dir, &opts);
@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)
 		};
 		snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		err = reftable_stack_auto_compact(st);
@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)
 		};
 		xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 
 		check(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));
@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)
 		snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);
 		ref.refname = buf;
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		/*

-- 
2.50.1.723.g3e08bea96f.dirty

Re: [PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`

[Justin Tobler]

On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> The `reftable_stack_add()` function is a simple wrapper to lock the
> stack, add records to it via a callback and then commit the
> result. One problem with it though is that it doesn't accept any flags
> for creating the addition. This makes it impossible to automatically
> reload the stack in case it was modified before we managed to lock the
> stack.
> 
> Add a `flags` field to plug this gap and pass it through accordingly.
> For now this new flag won't be used by us, but it will be used by
> libgit2.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c         |  8 +++----
>  reftable/reftable-stack.h       |  9 +++++---
>  reftable/stack.c                |  8 +++----
>  t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------
>  4 files changed, 39 insertions(+), 36 deletions(-)
> 
[snip]
> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
> index 910ec6ef3a2..d70fcb705dc 100644
> --- a/reftable/reftable-stack.h
> +++ b/reftable/reftable-stack.h
> @@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);
>   * transaction. Releases the lock if held. */
>  void reftable_addition_destroy(struct reftable_addition *add);
>  
> -/* add a new table to the stack. The write_table function must call
> - * reftable_writer_set_limits, add refs and return an error value. */
> +/*
> + * Add a new table to the stack. The write_table function must call
> + * reftable_writer_set_limits, add refs and return an error value.
> + * The flags are passed through to `reftable_stack_new_addition()`.
> + */
>  int reftable_stack_add(struct reftable_stack *st,
>  		       int (*write_table)(struct reftable_writer *wr,
>  					  void *write_arg),
> -		       void *write_arg);
> +		       void *write_arg, unsigned flags);
>  
>  struct reftable_iterator;
>  
> diff --git a/reftable/stack.c b/reftable/stack.c
> index d6e4ea93a37..f77d7f58e8e 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  static int stack_try_add(struct reftable_stack *st,
>  			 int (*write_table)(struct reftable_writer *wr,
>  					    void *arg),
> -			 void *arg)
> +			 void *arg, unsigned flags)
>  {
>  	struct reftable_addition add = REFTABLE_ADDITION_INIT;
> -	int err = reftable_stack_init_addition(&add, st, 0);
> +	int err = reftable_stack_init_addition(&add, st, flags);

Ok, so now if the `REFTABLE_STACK_NEW_ADDITION_RELOAD` flag is provided,
reftable_stack_init_addition() will attempt to reload the stack if it is
outdated before locking the stack. I assume Git itself hasn't needed
this because it just uses reftable_stack_new_addition() directly when
neccessary.

This patch looks good.

-Justin

Re: [PATCH v2 5/6] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

On Mon, Aug 11, 2025 at 02:34:49PM -0500, Justin Tobler wrote:
> On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index d6e4ea93a37..f77d7f58e8e 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -737,10 +737,10 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> >  static int stack_try_add(struct reftable_stack *st,
> >  			 int (*write_table)(struct reftable_writer *wr,
> >  					    void *arg),
> > -			 void *arg)
> > +			 void *arg, unsigned flags)
> >  {
> >  	struct reftable_addition add = REFTABLE_ADDITION_INIT;
> > -	int err = reftable_stack_init_addition(&add, st, 0);
> > +	int err = reftable_stack_init_addition(&add, st, flags);
> 
> Ok, so now if the `REFTABLE_STACK_NEW_ADDITION_RELOAD` flag is provided,
> reftable_stack_init_addition() will attempt to reload the stack if it is
> outdated before locking the stack. I assume Git itself hasn't needed
> this because it just uses reftable_stack_new_addition() directly when
> neccessary.

Not in all code paths, and arguably Git should pass the flag in those
code paths that don't yet. We already perform all verification under the
lock anyway, so this would be the right thing to do.

In this patch series I wanted to focus on improvements to the library
for now, not to the backend. But honestly, there isn't really a good
reason to not already fix this while we're working on this.

Patrick

[PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting

[Patrick Steinhardt]

When we compact the reftable stack we first acquire the lock for the
"tables.list" file and then reload the stack to check that it is still
up-to-date. This is done by calling `stack_uptodate()`, which knows to
return zero in case the stack is up-to-date, a positive value if it is
not and a negative error code on unexpected conditions.

We don't do proper error checking though, but instead we only check
whether the returned error code is non-zero. If so, we simply bubble it
up the calling stack, which means that callers may see an unexpected
positive value.

Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
Handle this situation in `reftable_addition_commit()`, where we perform
a best-effort auto-compaction.

All other callsites of `stack_uptodate()` know to handle a positive
return value and thus don't need to be fixed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index f77d7f58e8..effa2fc8cb 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
 	return err;
 }
 
-/* -1 = error
- 0 = up to date
- 1 = changed. */
+/*
+ * Check whether the given stack is up-to-date with what we have in memory.
+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
+ * otherwise.
+ */
 static int stack_uptodate(struct reftable_stack *st)
 {
 	char **names = NULL;
@@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)
 		 * control. It is possible that a concurrent writer is already
 		 * trying to compact parts of the stack, which would lead to a
 		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
-		 * already. This is a benign error though, so we ignore it.
+		 * already. Similarly, the stack may have been rewritten by a
+		 * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
+		 * Both of these errors are benign, so we simply ignore them.
 		 */
 		err = reftable_stack_auto_compact(add->stack);
-		if (err < 0 && err != REFTABLE_LOCK_ERROR)
+		if (err < 0 && err != REFTABLE_LOCK_ERROR &&
+		    err != REFTABLE_OUTDATED_ERROR)
 			goto done;
 		err = 0;
 	}
@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
 		goto done;
 	}
 
+	/*
+	 * Check whether the stack is up-to-date. We unfortunately cannot
+	 * handle the situation gracefully in case it's _not_ up-to-date
+	 * because the range of tables that the user has requested us to
+	 * compact may have been changed. So instead we abort.
+	 *
+	 * We could in theory improve the situation by having the caller not
+	 * pass in a range, but instead the list of tables to compact. If so,
+	 * we could check that relevant tables still exist. But for now it's
+	 * good enough to just abort.
+	 */
 	err = stack_uptodate(st);
-	if (err)
+	if (err < 0)
 		goto done;
+	if (err > 0) {
+		err = REFTABLE_OUTDATED_ERROR;
+		goto done;
+	}
 
 	/*
 	 * Lock all tables in the user-provided range. This is the slice of our

-- 
2.50.1.723.g3e08bea96f.dirty

Re: [PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting

[Justin Tobler]

On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> When we compact the reftable stack we first acquire the lock for the
> "tables.list" file and then reload the stack to check that it is still
> up-to-date. This is done by calling `stack_uptodate()`, which knows to
> return zero in case the stack is up-to-date, a positive value if it is
> not and a negative error code on unexpected conditions.

So `stack_uptodate()` returns a negative value for error cases and a
positive value if the stack is out of date. `REFTABLE_OUTDATED_ERROR` is
really also an error, but it is special cased to differentiate it from
the others.

> We don't do proper error checking though, but instead we only check
> whether the returned error code is non-zero. If so, we simply bubble it
> up the calling stack, which means that callers may see an unexpected
> positive value.
> 
> Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
> Handle this situation in `reftable_addition_commit()`, where we perform
> a best-effort auto-compaction.
> 
> All other callsites of `stack_uptodate()` know to handle a positive
> return value and thus don't need to be fixed.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index f77d7f58e8..effa2fc8cb 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
>  	return err;
>  }
>  
> -/* -1 = error
> - 0 = up to date
> - 1 = changed. */
> +/*
> + * Check whether the given stack is up-to-date with what we have in memory.
> + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
> + * otherwise.
> + */
>  static int stack_uptodate(struct reftable_stack *st)
>  {
>  	char **names = NULL;
> @@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)
>  		 * control. It is possible that a concurrent writer is already
>  		 * trying to compact parts of the stack, which would lead to a
>  		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
> -		 * already. This is a benign error though, so we ignore it.
> +		 * already. Similarly, the stack may have been rewritten by a
> +		 * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
> +		 * Both of these errors are benign, so we simply ignore them.
>  		 */
>  		err = reftable_stack_auto_compact(add->stack);
> -		if (err < 0 && err != REFTABLE_LOCK_ERROR)
> +		if (err < 0 && err != REFTABLE_LOCK_ERROR &&
> +		    err != REFTABLE_OUTDATED_ERROR)
>  			goto done;
>  		err = 0;
>  	}
> @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
>  		goto done;
>  	}
>  
> +	/*
> +	 * Check whether the stack is up-to-date. We unfortunately cannot
> +	 * handle the situation gracefully in case it's _not_ up-to-date
> +	 * because the range of tables that the user has requested us to
> +	 * compact may have been changed. So instead we abort.
> +	 *
> +	 * We could in theory improve the situation by having the caller not
> +	 * pass in a range, but instead the list of tables to compact. If so,
> +	 * we could check that relevant tables still exist. But for now it's
> +	 * good enough to just abort.
> +	 */
>  	err = stack_uptodate(st);
> -	if (err)
> +	if (err < 0)
>  		goto done;
> +	if (err > 0) {
> +		err = REFTABLE_OUTDATED_ERROR;
> +		goto done;
> +	}

I was thinking that maybe `stack_uptodate()` could maybe handle
returning the `REFTABLE_OUTDATED_ERROR` directly so we could avoid
having to map the error here. This could require callers to check for
`err == REFTABLE_OUTDATED_ERROR` instead of `err > 0`. Probably not a
big deal either way though.

Otherwise this looks good to me :)

-Justin

>  
>  	/*
>  	 * Lock all tables in the user-provided range. This is the slice of our
> 
> -- 
> 2.50.1.723.g3e08bea96f.dirty
> 
> 

Re: [PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting

[Patrick Steinhardt]

On Mon, Aug 11, 2025 at 03:04:05PM -0500, Justin Tobler wrote:
> On 25/08/04 11:40AM, Patrick Steinhardt wrote:
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index f77d7f58e8..effa2fc8cb 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
> >  		goto done;
> >  	}
> >  
> > +	/*
> > +	 * Check whether the stack is up-to-date. We unfortunately cannot
> > +	 * handle the situation gracefully in case it's _not_ up-to-date
> > +	 * because the range of tables that the user has requested us to
> > +	 * compact may have been changed. So instead we abort.
> > +	 *
> > +	 * We could in theory improve the situation by having the caller not
> > +	 * pass in a range, but instead the list of tables to compact. If so,
> > +	 * we could check that relevant tables still exist. But for now it's
> > +	 * good enough to just abort.
> > +	 */
> >  	err = stack_uptodate(st);
> > -	if (err)
> > +	if (err < 0)
> >  		goto done;
> > +	if (err > 0) {
> > +		err = REFTABLE_OUTDATED_ERROR;
> > +		goto done;
> > +	}
> 
> I was thinking that maybe `stack_uptodate()` could maybe handle
> returning the `REFTABLE_OUTDATED_ERROR` directly so we could avoid
> having to map the error here. This could require callers to check for
> `err == REFTABLE_OUTDATED_ERROR` instead of `err > 0`. Probably not a
> big deal either way though.

We probably could, but I don't see a strong-enough reason to do so now.

Patrick

[PATCH v3 0/8] reftable: a couple of improvements for libgit2

[Patrick Steinhardt]

Hi,

this small patch series contains a couple of improvements I required for
libgit2. With those changes libgit2 is now able to run its full test
suite with reftable-enabled repositories. I still need to invest a bit
of work to make it memory-leak free and compile on Windows, but overall
I think that support for reftables is almost ready.

Changes in v2:
  - Another commit that fixes handling of outdated stacks when doing
    compaction. This issue is hit in libgit2, which has a test that
    performs writes with a couple dozen concurrent threads.
  - Add a link to past discussions around `{0}` vs `{{0}}` as provided
    by Eric.
  - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im

Changes in v3:
  - Adapt `reftable_stack_init_addition()` to `memset()` the addition
    instead of using `{{0}}`.
  - Add another commit on top that makes the `flock` interface more
    robust by not relying on `errno`.
  - Add another commit to fix races with concurrent writers caused by
    the backend not passing `REFTABLE_STACK_NEW_ADDITION_RELOAD`.
  - Link to v2: https://lore.kernel.org/r/20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (8):
      reftable/writer: fix type used for number of records
      reftable/writer: drop Git-specific `QSORT()` macro
      reftable/stack: reorder code to avoid forward declarations
      reftable/stack: fix compiler warning due to missing braces
      reftable/stack: allow passing flags to `reftable_stack_add()`
      reftable/stack: handle outdated stacks when compacting
      reftable: don't second-guess errors from flock interface
      refs/reftable: always reload stacks when creating lock

 refs/reftable-backend.c         |  23 ++-
 reftable/reftable-stack.h       |   9 +-
 reftable/reftable-writer.h      |   4 +-
 reftable/stack.c                | 439 +++++++++++++++++++---------------------
 reftable/system.c               |   2 +-
 reftable/system.h               |   4 +-
 reftable/writer.c               |  23 ++-
 t/unit-tests/t-reftable-stack.c |  50 ++---
 8 files changed, 275 insertions(+), 279 deletions(-)

Range-diff versus v2:

1:  ce08ba1217 = 1:  1613715dc9 reftable/writer: fix type used for number of records
2:  80a87ff19a = 2:  4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro
3:  e230c97347 < -:  ---------- reftable/stack: fix compiler warning due to missing braces
4:  dd413b76a2 ! 3:  3977a1f497 reftable/stack: reorder code to avoid forward declarations
    @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)
      	struct reftable_stack *stack;
     @@ reftable/stack.c: struct reftable_addition {
      
    - #define REFTABLE_ADDITION_INIT {{0}}
    + #define REFTABLE_ADDITION_INIT {0}
      
     +static void reftable_addition_close(struct reftable_addition *add)
     +{
-:  ---------- > 4:  e8424dd2ee reftable/stack: fix compiler warning due to missing braces
5:  494bbd6c97 ! 5:  4f0f5cd8e1 reftable/stack: allow passing flags to `reftable_stack_add()`
    @@ reftable/stack.c: static int reftable_stack_init_addition(struct reftable_additi
     -			 void *arg)
     +			 void *arg, unsigned flags)
      {
    - 	struct reftable_addition add = REFTABLE_ADDITION_INIT;
    --	int err = reftable_stack_init_addition(&add, st, 0);
    -+	int err = reftable_stack_init_addition(&add, st, flags);
    + 	struct reftable_addition add;
    + 	int err;
    + 
    +-	err = reftable_stack_init_addition(&add, st, 0);
    ++	err = reftable_stack_init_addition(&add, st, flags);
      	if (err < 0)
      		goto done;
      
6:  46411bf2c2 = 6:  e6e06c8d34 reftable/stack: handle outdated stacks when compacting
-:  ---------- > 7:  86d57dad6e reftable: don't second-guess errors from flock interface
-:  ---------- > 8:  b7bb745ffb refs/reftable: always reload stacks when creating lock

---
base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c
change-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603

[PATCH v3 1/8] reftable/writer: fix type used for number of records

[Patrick Steinhardt]

Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`
accept an array of records that should be added to the new table.
Callers of this function are expected to also pass the number of such
records to the function to tell it how many such records it is supposed
to write.

But while all callers pass in a `size_t`, which is a sensible choice,
the function in fact accepts an `int` as argument, which is less so. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-writer.h |  4 ++--
 reftable/writer.c          | 17 +++++++++--------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 0fbeff17f4..1e7003cd69 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
   the records before adding them, reordering the records array passed in.
 */
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n);
+			     struct reftable_ref_record *refs, size_t n);
 
 /*
   adds reftable_log_records. Log records are keyed by (refname, decreasing
@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,
   the records before adding them, reordering records array passed in.
 */
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n);
+			     struct reftable_log_record *logs, size_t n);
 
 /* reftable_writer_close finalizes the reftable. The writer is retained so
  * statistics can be inspected. */
diff --git a/reftable/writer.c b/reftable/writer.c
index 3b4ebdd6dc..5bad130c7e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 }
 
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n)
+			     struct reftable_ref_record *refs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(refs, n, reftable_ref_record_compare_name);
-	for (i = 0; err == 0 && i < n; i++) {
+
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
-	}
+
 	return err;
 }
 
@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,
 }
 
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n)
+			     struct reftable_log_record *logs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(logs, n, reftable_log_record_compare_key);
 
-	for (i = 0; err == 0 && i < n; i++) {
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);
-	}
+
 	return err;
 }
 

-- 
2.51.0.rc1.163.g2494970778.dirty

[PATCH v3 2/8] reftable/writer: drop Git-specific `QSORT()` macro

[Patrick Steinhardt]

The reftable writer accidentally uses the Git-specific `QSORT()` macro.
This macro removes the need for the caller to provide the element size,
but other than that it's mostly equivalent to `qsort()`.

Replace the macro accordingly to make the library usable outside of Git.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/writer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index 5bad130c7e..0133b64975 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(refs, n, reftable_ref_record_compare_name);
+	if (n)
+		qsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(logs, n, reftable_log_record_compare_key);
+	if (n)
+		qsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);

-- 
2.51.0.rc1.163.g2494970778.dirty

[PATCH v3 3/8] reftable/stack: reorder code to avoid forward declarations

[Patrick Steinhardt]

We have a couple of forward declarations in the stack-related code of
the reftable library. These declarations aren't really required, but are
simply caused by unfortunate ordering.

Reorder the code and remove the forward declarations.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------
 1 file changed, 176 insertions(+), 188 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 4caf96aa1d..ed80710572 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,18 +17,6 @@
 #include "table.h"
 #include "writer.h"
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg);
-static int stack_write_compact(struct reftable_stack *st,
-			       struct reftable_writer *wr,
-			       size_t first, size_t last,
-			       struct reftable_log_expiry_config *config);
-static void reftable_addition_close(struct reftable_addition *add);
-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
-					     int reuse_open);
-
 static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 			  const char *name)
 {
@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)
 	return stack_fsync(writer->opts, writer->fd);
 }
 
-int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       const struct reftable_write_options *_opts)
-{
-	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
-	struct reftable_write_options opts = { 0 };
-	struct reftable_stack *p;
-	int err;
-
-	p = reftable_calloc(1, sizeof(*p));
-	if (!p) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	if (_opts)
-		opts = *_opts;
-	if (opts.hash_id == 0)
-		opts.hash_id = REFTABLE_HASH_SHA1;
-
-	*dest = NULL;
-
-	reftable_buf_reset(&list_file_name);
-	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
-	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
-		goto out;
-
-	p->list_file = reftable_buf_detach(&list_file_name);
-	p->list_fd = -1;
-	p->opts = opts;
-	p->reftable_dir = reftable_strdup(dir);
-	if (!p->reftable_dir) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	err = reftable_stack_reload_maybe_reuse(p, 1);
-	if (err < 0)
-		goto out;
-
-	*dest = p;
-	err = 0;
-
-out:
-	if (err < 0)
-		reftable_stack_destroy(p);
-	return err;
-}
-
 static int fd_read_lines(int fd, char ***namesp)
 {
 	char *buf = NULL;
@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 	return err;
 }
 
+int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+		       const struct reftable_write_options *_opts)
+{
+	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
+	struct reftable_write_options opts = { 0 };
+	struct reftable_stack *p;
+	int err;
+
+	p = reftable_calloc(1, sizeof(*p));
+	if (!p) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	if (_opts)
+		opts = *_opts;
+	if (opts.hash_id == 0)
+		opts.hash_id = REFTABLE_HASH_SHA1;
+
+	*dest = NULL;
+
+	reftable_buf_reset(&list_file_name);
+	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
+	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
+		goto out;
+
+	p->list_file = reftable_buf_detach(&list_file_name);
+	p->list_fd = -1;
+	p->opts = opts;
+	p->reftable_dir = reftable_strdup(dir);
+	if (!p->reftable_dir) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	err = reftable_stack_reload_maybe_reuse(p, 1);
+	if (err < 0)
+		goto out;
+
+	*dest = p;
+	err = 0;
+
+out:
+	if (err < 0)
+		reftable_stack_destroy(p);
+	return err;
+}
+
 /* -1 = error
  0 = up to date
  1 = changed. */
@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)
 	return err;
 }
 
-int reftable_stack_add(struct reftable_stack *st,
-		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
-{
-	int err = stack_try_add(st, write, arg);
-	if (err < 0) {
-		if (err == REFTABLE_OUTDATED_ERROR) {
-			/* Ignore error return, we want to propagate
-			   REFTABLE_OUTDATED_ERROR.
-			*/
-			reftable_stack_reload(st);
-		}
-		return err;
-	}
-
-	return 0;
-}
-
-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
-{
-	char buf[100];
-	uint32_t rnd = reftable_rand();
-	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
-		 min, max, rnd);
-	reftable_buf_reset(dest);
-	return reftable_buf_addstr(dest, buf);
-}
-
 struct reftable_addition {
 	struct reftable_flock tables_list_lock;
 	struct reftable_stack *stack;
@@ -706,6 +666,26 @@ struct reftable_addition {
 
 #define REFTABLE_ADDITION_INIT {0}
 
+static void reftable_addition_close(struct reftable_addition *add)
+{
+	struct reftable_buf nm = REFTABLE_BUF_INIT;
+	size_t i;
+
+	for (i = 0; i < add->new_tables_len; i++) {
+		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
+			unlink(nm.buf);
+		reftable_free(add->new_tables[i]);
+		add->new_tables[i] = NULL;
+	}
+	reftable_free(add->new_tables);
+	add->new_tables = NULL;
+	add->new_tables_len = 0;
+	add->new_tables_cap = 0;
+
+	flock_release(&add->tables_list_lock);
+	reftable_buf_release(&nm);
+}
+
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
 					unsigned int flags)
@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	return err;
 }
 
-static void reftable_addition_close(struct reftable_addition *add)
+static int stack_try_add(struct reftable_stack *st,
+			 int (*write_table)(struct reftable_writer *wr,
+					    void *arg),
+			 void *arg)
 {
-	struct reftable_buf nm = REFTABLE_BUF_INIT;
-	size_t i;
+	struct reftable_addition add = REFTABLE_ADDITION_INIT;
+	int err = reftable_stack_init_addition(&add, st, 0);
+	if (err < 0)
+		goto done;
 
-	for (i = 0; i < add->new_tables_len; i++) {
-		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
-			unlink(nm.buf);
-		reftable_free(add->new_tables[i]);
-		add->new_tables[i] = NULL;
+	err = reftable_addition_add(&add, write_table, arg);
+	if (err < 0)
+		goto done;
+
+	err = reftable_addition_commit(&add);
+done:
+	reftable_addition_close(&add);
+	return err;
+}
+
+int reftable_stack_add(struct reftable_stack *st,
+		       int (*write)(struct reftable_writer *wr, void *arg),
+		       void *arg)
+{
+	int err = stack_try_add(st, write, arg);
+	if (err < 0) {
+		if (err == REFTABLE_OUTDATED_ERROR) {
+			/* Ignore error return, we want to propagate
+			   REFTABLE_OUTDATED_ERROR.
+			*/
+			reftable_stack_reload(st);
+		}
+		return err;
 	}
-	reftable_free(add->new_tables);
-	add->new_tables = NULL;
-	add->new_tables_len = 0;
-	add->new_tables_cap = 0;
 
-	flock_release(&add->tables_list_lock);
-	reftable_buf_release(&nm);
+	return 0;
+}
+
+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
+{
+	char buf[100];
+	uint32_t rnd = reftable_rand();
+	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
+		 min, max, rnd);
+	reftable_buf_reset(dest);
+	return reftable_buf_addstr(dest, buf);
 }
 
 void reftable_addition_destroy(struct reftable_addition *add)
@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 	return err;
 }
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg)
-{
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_add(&add, write_table, arg);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_commit(&add);
-done:
-	reftable_addition_close(&add);
-	return err;
-}
-
 int reftable_addition_add(struct reftable_addition *add,
 			  int (*write_table)(struct reftable_writer *wr,
 					     void *arg),
@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
 	return 1;
 }
 
-static int stack_compact_locked(struct reftable_stack *st,
-				size_t first, size_t last,
-				struct reftable_log_expiry_config *config,
-				struct reftable_tmpfile *tab_file_out)
-{
-	struct reftable_buf next_name = REFTABLE_BUF_INIT;
-	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
-	struct reftable_writer *wr = NULL;
-	struct fd_writer writer=  {
-		.opts = &st->opts,
-	};
-	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
-	int err = 0;
-
-	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
-			  reftable_table_max_update_index(st->tables[last]));
-	if (err < 0)
-		goto done;
-
-	err = stack_filename(&tab_file_path, st, next_name.buf);
-	if (err < 0)
-		goto done;
-
-	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
-	if (err < 0)
-		goto done;
-
-	if (st->opts.default_permissions &&
-	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
-		err = REFTABLE_IO_ERROR;
-		goto done;
-	}
-
-	writer.fd = tab_file.fd;
-	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
-				  &writer, &st->opts);
-	if (err < 0)
-		goto done;
-
-	err = stack_write_compact(st, wr, first, last, config);
-	if (err < 0)
-		goto done;
-
-	err = reftable_writer_close(wr);
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_close(&tab_file);
-	if (err < 0)
-		goto done;
-
-	*tab_file_out = tab_file;
-	tab_file = REFTABLE_TMPFILE_INIT;
-
-done:
-	tmpfile_delete(&tab_file);
-	reftable_writer_free(wr);
-	reftable_buf_release(&next_name);
-	reftable_buf_release(&tab_file_path);
-	return err;
-}
-
 static int stack_write_compact(struct reftable_stack *st,
 			       struct reftable_writer *wr,
 			       size_t first, size_t last,
@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,
 	return err;
 }
 
+static int stack_compact_locked(struct reftable_stack *st,
+				size_t first, size_t last,
+				struct reftable_log_expiry_config *config,
+				struct reftable_tmpfile *tab_file_out)
+{
+	struct reftable_buf next_name = REFTABLE_BUF_INIT;
+	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
+	struct reftable_writer *wr = NULL;
+	struct fd_writer writer=  {
+		.opts = &st->opts,
+	};
+	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
+	int err = 0;
+
+	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
+			  reftable_table_max_update_index(st->tables[last]));
+	if (err < 0)
+		goto done;
+
+	err = stack_filename(&tab_file_path, st, next_name.buf);
+	if (err < 0)
+		goto done;
+
+	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
+	if (err < 0)
+		goto done;
+
+	if (st->opts.default_permissions &&
+	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
+	writer.fd = tab_file.fd;
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &st->opts);
+	if (err < 0)
+		goto done;
+
+	err = stack_write_compact(st, wr, first, last, config);
+	if (err < 0)
+		goto done;
+
+	err = reftable_writer_close(wr);
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_close(&tab_file);
+	if (err < 0)
+		goto done;
+
+	*tab_file_out = tab_file;
+	tab_file = REFTABLE_TMPFILE_INIT;
+
+done:
+	tmpfile_delete(&tab_file);
+	reftable_writer_free(wr);
+	reftable_buf_release(&next_name);
+	reftable_buf_release(&tab_file_path);
+	return err;
+}
+
 enum stack_compact_range_flags {
 	/*
 	 * Perform a best-effort compaction. That is, even if we cannot lock

-- 
2.51.0.rc1.163.g2494970778.dirty

[PATCH v3 4/8] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

While perfectly legal, older compiler toolchains complain when
zero-initializing structs that contain nested structs with `{0}`:

    /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
            struct reftable_addition empty = REFTABLE_ADDITION_INIT;
                                             ^~~~~~~~~~~~~~~~~~~~~~
    /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
    #define REFTABLE_ADDITION_INIT {0}
                                    ^

We had the discussion around whether or not we want to handle such bogus
compiler errors in the past already [1]. Back then we basically decided
that we do not care about such old-and-buggy compilers, so while we
could fix the issue by using `{{0}}` instead this is not the preferred
way to handle this in the Git codebase.

We have an easier fix though: we can just drop the macro altogether and
handle initialization of the struct in `reftable_stack_addition_init()`.
Callers are expected to call this function already, so this change even
simplifies the calling convention.

[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/

Suggested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index ed80710572..9db90cf4ed 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -664,8 +664,6 @@ struct reftable_addition {
 	uint64_t next_update_index;
 };
 
-#define REFTABLE_ADDITION_INIT {0}
-
 static void reftable_addition_close(struct reftable_addition *add)
 {
 	struct reftable_buf nm = REFTABLE_BUF_INIT;
@@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	struct reftable_buf lock_file_name = REFTABLE_BUF_INIT;
 	int err;
 
+	memset(add, 0, sizeof(*add));
 	add->stack = st;
 
 	err = flock_acquire(&add->tables_list_lock, st->list_file,
@@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,
 					    void *arg),
 			 void *arg)
 {
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
+	struct reftable_addition add;
+	int err;
+
+	err = reftable_stack_init_addition(&add, st, 0);
 	if (err < 0)
 		goto done;
 
@@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 				struct reftable_stack *st,
 				unsigned int flags)
 {
-	int err = 0;
-	struct reftable_addition empty = REFTABLE_ADDITION_INIT;
+	int err;
 
 	REFTABLE_CALLOC_ARRAY(*dest, 1);
 	if (!*dest)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 
-	**dest = empty;
 	err = reftable_stack_init_addition(*dest, st, flags);
 	if (err) {
 		reftable_free(*dest);
 		*dest = NULL;
 	}
+
 	return err;
 }
 

-- 
2.51.0.rc1.163.g2494970778.dirty

Re: [PATCH v3 4/8] reftable/stack: fix compiler warning due to missing braces

[Justin Tobler]

On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> While perfectly legal, older compiler toolchains complain when
> zero-initializing structs that contain nested structs with `{0}`:
> 
>     /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
>             struct reftable_addition empty = REFTABLE_ADDITION_INIT;
>                                              ^~~~~~~~~~~~~~~~~~~~~~
>     /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
>     #define REFTABLE_ADDITION_INIT {0}
>                                     ^
> 
> We had the discussion around whether or not we want to handle such bogus
> compiler errors in the past already [1]. Back then we basically decided
> that we do not care about such old-and-buggy compilers, so while we
> could fix the issue by using `{{0}}` instead this is not the preferred
> way to handle this in the Git codebase.
> 
> We have an easier fix though: we can just drop the macro altogether and
> handle initialization of the struct in `reftable_stack_addition_init()`.
> Callers are expected to call this function already, so this change even
> simplifies the calling convention.
> 
> [1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/
> 
> Suggested-by: Carlo Arenas <carenas@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index ed80710572..9db90cf4ed 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -664,8 +664,6 @@ struct reftable_addition {
>  	uint64_t next_update_index;
>  };
>  
> -#define REFTABLE_ADDITION_INIT {0}

It looks like there are only two places where this macro gets used.
Being that `reftable_stack_init_addition()` is always expected to be
called, deferring initialization of the structure to that point seems
sensible.

> -
>  static void reftable_addition_close(struct reftable_addition *add)
>  {
>  	struct reftable_buf nm = REFTABLE_BUF_INIT;
> @@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  	struct reftable_buf lock_file_name = REFTABLE_BUF_INIT;
>  	int err;
>  
> +	memset(add, 0, sizeof(*add));

Looks good.

>  	add->stack = st;
>  
>  	err = flock_acquire(&add->tables_list_lock, st->list_file,
> @@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,
>  					    void *arg),
>  			 void *arg)
>  {
> -	struct reftable_addition add = REFTABLE_ADDITION_INIT;
> -	int err = reftable_stack_init_addition(&add, st, 0);
> +	struct reftable_addition add;
> +	int err;
> +
> +	err = reftable_stack_init_addition(&add, st, 0);
>  	if (err < 0)
>  		goto done;
>  
> @@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
>  				struct reftable_stack *st,
>  				unsigned int flags)
>  {
> -	int err = 0;
> -	struct reftable_addition empty = REFTABLE_ADDITION_INIT;
> +	int err;
>  
>  	REFTABLE_CALLOC_ARRAY(*dest, 1);
>  	if (!*dest)
>  		return REFTABLE_OUT_OF_MEMORY_ERROR;
>  
> -	**dest = empty;

Now resetting the `reftable_addition` is handled by
`reftable_stack_init_addition` automatically, which is nicer IMO.

>  	err = reftable_stack_init_addition(*dest, st, flags);
>  	if (err) {
>  		reftable_free(*dest);
>  		*dest = NULL;
>  	}
> +
>  	return err;
>  }
>  
> 
> -- 
> 2.51.0.rc1.163.g2494970778.dirty
> 

[PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

The `reftable_stack_add()` function is a simple wrapper to lock the
stack, add records to it via a callback and then commit the
result. One problem with it though is that it doesn't accept any flags
for creating the addition. This makes it impossible to automatically
reload the stack in case it was modified before we managed to lock the
stack.

Add a `flags` field to plug this gap and pass it through accordingly.
For now this new flag won't be used by us, but it will be used by
libgit2.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c         |  8 +++----
 reftable/reftable-stack.h       |  9 +++++---
 reftable/stack.c                |  8 +++----
 t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4c3817f4ec1..3f0deab338c 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
 
 done:
 	return ret;
@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 910ec6ef3a2..d70fcb705dc 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);
  * transaction. Releases the lock if held. */
 void reftable_addition_destroy(struct reftable_addition *add);
 
-/* add a new table to the stack. The write_table function must call
- * reftable_writer_set_limits, add refs and return an error value. */
+/*
+ * Add a new table to the stack. The write_table function must call
+ * reftable_writer_set_limits, add refs and return an error value.
+ * The flags are passed through to `reftable_stack_new_addition()`.
+ */
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write_table)(struct reftable_writer *wr,
 					  void *write_arg),
-		       void *write_arg);
+		       void *write_arg, unsigned flags);
 
 struct reftable_iterator;
 
diff --git a/reftable/stack.c b/reftable/stack.c
index 9db90cf4ed0..1ce4d90cb82 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -736,12 +736,12 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 static int stack_try_add(struct reftable_stack *st,
 			 int (*write_table)(struct reftable_writer *wr,
 					    void *arg),
-			 void *arg)
+			 void *arg, unsigned flags)
 {
 	struct reftable_addition add;
 	int err;
 
-	err = reftable_stack_init_addition(&add, st, 0);
+	err = reftable_stack_init_addition(&add, st, flags);
 	if (err < 0)
 		goto done;
 
@@ -757,9 +757,9 @@ static int stack_try_add(struct reftable_stack *st,
 
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
+		       void *arg, unsigned flags)
 {
-	int err = stack_try_add(st, write, arg);
+	int err = stack_try_add(st, write, arg, flags);
 	if (err < 0) {
 		if (err == REFTABLE_OUTDATED_ERROR) {
 			/* Ignore error return, we want to propagate
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 2f49c975194..ce10247903c 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,
 		ref.refname = buf;
 		t_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 	}
 
@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	err = reftable_stack_read_ref(st, ref.refname, &dest);
@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)
 	err = reftable_new_stack(&st2, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st1, write_test_ref, &ref1);
+	err = reftable_stack_add(st1, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_OUTDATED_ERROR);
 
 	err = reftable_stack_reload(st2);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check(!err);
 	reftable_stack_destroy(st1);
 	reftable_stack_destroy(st2);
@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 1);
 	check_int(st->stats.attempts, ==, 0);
@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 2);
 	check_int(st->stats.attempts, ==, 1);
@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref1);
+	err = reftable_stack_add(st, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref2);
+	err = reftable_stack_add(st, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_stack_destroy(st);
 	clear_dir(dir);
@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 	for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {
-		err = reftable_stack_add(st, write_error, &i);
+		err = reftable_stack_add(st, write_error, &i, 0);
 		check_int(err, ==, i);
 	}
 
@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 
-		err = reftable_stack_add(st, write_test_log, &arg);
+		err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)
 	check(!err);
 
 	input.value.update.message = (char *) "one\ntwo";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	input.value.update.message = (char *) "one";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 
 	err = reftable_stack_read_log(st, input.refname, &dest);
@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)
 
 	input.value.update.message = (char *) "two\n";
 	arg.update_index = 2;
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 	err = reftable_stack_read_log(st, input.refname, &dest);
 	check(!err);
@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)
 		}
 	}
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	/* can't read it with the wrong hash ID. */
@@ -927,7 +927,7 @@ static void t_reflog_expire(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -978,7 +978,7 @@ static void t_empty_add(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_nothing, NULL);
+	err = reftable_stack_add(st, write_nothing, NULL, 0);
 	check(!err);
 
 	err = reftable_new_stack(&st2, dir, &opts);
@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)
 		};
 		snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		err = reftable_stack_auto_compact(st);
@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)
 		};
 		xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 
 		check(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));
@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)
 		snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);
 		ref.refname = buf;
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		/*

-- 
2.51.0.rc1.163.g2494970778.dirty

Re: [PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`

[Justin Tobler]

On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> The `reftable_stack_add()` function is a simple wrapper to lock the
> stack, add records to it via a callback and then commit the
> result. One problem with it though is that it doesn't accept any flags
> for creating the addition. This makes it impossible to automatically
> reload the stack in case it was modified before we managed to lock the
> stack.
> 
> Add a `flags` field to plug this gap and pass it through accordingly.
> For now this new flag won't be used by us, but it will be used by
> libgit2.

It looks like we will use the new `flags` field for
`reftable-stack-add()` later in the series though.

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH v3 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

On Tue, Aug 12, 2025 at 11:59:03AM -0500, Justin Tobler wrote:
> On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> > The `reftable_stack_add()` function is a simple wrapper to lock the
> > stack, add records to it via a callback and then commit the
> > result. One problem with it though is that it doesn't accept any flags
> > for creating the addition. This makes it impossible to automatically
> > reload the stack in case it was modified before we managed to lock the
> > stack.
> > 
> > Add a `flags` field to plug this gap and pass it through accordingly.
> > For now this new flag won't be used by us, but it will be used by
> > libgit2.
> 
> It looks like we will use the new `flags` field for
> `reftable-stack-add()` later in the series though.

Good point, this is definitely stale now. Will adjust.

Patrick

[PATCH v3 6/8] reftable/stack: handle outdated stacks when compacting

[Patrick Steinhardt]

When we compact the reftable stack we first acquire the lock for the
"tables.list" file and then reload the stack to check that it is still
up-to-date. This is done by calling `stack_uptodate()`, which knows to
return zero in case the stack is up-to-date, a positive value if it is
not and a negative error code on unexpected conditions.

We don't do proper error checking though, but instead we only check
whether the returned error code is non-zero. If so, we simply bubble it
up the calling stack, which means that callers may see an unexpected
positive value.

Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
Handle this situation in `reftable_addition_commit()`, where we perform
a best-effort auto-compaction.

All other callsites of `stack_uptodate()` know to handle a positive
return value and thus don't need to be fixed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 1ce4d90cb8..af0f94d882 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
 	return err;
 }
 
-/* -1 = error
- 0 = up to date
- 1 = changed. */
+/*
+ * Check whether the given stack is up-to-date with what we have in memory.
+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
+ * otherwise.
+ */
 static int stack_uptodate(struct reftable_stack *st)
 {
 	char **names = NULL;
@@ -850,10 +852,13 @@ int reftable_addition_commit(struct reftable_addition *add)
 		 * control. It is possible that a concurrent writer is already
 		 * trying to compact parts of the stack, which would lead to a
 		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
-		 * already. This is a benign error though, so we ignore it.
+		 * already. Similarly, the stack may have been rewritten by a
+		 * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
+		 * Both of these errors are benign, so we simply ignore them.
 		 */
 		err = reftable_stack_auto_compact(add->stack);
-		if (err < 0 && err != REFTABLE_LOCK_ERROR)
+		if (err < 0 && err != REFTABLE_LOCK_ERROR &&
+		    err != REFTABLE_OUTDATED_ERROR)
 			goto done;
 		err = 0;
 	}
@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
 		goto done;
 	}
 
+	/*
+	 * Check whether the stack is up-to-date. We unfortunately cannot
+	 * handle the situation gracefully in case it's _not_ up-to-date
+	 * because the range of tables that the user has requested us to
+	 * compact may have been changed. So instead we abort.
+	 *
+	 * We could in theory improve the situation by having the caller not
+	 * pass in a range, but instead the list of tables to compact. If so,
+	 * we could check that relevant tables still exist. But for now it's
+	 * good enough to just abort.
+	 */
 	err = stack_uptodate(st);
-	if (err)
+	if (err < 0)
 		goto done;
+	if (err > 0) {
+		err = REFTABLE_OUTDATED_ERROR;
+		goto done;
+	}
 
 	/*
 	 * Lock all tables in the user-provided range. This is the slice of our

-- 
2.51.0.rc1.163.g2494970778.dirty

[PATCH v3 7/8] reftable: don't second-guess errors from flock interface

[Patrick Steinhardt]

The `flock` interface is implemented as part of "reftable/system.c" and
thus needs to be implemented by the integrator between the reftable
library and its parent code base. As such, we cannot rely on any
specific implementation thereof.

Regardless of that, users of the `flock` subsystem rely on `errno` being
set to specific values. This is fragile and not documented anywhere and
doesn't really make for a good interface.

Refactor the code so that the implementations themselves are expected to
return reftable-specific error codes. Our implementation of the `flock`
subsystem already knows to do this for all error paths except one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c  | 37 ++++++++-----------------------------
 reftable/system.c |  2 +-
 reftable/system.h |  4 +++-
 3 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index af0f94d882..f91ce50bcd 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 
 	err = flock_acquire(&add->tables_list_lock, st->list_file,
 			    st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST) {
-			err = REFTABLE_LOCK_ERROR;
-		} else {
-			err = REFTABLE_IO_ERROR;
-		}
+	if (err < 0)
 		goto done;
-	}
+
 	if (st->opts.default_permissions) {
 		if (chmod(add->tables_list_lock.path,
 			  st->opts.default_permissions) < 0) {
@@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,
 	 * which are part of the user-specified range.
 	 */
 	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST)
-			err = REFTABLE_LOCK_ERROR;
-		else
-			err = REFTABLE_IO_ERROR;
+	if (err < 0)
 		goto done;
-	}
 
 	/*
 	 * Check whether the stack is up-to-date. We unfortunately cannot
@@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,
 			 * tables, otherwise there would be nothing to compact.
 			 * In that case, we return a lock error to our caller.
 			 */
-			if (errno == EEXIST && last - (i - 1) >= 2 &&
+			if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&
 			    flags & STACK_COMPACT_RANGE_BEST_EFFORT) {
 				err = 0;
 				/*
@@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,
 				 */
 				first = (i - 1) + 1;
 				break;
-			} else if (errno == EEXIST) {
-				err = REFTABLE_LOCK_ERROR;
-				goto done;
-			} else {
-				err = REFTABLE_IO_ERROR;
-				goto done;
 			}
+
+			goto done;
 		}
 
 		/*
@@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,
 		 * of tables.
 		 */
 		err = flock_close(&table_locks[nlocks++]);
-		if (err < 0) {
-			err = REFTABLE_IO_ERROR;
+		if (err < 0)
 			goto done;
-		}
 	}
 
 	/*
@@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,
 	 * the new table.
 	 */
 	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST)
-			err = REFTABLE_LOCK_ERROR;
-		else
-			err = REFTABLE_IO_ERROR;
+	if (err < 0)
 		goto done;
-	}
 
 	if (st->opts.default_permissions) {
 		if (chmod(tables_list_lock.path,
diff --git a/reftable/system.c b/reftable/system.c
index 1ee268b125..725a25844e 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,
 		reftable_free(lockfile);
 		if (errno == EEXIST)
 			return REFTABLE_LOCK_ERROR;
-		return -1;
+		return REFTABLE_IO_ERROR;
 	}
 
 	l->fd = get_lock_file_fd(lockfile);
diff --git a/reftable/system.h b/reftable/system.h
index beb9d2431f..c54ed4cad6 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -81,7 +81,9 @@ struct reftable_flock {
  * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative
  * we block indefinitely.
  *
- * Retrun 0 on success, a reftable error code on error.
+ * Retrun 0 on success, a reftable error code on error. Specifically,
+ * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already
+ * locked.
  */
 int flock_acquire(struct reftable_flock *l, const char *target_path,
 		  long timeout_ms);

-- 
2.51.0.rc1.163.g2494970778.dirty

Re: [PATCH v3 7/8] reftable: don't second-guess errors from flock interface

[Justin Tobler]

On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> The `flock` interface is implemented as part of "reftable/system.c" and
> thus needs to be implemented by the integrator between the reftable
> library and its parent code base. As such, we cannot rely on any
> specific implementation thereof.
> 
> Regardless of that, users of the `flock` subsystem rely on `errno` being
> set to specific values. This is fragile and not documented anywhere and
> doesn't really make for a good interface.
> 
> Refactor the code so that the implementations themselves are expected to
> return reftable-specific error codes. Our implementation of the `flock`
> subsystem already knows to do this for all error paths except one.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c  | 37 ++++++++-----------------------------
>  reftable/system.c |  2 +-
>  reftable/system.h |  4 +++-
>  3 files changed, 12 insertions(+), 31 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index af0f94d882..f91ce50bcd 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>  
>  	err = flock_acquire(&add->tables_list_lock, st->list_file,
>  			    st->opts.lock_timeout_ms);
> -	if (err < 0) {
> -		if (errno == EEXIST) {
> -			err = REFTABLE_LOCK_ERROR;
> -		} else {
> -			err = REFTABLE_IO_ERROR;
> -		}
> +	if (err < 0)
>  		goto done;
> -	}
> +
>  	if (st->opts.default_permissions) {
>  		if (chmod(add->tables_list_lock.path,
>  			  st->opts.default_permissions) < 0) {
> @@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,
>  	 * which are part of the user-specified range.
>  	 */
>  	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
> -	if (err < 0) {
> -		if (errno == EEXIST)
> -			err = REFTABLE_LOCK_ERROR;
> -		else
> -			err = REFTABLE_IO_ERROR;
> +	if (err < 0)
>  		goto done;
> -	}
>  
>  	/*
>  	 * Check whether the stack is up-to-date. We unfortunately cannot
> @@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,
>  			 * tables, otherwise there would be nothing to compact.
>  			 * In that case, we return a lock error to our caller.
>  			 */
> -			if (errno == EEXIST && last - (i - 1) >= 2 &&
> +			if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&
>  			    flags & STACK_COMPACT_RANGE_BEST_EFFORT) {
>  				err = 0;
>  				/*
> @@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,
>  				 */
>  				first = (i - 1) + 1;
>  				break;
> -			} else if (errno == EEXIST) {
> -				err = REFTABLE_LOCK_ERROR;
> -				goto done;
> -			} else {
> -				err = REFTABLE_IO_ERROR;
> -				goto done;
>  			}
> +
> +			goto done;
>  		}
>  
>  		/*
> @@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,
>  		 * of tables.
>  		 */
>  		err = flock_close(&table_locks[nlocks++]);
> -		if (err < 0) {
> -			err = REFTABLE_IO_ERROR;
> +		if (err < 0)
>  			goto done;
> -		}
>  	}
>  
>  	/*
> @@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,
>  	 * the new table.
>  	 */
>  	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
> -	if (err < 0) {
> -		if (errno == EEXIST)
> -			err = REFTABLE_LOCK_ERROR;
> -		else
> -			err = REFTABLE_IO_ERROR;
> +	if (err < 0)

Now we no longer rely on errno to determine the correct err to return.
Nice.

>  		goto done;
> -	}
>  
>  	if (st->opts.default_permissions) {
>  		if (chmod(tables_list_lock.path,
> diff --git a/reftable/system.c b/reftable/system.c
> index 1ee268b125..725a25844e 100644
> --- a/reftable/system.c
> +++ b/reftable/system.c
> @@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,
>  		reftable_free(lockfile);
>  		if (errno == EEXIST)
>  			return REFTABLE_LOCK_ERROR;
> -		return -1;
> +		return REFTABLE_IO_ERROR;
>  	}
>  
>  	l->fd = get_lock_file_fd(lockfile);
> diff --git a/reftable/system.h b/reftable/system.h
> index beb9d2431f..c54ed4cad6 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -81,7 +81,9 @@ struct reftable_flock {
>   * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative
>   * we block indefinitely.
>   *
> - * Retrun 0 on success, a reftable error code on error.
> + * Retrun 0 on success, a reftable error code on error. Specifically,

Not a new typo, but we could fix it:

s/Retrun/Return/

> + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already
> + * locked.
>   */
>  int flock_acquire(struct reftable_flock *l, const char *target_path,
>  		  long timeout_ms);
> 
> -- 
> 2.51.0.rc1.163.g2494970778.dirty
> 

[PATCH v3 8/8] refs/reftable: always reload stacks when creating lock

[Patrick Steinhardt]

When creating a new addition via either `reftable_stack_new_addition()`
or its convenince wrapper `reftable_stack_add()` we:

  1. Create the "tables.list.lock" file.

  2. Verify that the current version of the "tables.list" file is
     up-to-date.

  3. Write the new table records if so.

By default, the second step would cause us to bail out if we see that
there has been a concurrent write to the stack that made our in-memory
copy of the stack out-of-date. This is a safety mechanism to not write
records to the stack based on outdated information.

The downside though is that concurrent writes may now cause us to bail
out, which is not a good user experience. In addition, this isn't even
necessary for us, as Git knows to perform all checks for the old state
of references under the lock. (Well, in all except one case: when we
expire the reflog we first create the log iterator before we create the
lock, but this ordering is fixed as part of this commit.)

Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
flag. The effect of this flag is that we reload the stack after having
acquired the lock in case the stack is out-of-date. This plugs the race
with concurrent writers, but we continue performing the verifications of
the expected old state to catch actual conflicts in the references we
are about to write.

Adapt the remaining callsites that don't yet pass this flag to do so.
While at it, drop a needless manual reload.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 3f0deab338..66d25411f1 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
 	if (!arg) {
 		struct reftable_addition *addition;
 
-		ret = reftable_stack_reload(be->stack);
-		if (ret)
-			return ret;
-
 		ret = reftable_stack_new_addition(&addition, be->stack,
 						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 		if (ret) {
@@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	return ret;
@@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
@@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_stack_init_log_iterator(be->stack, &it);
+	ret = reftable_stack_new_addition(&add, be->stack,
+					  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_iterator_seek_log(&it, refname);
+	ret = reftable_stack_init_log_iterator(be->stack, &it);
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_stack_new_addition(&add, be->stack, 0);
+	ret = reftable_iterator_seek_log(&it, refname);
 	if (ret < 0)
 		goto done;
 

-- 
2.51.0.rc1.163.g2494970778.dirty

Re: [PATCH v3 8/8] refs/reftable: always reload stacks when creating lock

[Justin Tobler]

On 25/08/12 11:54AM, Patrick Steinhardt wrote:
> When creating a new addition via either `reftable_stack_new_addition()`
> or its convenince wrapper `reftable_stack_add()` we:
> 
>   1. Create the "tables.list.lock" file.
> 
>   2. Verify that the current version of the "tables.list" file is
>      up-to-date.
> 
>   3. Write the new table records if so.
> 
> By default, the second step would cause us to bail out if we see that
> there has been a concurrent write to the stack that made our in-memory
> copy of the stack out-of-date. This is a safety mechanism to not write
> records to the stack based on outdated information.
> 
> The downside though is that concurrent writes may now cause us to bail
> out, which is not a good user experience. In addition, this isn't even
> necessary for us, as Git knows to perform all checks for the old state
> of references under the lock. (Well, in all except one case: when we
> expire the reflog we first create the log iterator before we create the
> lock, but this ordering is fixed as part of this commit.)
> 
> Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
> flag. The effect of this flag is that we reload the stack after having
> acquired the lock in case the stack is out-of-date. This plugs the race
> with concurrent writers, but we continue performing the verifications of
> the expected old state to catch actual conflicts in the references we
> are about to write.
> 
> Adapt the remaining callsites that don't yet pass this flag to do so.
> While at it, drop a needless manual reload.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 3f0deab338..66d25411f1 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
>  	if (!arg) {
>  		struct reftable_addition *addition;
>  
> -		ret = reftable_stack_reload(be->stack);
> -		if (ret)
> -			return ret;

Here we don't need to reload because `reftable_stack_new_addition()`
immediately following already does this for us.

> -
>  		ret = reftable_stack_new_addition(&addition, be->stack,
>  						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  		if (ret) {
> @@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
>  	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
>  	if (ret)
>  		goto done;
> -	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> +	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	assert(ret != REFTABLE_API_ERROR);
> @@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
>  	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
>  	if (ret)
>  		goto done;
> -	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> +	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	assert(ret != REFTABLE_API_ERROR);
> @@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
>  		goto done;
>  	arg.stack = be->stack;
>  
> -	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
> +	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	return ret;
> @@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
>  		return ret;
>  	arg.stack = be->stack;
>  
> -	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
> +	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  	assert(ret != REFTABLE_API_ERROR);
>  	return ret;
> @@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_stack_init_log_iterator(be->stack, &it);
> +	ret = reftable_stack_new_addition(&add, be->stack,
> +					  REFTABLE_STACK_NEW_ADDITION_RELOAD);

Here we change the order so that we now acquire the lock first.

This patch looks good to me :)

-Justin

>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_iterator_seek_log(&it, refname);
> +	ret = reftable_stack_init_log_iterator(be->stack, &it);
>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_stack_new_addition(&add, be->stack, 0);
> +	ret = reftable_iterator_seek_log(&it, refname);
>  	if (ret < 0)
>  		goto done;
>  
> 
> -- 
> 2.51.0.rc1.163.g2494970778.dirty
> 

Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2

[Carlo Arenas]

On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt <ps@pks.im> wrote:

> Range-diff versus v2:
>
> 1:  ce08ba1217 = 1:  1613715dc9 reftable/writer: fix type used for number of records
> 2:  80a87ff19a = 2:  4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro
> 3:  e230c97347 < -:  ---------- reftable/stack: fix compiler warning due to missing braces
> 4:  dd413b76a2 ! 3:  3977a1f497 reftable/stack: reorder code to avoid forward declarations
>     @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)
>         struct reftable_stack *stack;
>      @@ reftable/stack.c: struct reftable_addition {
>
>     - #define REFTABLE_ADDITION_INIT {{0}}
>     + #define REFTABLE_ADDITION_INIT {0}

This define shouldn't be needed anymore AFAIK

Carlo

Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2

[Patrick Steinhardt]

On Tue, Aug 12, 2025 at 12:00:53PM -0700, Carlo Arenas wrote:
> On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > Range-diff versus v2:
> >
> > 1:  ce08ba1217 = 1:  1613715dc9 reftable/writer: fix type used for number of records
> > 2:  80a87ff19a = 2:  4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro
> > 3:  e230c97347 < -:  ---------- reftable/stack: fix compiler warning due to missing braces
> > 4:  dd413b76a2 ! 3:  3977a1f497 reftable/stack: reorder code to avoid forward declarations
> >     @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)
> >         struct reftable_stack *stack;
> >      @@ reftable/stack.c: struct reftable_addition {
> >
> >     - #define REFTABLE_ADDITION_INIT {{0}}
> >     + #define REFTABLE_ADDITION_INIT {0}
> 
> This define shouldn't be needed anymore AFAIK

It doesn't exist anymore after this patch series, as it gets removed in
the fourth patch. The above change in the range-diff is merely a result
of me swapping the order of patch 3 and 4.

Patrick

Re: [PATCH v3 0/8] reftable: a couple of improvements for libgit2

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Aug 12, 2025 at 12:00:53PM -0700, Carlo Arenas wrote:
>> On Tue, Aug 12, 2025 at 2:54 AM Patrick Steinhardt <ps@pks.im> wrote:
>> 
>> > Range-diff versus v2:
>> >
>> > 1:  ce08ba1217 = 1:  1613715dc9 reftable/writer: fix type used for number of records
>> > 2:  80a87ff19a = 2:  4a082b71fb reftable/writer: drop Git-specific `QSORT()` macro
>> > 3:  e230c97347 < -:  ---------- reftable/stack: fix compiler warning due to missing braces
>> > 4:  dd413b76a2 ! 3:  3977a1f497 reftable/stack: reorder code to avoid forward declarations
>> >     @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)
>> >         struct reftable_stack *stack;
>> >      @@ reftable/stack.c: struct reftable_addition {
>> >
>> >     - #define REFTABLE_ADDITION_INIT {{0}}
>> >     + #define REFTABLE_ADDITION_INIT {0}
>> 
>> This define shouldn't be needed anymore AFAIK
>
> It doesn't exist anymore after this patch series, as it gets removed in
> the fourth patch. The above change in the range-diff is merely a result
> of me swapping the order of patch 3 and 4.

Interesting.

What is being shown here is that the reordering patch in the
previous round had {{0}} here (due to the old "fix compiler" step
making that change) while the new iteration has {0} instead.  These
are both context lines that we are seeing here.  As the "fix
compiler warning" step finds no counterpart, old #3 is not shown
above this part, and new #4 that removes this #define does not
appear in the output, either.

A diff-of-diff is a bit confusing to read when something like this
happens.

[PATCH v4 0/8] reftable: a couple of improvements for libgit2

[Patrick Steinhardt]

Hi,

this small patch series contains a couple of improvements I required for
libgit2. With those changes libgit2 is now able to run its full test
suite with reftable-enabled repositories. I still need to invest a bit
of work to make it memory-leak free and compile on Windows, but overall
I think that support for reftables is almost ready.

Changes in v2:
  - Another commit that fixes handling of outdated stacks when doing
    compaction. This issue is hit in libgit2, which has a test that
    performs writes with a couple dozen concurrent threads.
  - Add a link to past discussions around `{0}` vs `{{0}}` as provided
    by Eric.
  - Link to v1: https://lore.kernel.org/r/20250801-pks-reftable-fixes-for-libgit2-v1-0-f446e1c33cb9@pks.im

Changes in v3:
  - Adapt `reftable_stack_init_addition()` to `memset()` the addition
    instead of using `{{0}}`.
  - Add another commit on top that makes the `flock` interface more
    robust by not relying on `errno`.
  - Add another commit to fix races with concurrent writers caused by
    the backend not passing `REFTABLE_STACK_NEW_ADDITION_RELOAD`.
  - Link to v2: https://lore.kernel.org/r/20250804-pks-reftable-fixes-for-libgit2-v2-0-fef06209a984@pks.im

Changes in v4:
  - Adjust stale commit message.
  - Fix a typo while at it.
  - Link to v3: https://lore.kernel.org/r/20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (8):
      reftable/writer: fix type used for number of records
      reftable/writer: drop Git-specific `QSORT()` macro
      reftable/stack: reorder code to avoid forward declarations
      reftable/stack: fix compiler warning due to missing braces
      reftable/stack: allow passing flags to `reftable_stack_add()`
      reftable/stack: handle outdated stacks when compacting
      reftable: don't second-guess errors from flock interface
      refs/reftable: always reload stacks when creating lock

 refs/reftable-backend.c         |  23 ++-
 reftable/reftable-stack.h       |   9 +-
 reftable/reftable-writer.h      |   4 +-
 reftable/stack.c                | 439 +++++++++++++++++++---------------------
 reftable/system.c               |   2 +-
 reftable/system.h               |   4 +-
 reftable/writer.c               |  23 ++-
 t/unit-tests/t-reftable-stack.c |  50 ++---
 8 files changed, 275 insertions(+), 279 deletions(-)

Range-diff versus v3:

1:  afb757e6c1 = 1:  631293a69e reftable/writer: fix type used for number of records
2:  75dd5f6cb6 = 2:  a583b9f51c reftable/writer: drop Git-specific `QSORT()` macro
3:  6badd0ae26 = 3:  7153526268 reftable/stack: reorder code to avoid forward declarations
4:  9f5aa7f808 = 4:  7372cbe037 reftable/stack: fix compiler warning due to missing braces
5:  d4fea00eb2 ! 5:  57810b984e reftable/stack: allow passing flags to `reftable_stack_add()`
    @@ Commit message
         stack.
     
         Add a `flags` field to plug this gap and pass it through accordingly.
    -    For now this new flag won't be used by us, but it will be used by
    -    libgit2.
    +    This flag will be used in a subsequent patch and by libgit2.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
6:  d2cdd89280 = 6:  211d3f646e reftable/stack: handle outdated stacks when compacting
7:  635dd20dcd ! 7:  f8d541af00 reftable: don't second-guess errors from flock interface
    @@ reftable/system.h: struct reftable_flock {
       * we block indefinitely.
       *
     - * Retrun 0 on success, a reftable error code on error.
    -+ * Retrun 0 on success, a reftable error code on error. Specifically,
    ++ * Return 0 on success, a reftable error code on error. Specifically,
     + * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already
     + * locked.
       */
8:  4396793747 = 8:  912d00a4a8 refs/reftable: always reload stacks when creating lock

---
base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c
change-id: 20250801-pks-reftable-fixes-for-libgit2-562b959a5603

[PATCH v4 1/8] reftable/writer: fix type used for number of records

[Patrick Steinhardt]

Both `reftable_writer_add_refs()` and `reftable_writer_add_logs()`
accept an array of records that should be added to the new table.
Callers of this function are expected to also pass the number of such
records to the function to tell it how many such records it is supposed
to write.

But while all callers pass in a `size_t`, which is a sensible choice,
the function in fact accepts an `int` as argument, which is less so. Fix
this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-writer.h |  4 ++--
 reftable/writer.c          | 17 +++++++++--------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 0fbeff17f4..1e7003cd69 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -156,7 +156,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
   the records before adding them, reordering the records array passed in.
 */
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n);
+			     struct reftable_ref_record *refs, size_t n);
 
 /*
   adds reftable_log_records. Log records are keyed by (refname, decreasing
@@ -171,7 +171,7 @@ int reftable_writer_add_log(struct reftable_writer *w,
   the records before adding them, reordering records array passed in.
 */
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n);
+			     struct reftable_log_record *logs, size_t n);
 
 /* reftable_writer_close finalizes the reftable. The writer is retained so
  * statistics can be inspected. */
diff --git a/reftable/writer.c b/reftable/writer.c
index 3b4ebdd6dc..5bad130c7e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -395,14 +395,15 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 }
 
 int reftable_writer_add_refs(struct reftable_writer *w,
-			     struct reftable_ref_record *refs, int n)
+			     struct reftable_ref_record *refs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(refs, n, reftable_ref_record_compare_name);
-	for (i = 0; err == 0 && i < n; i++) {
+
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
-	}
+
 	return err;
 }
 
@@ -486,15 +487,15 @@ int reftable_writer_add_log(struct reftable_writer *w,
 }
 
 int reftable_writer_add_logs(struct reftable_writer *w,
-			     struct reftable_log_record *logs, int n)
+			     struct reftable_log_record *logs, size_t n)
 {
 	int err = 0;
-	int i = 0;
+
 	QSORT(logs, n, reftable_log_record_compare_key);
 
-	for (i = 0; err == 0 && i < n; i++) {
+	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);
-	}
+
 	return err;
 }
 

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 2/8] reftable/writer: drop Git-specific `QSORT()` macro

[Patrick Steinhardt]

The reftable writer accidentally uses the Git-specific `QSORT()` macro.
This macro removes the need for the caller to provide the element size,
but other than that it's mostly equivalent to `qsort()`.

Replace the macro accordingly to make the library usable outside of Git.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/writer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index 5bad130c7e..0133b64975 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -399,7 +399,8 @@ int reftable_writer_add_refs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(refs, n, reftable_ref_record_compare_name);
+	if (n)
+		qsort(refs, n, sizeof(*refs), reftable_ref_record_compare_name);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_ref(w, &refs[i]);
@@ -491,7 +492,8 @@ int reftable_writer_add_logs(struct reftable_writer *w,
 {
 	int err = 0;
 
-	QSORT(logs, n, reftable_log_record_compare_key);
+	if (n)
+		qsort(logs, n, sizeof(*logs), reftable_log_record_compare_key);
 
 	for (size_t i = 0; err == 0 && i < n; i++)
 		err = reftable_writer_add_log(w, &logs[i]);

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 3/8] reftable/stack: reorder code to avoid forward declarations

[Patrick Steinhardt]

We have a couple of forward declarations in the stack-related code of
the reftable library. These declarations aren't really required, but are
simply caused by unfortunate ordering.

Reorder the code and remove the forward declarations.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 364 +++++++++++++++++++++++++++----------------------------
 1 file changed, 176 insertions(+), 188 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 4caf96aa1d..ed80710572 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,18 +17,6 @@
 #include "table.h"
 #include "writer.h"
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg);
-static int stack_write_compact(struct reftable_stack *st,
-			       struct reftable_writer *wr,
-			       size_t first, size_t last,
-			       struct reftable_log_expiry_config *config);
-static void reftable_addition_close(struct reftable_addition *add);
-static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
-					     int reuse_open);
-
 static int stack_filename(struct reftable_buf *dest, struct reftable_stack *st,
 			  const char *name)
 {
@@ -84,54 +72,6 @@ static int fd_writer_flush(void *arg)
 	return stack_fsync(writer->opts, writer->fd);
 }
 
-int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       const struct reftable_write_options *_opts)
-{
-	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
-	struct reftable_write_options opts = { 0 };
-	struct reftable_stack *p;
-	int err;
-
-	p = reftable_calloc(1, sizeof(*p));
-	if (!p) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	if (_opts)
-		opts = *_opts;
-	if (opts.hash_id == 0)
-		opts.hash_id = REFTABLE_HASH_SHA1;
-
-	*dest = NULL;
-
-	reftable_buf_reset(&list_file_name);
-	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
-	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
-		goto out;
-
-	p->list_file = reftable_buf_detach(&list_file_name);
-	p->list_fd = -1;
-	p->opts = opts;
-	p->reftable_dir = reftable_strdup(dir);
-	if (!p->reftable_dir) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto out;
-	}
-
-	err = reftable_stack_reload_maybe_reuse(p, 1);
-	if (err < 0)
-		goto out;
-
-	*dest = p;
-	err = 0;
-
-out:
-	if (err < 0)
-		reftable_stack_destroy(p);
-	return err;
-}
-
 static int fd_read_lines(int fd, char ***namesp)
 {
 	char *buf = NULL;
@@ -591,6 +531,54 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
 	return err;
 }
 
+int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+		       const struct reftable_write_options *_opts)
+{
+	struct reftable_buf list_file_name = REFTABLE_BUF_INIT;
+	struct reftable_write_options opts = { 0 };
+	struct reftable_stack *p;
+	int err;
+
+	p = reftable_calloc(1, sizeof(*p));
+	if (!p) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	if (_opts)
+		opts = *_opts;
+	if (opts.hash_id == 0)
+		opts.hash_id = REFTABLE_HASH_SHA1;
+
+	*dest = NULL;
+
+	reftable_buf_reset(&list_file_name);
+	if ((err = reftable_buf_addstr(&list_file_name, dir)) < 0 ||
+	    (err = reftable_buf_addstr(&list_file_name, "/tables.list")) < 0)
+		goto out;
+
+	p->list_file = reftable_buf_detach(&list_file_name);
+	p->list_fd = -1;
+	p->opts = opts;
+	p->reftable_dir = reftable_strdup(dir);
+	if (!p->reftable_dir) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto out;
+	}
+
+	err = reftable_stack_reload_maybe_reuse(p, 1);
+	if (err < 0)
+		goto out;
+
+	*dest = p;
+	err = 0;
+
+out:
+	if (err < 0)
+		reftable_stack_destroy(p);
+	return err;
+}
+
 /* -1 = error
  0 = up to date
  1 = changed. */
@@ -667,34 +655,6 @@ int reftable_stack_reload(struct reftable_stack *st)
 	return err;
 }
 
-int reftable_stack_add(struct reftable_stack *st,
-		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
-{
-	int err = stack_try_add(st, write, arg);
-	if (err < 0) {
-		if (err == REFTABLE_OUTDATED_ERROR) {
-			/* Ignore error return, we want to propagate
-			   REFTABLE_OUTDATED_ERROR.
-			*/
-			reftable_stack_reload(st);
-		}
-		return err;
-	}
-
-	return 0;
-}
-
-static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
-{
-	char buf[100];
-	uint32_t rnd = reftable_rand();
-	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
-		 min, max, rnd);
-	reftable_buf_reset(dest);
-	return reftable_buf_addstr(dest, buf);
-}
-
 struct reftable_addition {
 	struct reftable_flock tables_list_lock;
 	struct reftable_stack *stack;
@@ -706,6 +666,26 @@ struct reftable_addition {
 
 #define REFTABLE_ADDITION_INIT {0}
 
+static void reftable_addition_close(struct reftable_addition *add)
+{
+	struct reftable_buf nm = REFTABLE_BUF_INIT;
+	size_t i;
+
+	for (i = 0; i < add->new_tables_len; i++) {
+		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
+			unlink(nm.buf);
+		reftable_free(add->new_tables[i]);
+		add->new_tables[i] = NULL;
+	}
+	reftable_free(add->new_tables);
+	add->new_tables = NULL;
+	add->new_tables_len = 0;
+	add->new_tables_cap = 0;
+
+	flock_release(&add->tables_list_lock);
+	reftable_buf_release(&nm);
+}
+
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
 					unsigned int flags)
@@ -754,24 +734,52 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	return err;
 }
 
-static void reftable_addition_close(struct reftable_addition *add)
+static int stack_try_add(struct reftable_stack *st,
+			 int (*write_table)(struct reftable_writer *wr,
+					    void *arg),
+			 void *arg)
 {
-	struct reftable_buf nm = REFTABLE_BUF_INIT;
-	size_t i;
+	struct reftable_addition add = REFTABLE_ADDITION_INIT;
+	int err = reftable_stack_init_addition(&add, st, 0);
+	if (err < 0)
+		goto done;
 
-	for (i = 0; i < add->new_tables_len; i++) {
-		if (!stack_filename(&nm, add->stack, add->new_tables[i]))
-			unlink(nm.buf);
-		reftable_free(add->new_tables[i]);
-		add->new_tables[i] = NULL;
+	err = reftable_addition_add(&add, write_table, arg);
+	if (err < 0)
+		goto done;
+
+	err = reftable_addition_commit(&add);
+done:
+	reftable_addition_close(&add);
+	return err;
+}
+
+int reftable_stack_add(struct reftable_stack *st,
+		       int (*write)(struct reftable_writer *wr, void *arg),
+		       void *arg)
+{
+	int err = stack_try_add(st, write, arg);
+	if (err < 0) {
+		if (err == REFTABLE_OUTDATED_ERROR) {
+			/* Ignore error return, we want to propagate
+			   REFTABLE_OUTDATED_ERROR.
+			*/
+			reftable_stack_reload(st);
+		}
+		return err;
 	}
-	reftable_free(add->new_tables);
-	add->new_tables = NULL;
-	add->new_tables_len = 0;
-	add->new_tables_cap = 0;
 
-	flock_release(&add->tables_list_lock);
-	reftable_buf_release(&nm);
+	return 0;
+}
+
+static int format_name(struct reftable_buf *dest, uint64_t min, uint64_t max)
+{
+	char buf[100];
+	uint32_t rnd = reftable_rand();
+	snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
+		 min, max, rnd);
+	reftable_buf_reset(dest);
+	return reftable_buf_addstr(dest, buf);
 }
 
 void reftable_addition_destroy(struct reftable_addition *add)
@@ -874,26 +882,6 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 	return err;
 }
 
-static int stack_try_add(struct reftable_stack *st,
-			 int (*write_table)(struct reftable_writer *wr,
-					    void *arg),
-			 void *arg)
-{
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_add(&add, write_table, arg);
-	if (err < 0)
-		goto done;
-
-	err = reftable_addition_commit(&add);
-done:
-	reftable_addition_close(&add);
-	return err;
-}
-
 int reftable_addition_add(struct reftable_addition *add,
 			  int (*write_table)(struct reftable_writer *wr,
 					     void *arg),
@@ -1007,72 +995,6 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
 	return 1;
 }
 
-static int stack_compact_locked(struct reftable_stack *st,
-				size_t first, size_t last,
-				struct reftable_log_expiry_config *config,
-				struct reftable_tmpfile *tab_file_out)
-{
-	struct reftable_buf next_name = REFTABLE_BUF_INIT;
-	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
-	struct reftable_writer *wr = NULL;
-	struct fd_writer writer=  {
-		.opts = &st->opts,
-	};
-	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
-	int err = 0;
-
-	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
-			  reftable_table_max_update_index(st->tables[last]));
-	if (err < 0)
-		goto done;
-
-	err = stack_filename(&tab_file_path, st, next_name.buf);
-	if (err < 0)
-		goto done;
-
-	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
-	if (err < 0)
-		goto done;
-
-	if (st->opts.default_permissions &&
-	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
-		err = REFTABLE_IO_ERROR;
-		goto done;
-	}
-
-	writer.fd = tab_file.fd;
-	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
-				  &writer, &st->opts);
-	if (err < 0)
-		goto done;
-
-	err = stack_write_compact(st, wr, first, last, config);
-	if (err < 0)
-		goto done;
-
-	err = reftable_writer_close(wr);
-	if (err < 0)
-		goto done;
-
-	err = tmpfile_close(&tab_file);
-	if (err < 0)
-		goto done;
-
-	*tab_file_out = tab_file;
-	tab_file = REFTABLE_TMPFILE_INIT;
-
-done:
-	tmpfile_delete(&tab_file);
-	reftable_writer_free(wr);
-	reftable_buf_release(&next_name);
-	reftable_buf_release(&tab_file_path);
-	return err;
-}
-
 static int stack_write_compact(struct reftable_stack *st,
 			       struct reftable_writer *wr,
 			       size_t first, size_t last,
@@ -1172,6 +1094,72 @@ static int stack_write_compact(struct reftable_stack *st,
 	return err;
 }
 
+static int stack_compact_locked(struct reftable_stack *st,
+				size_t first, size_t last,
+				struct reftable_log_expiry_config *config,
+				struct reftable_tmpfile *tab_file_out)
+{
+	struct reftable_buf next_name = REFTABLE_BUF_INIT;
+	struct reftable_buf tab_file_path = REFTABLE_BUF_INIT;
+	struct reftable_writer *wr = NULL;
+	struct fd_writer writer=  {
+		.opts = &st->opts,
+	};
+	struct reftable_tmpfile tab_file = REFTABLE_TMPFILE_INIT;
+	int err = 0;
+
+	err = format_name(&next_name, reftable_table_min_update_index(st->tables[first]),
+			  reftable_table_max_update_index(st->tables[last]));
+	if (err < 0)
+		goto done;
+
+	err = stack_filename(&tab_file_path, st, next_name.buf);
+	if (err < 0)
+		goto done;
+
+	err = reftable_buf_addstr(&tab_file_path, ".temp.XXXXXX");
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_from_pattern(&tab_file, tab_file_path.buf);
+	if (err < 0)
+		goto done;
+
+	if (st->opts.default_permissions &&
+	    chmod(tab_file.path, st->opts.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
+	writer.fd = tab_file.fd;
+	err = reftable_writer_new(&wr, fd_writer_write, fd_writer_flush,
+				  &writer, &st->opts);
+	if (err < 0)
+		goto done;
+
+	err = stack_write_compact(st, wr, first, last, config);
+	if (err < 0)
+		goto done;
+
+	err = reftable_writer_close(wr);
+	if (err < 0)
+		goto done;
+
+	err = tmpfile_close(&tab_file);
+	if (err < 0)
+		goto done;
+
+	*tab_file_out = tab_file;
+	tab_file = REFTABLE_TMPFILE_INIT;
+
+done:
+	tmpfile_delete(&tab_file);
+	reftable_writer_free(wr);
+	reftable_buf_release(&next_name);
+	reftable_buf_release(&tab_file_path);
+	return err;
+}
+
 enum stack_compact_range_flags {
 	/*
 	 * Perform a best-effort compaction. That is, even if we cannot lock

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 4/8] reftable/stack: fix compiler warning due to missing braces

[Patrick Steinhardt]

While perfectly legal, older compiler toolchains complain when
zero-initializing structs that contain nested structs with `{0}`:

    /home/libgit2/source/deps/reftable/stack.c:862:35: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
            struct reftable_addition empty = REFTABLE_ADDITION_INIT;
                                             ^~~~~~~~~~~~~~~~~~~~~~
    /home/libgit2/source/deps/reftable/stack.c:707:33: note: expanded from macro 'REFTABLE_ADDITION_INIT'
    #define REFTABLE_ADDITION_INIT {0}
                                    ^

We had the discussion around whether or not we want to handle such bogus
compiler errors in the past already [1]. Back then we basically decided
that we do not care about such old-and-buggy compilers, so while we
could fix the issue by using `{{0}}` instead this is not the preferred
way to handle this in the Git codebase.

We have an easier fix though: we can just drop the macro altogether and
handle initialization of the struct in `reftable_stack_addition_init()`.
Callers are expected to call this function already, so this change even
simplifies the calling convention.

[1]: https://lore.kernel.org/git/20220710081135.74964-1-sunshine@sunshineco.com/T/

Suggested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index ed80710572..9db90cf4ed 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -664,8 +664,6 @@ struct reftable_addition {
 	uint64_t next_update_index;
 };
 
-#define REFTABLE_ADDITION_INIT {0}
-
 static void reftable_addition_close(struct reftable_addition *add)
 {
 	struct reftable_buf nm = REFTABLE_BUF_INIT;
@@ -693,6 +691,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	struct reftable_buf lock_file_name = REFTABLE_BUF_INIT;
 	int err;
 
+	memset(add, 0, sizeof(*add));
 	add->stack = st;
 
 	err = flock_acquire(&add->tables_list_lock, st->list_file,
@@ -739,8 +738,10 @@ static int stack_try_add(struct reftable_stack *st,
 					    void *arg),
 			 void *arg)
 {
-	struct reftable_addition add = REFTABLE_ADDITION_INIT;
-	int err = reftable_stack_init_addition(&add, st, 0);
+	struct reftable_addition add;
+	int err;
+
+	err = reftable_stack_init_addition(&add, st, 0);
 	if (err < 0)
 		goto done;
 
@@ -866,19 +867,18 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 				struct reftable_stack *st,
 				unsigned int flags)
 {
-	int err = 0;
-	struct reftable_addition empty = REFTABLE_ADDITION_INIT;
+	int err;
 
 	REFTABLE_CALLOC_ARRAY(*dest, 1);
 	if (!*dest)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 
-	**dest = empty;
 	err = reftable_stack_init_addition(*dest, st, flags);
 	if (err) {
 		reftable_free(*dest);
 		*dest = NULL;
 	}
+
 	return err;
 }
 

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 5/8] reftable/stack: allow passing flags to `reftable_stack_add()`

[Patrick Steinhardt]

The `reftable_stack_add()` function is a simple wrapper to lock the
stack, add records to it via a callback and then commit the
result. One problem with it though is that it doesn't accept any flags
for creating the addition. This makes it impossible to automatically
reload the stack in case it was modified before we managed to lock the
stack.

Add a `flags` field to plug this gap and pass it through accordingly.
This flag will be used in a subsequent patch and by libgit2.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c         |  8 +++----
 reftable/reftable-stack.h       |  9 +++++---
 reftable/stack.c                |  8 +++----
 t/unit-tests/t-reftable-stack.c | 50 ++++++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4c3817f4ec1..3f0deab338c 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1960,7 +1960,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1989,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2360,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
 
 done:
 	return ret;
@@ -2431,7 +2431,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 910ec6ef3a2..d70fcb705dc 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -68,12 +68,15 @@ int reftable_addition_commit(struct reftable_addition *add);
  * transaction. Releases the lock if held. */
 void reftable_addition_destroy(struct reftable_addition *add);
 
-/* add a new table to the stack. The write_table function must call
- * reftable_writer_set_limits, add refs and return an error value. */
+/*
+ * Add a new table to the stack. The write_table function must call
+ * reftable_writer_set_limits, add refs and return an error value.
+ * The flags are passed through to `reftable_stack_new_addition()`.
+ */
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write_table)(struct reftable_writer *wr,
 					  void *write_arg),
-		       void *write_arg);
+		       void *write_arg, unsigned flags);
 
 struct reftable_iterator;
 
diff --git a/reftable/stack.c b/reftable/stack.c
index 9db90cf4ed0..1ce4d90cb82 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -736,12 +736,12 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 static int stack_try_add(struct reftable_stack *st,
 			 int (*write_table)(struct reftable_writer *wr,
 					    void *arg),
-			 void *arg)
+			 void *arg, unsigned flags)
 {
 	struct reftable_addition add;
 	int err;
 
-	err = reftable_stack_init_addition(&add, st, 0);
+	err = reftable_stack_init_addition(&add, st, flags);
 	if (err < 0)
 		goto done;
 
@@ -757,9 +757,9 @@ static int stack_try_add(struct reftable_stack *st,
 
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write)(struct reftable_writer *wr, void *arg),
-		       void *arg)
+		       void *arg, unsigned flags)
 {
-	int err = stack_try_add(st, write, arg);
+	int err = stack_try_add(st, write, arg, flags);
 	if (err < 0) {
 		if (err == REFTABLE_OUTDATED_ERROR) {
 			/* Ignore error return, we want to propagate
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 2f49c975194..ce10247903c 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -128,7 +128,7 @@ static void write_n_ref_tables(struct reftable_stack *st,
 		ref.refname = buf;
 		t_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 	}
 
@@ -170,7 +170,7 @@ static void t_reftable_stack_add_one(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	err = reftable_stack_read_ref(st, ref.refname, &dest);
@@ -235,16 +235,16 @@ static void t_reftable_stack_uptodate(void)
 	err = reftable_new_stack(&st2, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st1, write_test_ref, &ref1);
+	err = reftable_stack_add(st1, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_OUTDATED_ERROR);
 
 	err = reftable_stack_reload(st2);
 	check(!err);
 
-	err = reftable_stack_add(st2, write_test_ref, &ref2);
+	err = reftable_stack_add(st2, write_test_ref, &ref2, 0);
 	check(!err);
 	reftable_stack_destroy(st1);
 	reftable_stack_destroy(st2);
@@ -428,7 +428,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 1);
 	check_int(st->stats.attempts, ==, 0);
@@ -446,7 +446,7 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 	check_int(st->merged->tables_len, ==, 2);
 	check_int(st->stats.attempts, ==, 1);
@@ -484,10 +484,10 @@ static void t_reftable_stack_update_index_check(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref1);
+	err = reftable_stack_add(st, write_test_ref, &ref1, 0);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref2);
+	err = reftable_stack_add(st, write_test_ref, &ref2, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 	reftable_stack_destroy(st);
 	clear_dir(dir);
@@ -503,7 +503,7 @@ static void t_reftable_stack_lock_failure(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 	for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {
-		err = reftable_stack_add(st, write_error, &i);
+		err = reftable_stack_add(st, write_error, &i, 0);
 		check_int(err, ==, i);
 	}
 
@@ -546,7 +546,7 @@ static void t_reftable_stack_add(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -555,7 +555,7 @@ static void t_reftable_stack_add(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -639,7 +639,7 @@ static void t_reftable_stack_iterator(void)
 	}
 
 	for (i = 0; i < N; i++) {
-		err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -649,7 +649,7 @@ static void t_reftable_stack_iterator(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 
-		err = reftable_stack_add(st, write_test_log, &arg);
+		err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -725,11 +725,11 @@ static void t_reftable_stack_log_normalize(void)
 	check(!err);
 
 	input.value.update.message = (char *) "one\ntwo";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check_int(err, ==, REFTABLE_API_ERROR);
 
 	input.value.update.message = (char *) "one";
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 
 	err = reftable_stack_read_log(st, input.refname, &dest);
@@ -738,7 +738,7 @@ static void t_reftable_stack_log_normalize(void)
 
 	input.value.update.message = (char *) "two\n";
 	arg.update_index = 2;
-	err = reftable_stack_add(st, write_test_log, &arg);
+	err = reftable_stack_add(st, write_test_log, &arg, 0);
 	check(!err);
 	err = reftable_stack_read_log(st, input.refname, &dest);
 	check(!err);
@@ -792,7 +792,7 @@ static void t_reftable_stack_tombstone(void)
 		}
 	}
 	for (i = 0; i < N; i++) {
-		int err = reftable_stack_add(st, write_test_ref, &refs[i]);
+		int err = reftable_stack_add(st, write_test_ref, &refs[i], 0);
 		check(!err);
 	}
 
@@ -801,7 +801,7 @@ static void t_reftable_stack_tombstone(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -855,7 +855,7 @@ static void t_reftable_stack_hash_id(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref);
+	err = reftable_stack_add(st, write_test_ref, &ref, 0);
 	check(!err);
 
 	/* can't read it with the wrong hash ID. */
@@ -927,7 +927,7 @@ static void t_reflog_expire(void)
 			.log = &logs[i],
 			.update_index = reftable_stack_next_update_index(st),
 		};
-		int err = reftable_stack_add(st, write_test_log, &arg);
+		int err = reftable_stack_add(st, write_test_log, &arg, 0);
 		check(!err);
 	}
 
@@ -978,7 +978,7 @@ static void t_empty_add(void)
 	err = reftable_new_stack(&st, dir, &opts);
 	check(!err);
 
-	err = reftable_stack_add(st, write_nothing, NULL);
+	err = reftable_stack_add(st, write_nothing, NULL, 0);
 	check(!err);
 
 	err = reftable_new_stack(&st2, dir, &opts);
@@ -1021,7 +1021,7 @@ static void t_reftable_stack_auto_compaction(void)
 		};
 		snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		err = reftable_stack_auto_compact(st);
@@ -1058,7 +1058,7 @@ static void t_reftable_stack_auto_compaction_factor(void)
 		};
 		xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref);
+		err = reftable_stack_add(st, &write_test_ref, &ref, 0);
 		check(!err);
 
 		check(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));
@@ -1140,7 +1140,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)
 		snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);
 		ref.refname = buf;
 
-		err = reftable_stack_add(st, write_test_ref, &ref);
+		err = reftable_stack_add(st, write_test_ref, &ref, 0);
 		check(!err);
 
 		/*

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 6/8] reftable/stack: handle outdated stacks when compacting

[Patrick Steinhardt]

When we compact the reftable stack we first acquire the lock for the
"tables.list" file and then reload the stack to check that it is still
up-to-date. This is done by calling `stack_uptodate()`, which knows to
return zero in case the stack is up-to-date, a positive value if it is
not and a negative error code on unexpected conditions.

We don't do proper error checking though, but instead we only check
whether the returned error code is non-zero. If so, we simply bubble it
up the calling stack, which means that callers may see an unexpected
positive value.

Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
Handle this situation in `reftable_addition_commit()`, where we perform
a best-effort auto-compaction.

All other callsites of `stack_uptodate()` know to handle a positive
return value and thus don't need to be fixed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 1ce4d90cb8..af0f94d882 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
 	return err;
 }
 
-/* -1 = error
- 0 = up to date
- 1 = changed. */
+/*
+ * Check whether the given stack is up-to-date with what we have in memory.
+ * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
+ * otherwise.
+ */
 static int stack_uptodate(struct reftable_stack *st)
 {
 	char **names = NULL;
@@ -850,10 +852,13 @@ int reftable_addition_commit(struct reftable_addition *add)
 		 * control. It is possible that a concurrent writer is already
 		 * trying to compact parts of the stack, which would lead to a
 		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
-		 * already. This is a benign error though, so we ignore it.
+		 * already. Similarly, the stack may have been rewritten by a
+		 * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
+		 * Both of these errors are benign, so we simply ignore them.
 		 */
 		err = reftable_stack_auto_compact(add->stack);
-		if (err < 0 && err != REFTABLE_LOCK_ERROR)
+		if (err < 0 && err != REFTABLE_LOCK_ERROR &&
+		    err != REFTABLE_OUTDATED_ERROR)
 			goto done;
 		err = 0;
 	}
@@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
 		goto done;
 	}
 
+	/*
+	 * Check whether the stack is up-to-date. We unfortunately cannot
+	 * handle the situation gracefully in case it's _not_ up-to-date
+	 * because the range of tables that the user has requested us to
+	 * compact may have been changed. So instead we abort.
+	 *
+	 * We could in theory improve the situation by having the caller not
+	 * pass in a range, but instead the list of tables to compact. If so,
+	 * we could check that relevant tables still exist. But for now it's
+	 * good enough to just abort.
+	 */
 	err = stack_uptodate(st);
-	if (err)
+	if (err < 0)
 		goto done;
+	if (err > 0) {
+		err = REFTABLE_OUTDATED_ERROR;
+		goto done;
+	}
 
 	/*
 	 * Lock all tables in the user-provided range. This is the slice of our

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 7/8] reftable: don't second-guess errors from flock interface

[Patrick Steinhardt]

The `flock` interface is implemented as part of "reftable/system.c" and
thus needs to be implemented by the integrator between the reftable
library and its parent code base. As such, we cannot rely on any
specific implementation thereof.

Regardless of that, users of the `flock` subsystem rely on `errno` being
set to specific values. This is fragile and not documented anywhere and
doesn't really make for a good interface.

Refactor the code so that the implementations themselves are expected to
return reftable-specific error codes. Our implementation of the `flock`
subsystem already knows to do this for all error paths except one.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c  | 37 ++++++++-----------------------------
 reftable/system.c |  2 +-
 reftable/system.h |  4 +++-
 3 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index af0f94d882..f91ce50bcd 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -698,14 +698,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 
 	err = flock_acquire(&add->tables_list_lock, st->list_file,
 			    st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST) {
-			err = REFTABLE_LOCK_ERROR;
-		} else {
-			err = REFTABLE_IO_ERROR;
-		}
+	if (err < 0)
 		goto done;
-	}
+
 	if (st->opts.default_permissions) {
 		if (chmod(add->tables_list_lock.path,
 			  st->opts.default_permissions) < 0) {
@@ -1212,13 +1207,8 @@ static int stack_compact_range(struct reftable_stack *st,
 	 * which are part of the user-specified range.
 	 */
 	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST)
-			err = REFTABLE_LOCK_ERROR;
-		else
-			err = REFTABLE_IO_ERROR;
+	if (err < 0)
 		goto done;
-	}
 
 	/*
 	 * Check whether the stack is up-to-date. We unfortunately cannot
@@ -1272,7 +1262,7 @@ static int stack_compact_range(struct reftable_stack *st,
 			 * tables, otherwise there would be nothing to compact.
 			 * In that case, we return a lock error to our caller.
 			 */
-			if (errno == EEXIST && last - (i - 1) >= 2 &&
+			if (err == REFTABLE_LOCK_ERROR && last - (i - 1) >= 2 &&
 			    flags & STACK_COMPACT_RANGE_BEST_EFFORT) {
 				err = 0;
 				/*
@@ -1284,13 +1274,9 @@ static int stack_compact_range(struct reftable_stack *st,
 				 */
 				first = (i - 1) + 1;
 				break;
-			} else if (errno == EEXIST) {
-				err = REFTABLE_LOCK_ERROR;
-				goto done;
-			} else {
-				err = REFTABLE_IO_ERROR;
-				goto done;
 			}
+
+			goto done;
 		}
 
 		/*
@@ -1299,10 +1285,8 @@ static int stack_compact_range(struct reftable_stack *st,
 		 * of tables.
 		 */
 		err = flock_close(&table_locks[nlocks++]);
-		if (err < 0) {
-			err = REFTABLE_IO_ERROR;
+		if (err < 0)
 			goto done;
-		}
 	}
 
 	/*
@@ -1334,13 +1318,8 @@ static int stack_compact_range(struct reftable_stack *st,
 	 * the new table.
 	 */
 	err = flock_acquire(&tables_list_lock, st->list_file, st->opts.lock_timeout_ms);
-	if (err < 0) {
-		if (errno == EEXIST)
-			err = REFTABLE_LOCK_ERROR;
-		else
-			err = REFTABLE_IO_ERROR;
+	if (err < 0)
 		goto done;
-	}
 
 	if (st->opts.default_permissions) {
 		if (chmod(tables_list_lock.path,
diff --git a/reftable/system.c b/reftable/system.c
index 1ee268b125..725a25844e 100644
--- a/reftable/system.c
+++ b/reftable/system.c
@@ -72,7 +72,7 @@ int flock_acquire(struct reftable_flock *l, const char *target_path,
 		reftable_free(lockfile);
 		if (errno == EEXIST)
 			return REFTABLE_LOCK_ERROR;
-		return -1;
+		return REFTABLE_IO_ERROR;
 	}
 
 	l->fd = get_lock_file_fd(lockfile);
diff --git a/reftable/system.h b/reftable/system.h
index beb9d2431f..0c623617f0 100644
--- a/reftable/system.h
+++ b/reftable/system.h
@@ -81,7 +81,9 @@ struct reftable_flock {
  * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is negative
  * we block indefinitely.
  *
- * Retrun 0 on success, a reftable error code on error.
+ * Return 0 on success, a reftable error code on error. Specifically,
+ * `REFTABLE_LOCK_ERROR` should be returned in case the target path is already
+ * locked.
  */
 int flock_acquire(struct reftable_flock *l, const char *target_path,
 		  long timeout_ms);

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

[PATCH v4 8/8] refs/reftable: always reload stacks when creating lock

[Patrick Steinhardt]

When creating a new addition via either `reftable_stack_new_addition()`
or its convenince wrapper `reftable_stack_add()` we:

  1. Create the "tables.list.lock" file.

  2. Verify that the current version of the "tables.list" file is
     up-to-date.

  3. Write the new table records if so.

By default, the second step would cause us to bail out if we see that
there has been a concurrent write to the stack that made our in-memory
copy of the stack out-of-date. This is a safety mechanism to not write
records to the stack based on outdated information.

The downside though is that concurrent writes may now cause us to bail
out, which is not a good user experience. In addition, this isn't even
necessary for us, as Git knows to perform all checks for the old state
of references under the lock. (Well, in all except one case: when we
expire the reflog we first create the log iterator before we create the
lock, but this ordering is fixed as part of this commit.)

Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
flag. The effect of this flag is that we reload the stack after having
acquired the lock in case the stack is out-of-date. This plugs the race
with concurrent writers, but we continue performing the verifications of
the expected old state to catch actual conflicts in the references we
are about to write.

Adapt the remaining callsites that don't yet pass this flag to do so.
While at it, drop a needless manual reload.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 3f0deab338..66d25411f1 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
 	if (!arg) {
 		struct reftable_addition *addition;
 
-		ret = reftable_stack_reload(be->stack);
-		if (ret)
-			return ret;
-
 		ret = reftable_stack_new_addition(&addition, be->stack,
 						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 		if (ret) {
@@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
 	if (ret)
 		goto done;
-	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
+	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 		goto done;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
+	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
 	return ret;
@@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 		return ret;
 	arg.stack = be->stack;
 
-	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
+	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
+				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
@@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_stack_init_log_iterator(be->stack, &it);
+	ret = reftable_stack_new_addition(&add, be->stack,
+					  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_iterator_seek_log(&it, refname);
+	ret = reftable_stack_init_log_iterator(be->stack, &it);
 	if (ret < 0)
 		goto done;
 
-	ret = reftable_stack_new_addition(&add, be->stack, 0);
+	ret = reftable_iterator_seek_log(&it, refname);
 	if (ret < 0)
 		goto done;
 

-- 
2.51.0.rc1.215.g0f929dcec7.dirty

Re: [PATCH v4 0/8] reftable: a couple of improvements for libgit2

[Justin Tobler]

On 25/08/13 08:25AM, Patrick Steinhardt wrote:
> Changes in v4:
>   - Adjust stale commit message.
>   - Fix a typo while at it.
>   - Link to v3: https://lore.kernel.org/r/20250812-pks-reftable-fixes-for-libgit2-v3-0-cf3b2267867e@pks.im

After reviewing the range-diff, this version looks good to me.

-Justin