[GSoC PATCH] reftable: return proper error code from block_writer_add()

[GSoC PATCH] reftable: return proper error code from block_writer_add()

[Meet Soni]

Previously, block_writer_add() used to return generic -1, which forced
an assumption about the error type.

Replace generic -1 returns in block_writer_add() and related functions
with defined error codes.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
This patch attempts to avoid making an assumption regarding error codes
returned by block_writer_add().
 reftable/block.c  |  9 +++++----
 reftable/record.c | 16 +++++++++++-----
 reftable/writer.c |  8 +-------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..50fbac801a 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (is_restart)
 		rlen++;
 	if (2 + 3 * rlen + n > w->block_size - w->next)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	if (is_restart) {
 		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
 					    w->restart_cap);
@@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	int err;
 
 	err = reftable_record_key(rec, &w->scratch);
-	if (err < 0)
+	if (err < 0) {
 		goto done;
+	}
 
 	if (!w->scratch.len) {
 		err = REFTABLE_API_ERROR;
@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
 
 	n = reftable_record_encode(rec, out, w->hash_size);
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
diff --git a/reftable/record.c b/reftable/record.c
index 8919df8a4d..5523804a0c 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
 	uint64_t suffix_len = key.len - prefix_len;
 	int n = put_var_int(&dest, prefix_len);
 	if (n < 0)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	string_view_consume(&dest, n);
 
 	*restart = (prefix_len == 0);
 
 	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
 	if (n < 0)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	string_view_consume(&dest, n);
 
 	if (dest.len < suffix_len)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
 	string_view_consume(&dest, suffix_len);
 
@@ -1144,14 +1144,20 @@ static struct reftable_record_vtable reftable_index_record_vtable = {
 
 int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)
 {
-	return reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
+	int key_len = reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
+	if (key_len < 0)
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
+	return key_len;
 }
 
 int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
 			   uint32_t hash_size)
 {
-	return reftable_record_vtable(rec)->encode(reftable_record_data(rec),
+	int encode_len = reftable_record_vtable(rec)->encode(reftable_record_data(rec),
 						   dest, hash_size);
+	if (encode_len < 0)
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
+	return encode_len;
 }
 
 int reftable_record_copy_from(struct reftable_record *rec,
diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..600ba5441b 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -327,16 +327,10 @@ static int writer_add_record(struct reftable_writer *w,
 		goto done;
 
 	/*
-	 * Try to add the record to the writer again. If this still fails then
-	 * the record does not fit into the block size.
-	 *
-	 * TODO: it would be great to have `block_writer_add()` return proper
-	 *       error codes so that we don't have to second-guess the failure
-	 *       mode here.
+	 * Try to add the record to the writer again.
 	 */
 	err = block_writer_add(w->block_writer, rec);
 	if (err) {
-		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
 		goto done;
 	}
 

base-commit: e969bc875963a10890d61ba84eab3a460bd9e535
-- 
2.34.1

Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()

[Patrick Steinhardt]

On Thu, Mar 06, 2025 at 05:43:24PM +0530, Meet Soni wrote:
> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	int err;
>  
>  	err = reftable_record_key(rec, &w->scratch);
> -	if (err < 0)
> +	if (err < 0) {
>  		goto done;
> +	}
>  
>  	if (!w->scratch.len) {
>  		err = REFTABLE_API_ERROR;

This change probably shouldn't be here. Our style guide mentions that we
prefer to not have curly braces around single-line statements.

> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	n = reftable_encode_key(&is_restart, out, last, w->scratch,
>  				reftable_record_val_type(rec));
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}
>  	string_view_consume(&out, n);
>  
>  	n = reftable_record_encode(rec, out, w->hash_size);
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}
>  	string_view_consume(&out, n);

Okay. `reftable_encode_key()` right now only knows to return generic
errors, but you fix that further down, and you also adapt
`reftable_record_encode()`.

> diff --git a/reftable/record.c b/reftable/record.c
> index 8919df8a4d..5523804a0c 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
>  	uint64_t suffix_len = key.len - prefix_len;
>  	int n = put_var_int(&dest, prefix_len);
>  	if (n < 0)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
>  	string_view_consume(&dest, n);
>  
>  	*restart = (prefix_len == 0);
>  
>  	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
>  	if (n < 0)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
>  	string_view_consume(&dest, n);
>  
>  	if (dest.len < suffix_len)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
>  	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
>  	string_view_consume(&dest, suffix_len);
>  

Makes sense.

> @@ -1144,14 +1144,20 @@ static struct reftable_record_vtable reftable_index_record_vtable = {
>  
>  int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)
>  {
> -	return reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
> +	int key_len = reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
> +	if (key_len < 0)
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
> +	return key_len;
>  }

This here is incorrect. We don't know why the `->key()` function has
failed, so we shouldn't assume `TOO_BIG_ERROR`. We'd have to vet all the
implementations of that function and then should bubble up their
respective error codes.

>  int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
>  			   uint32_t hash_size)
>  {
> -	return reftable_record_vtable(rec)->encode(reftable_record_data(rec),
> +	int encode_len = reftable_record_vtable(rec)->encode(reftable_record_data(rec),
>  						   dest, hash_size);
> +	if (encode_len < 0)
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
> +	return encode_len;
>  }
>  
>  int reftable_record_copy_from(struct reftable_record *rec,

Same remark here.

Thanks!

Patrick

Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()

[Junio C Hamano]

Meet Soni <meetsoni3017@gmail.com> writes:

> Previously, block_writer_add() used to return generic -1, which forced
> an assumption about the error type.
>
> Replace generic -1 returns in block_writer_add() and related functions
> with defined error codes.

What's missing from this proposed log message is an audit of the
callers to tell readers that this change is safe and expected by the
callers.  IOW, are there callers that start to behave differently
when they see ENTRY_TOO_BIG instead of -1, for example?

> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> ---
> This patch attempts to avoid making an assumption regarding error codes
> returned by block_writer_add().
>  reftable/block.c  |  9 +++++----
>  reftable/record.c | 16 +++++++++++-----
>  reftable/writer.c |  8 +-------
>  3 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index b14a8f1259..50fbac801a 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
>  	if (is_restart)
>  		rlen++;
>  	if (2 + 3 * rlen + n > w->block_size - w->next)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;

So this makes block_writer_register_restart() to return -11 instead
of -1; the sole caller of the function is block_writer_add() that
begins like so:

        /* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
           success. Returns REFTABLE_API_ERROR if attempting to write a record with
           empty key. */
        int block_writer_add(struct block_writer *w, struct reftable_record *rec)
        {

Needless to say, the comment before the function needs to be
adjusted together with the above hunk (and others).  But more
importantly, are existing callers of this function now expected to
adjust to the change in behaviour when they receive the return value
of this function?  It used to be sufficient for them to deal with
-1, 0 or API_ERROR, but now they are required to handle other errors
(like the one that comes back from reftable_encode_key().  Do they
already handle these new error codes just fine?  Have you traced the
code paths to see how they react to them?

> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	int err;
>  
>  	err = reftable_record_key(rec, &w->scratch);
> -	if (err < 0)
> +	if (err < 0) {
>  		goto done;
> +	}

This is unwarranted, isn't it?

> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	n = reftable_encode_key(&is_restart, out, last, w->scratch,
>  				reftable_record_val_type(rec));
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}

Here, block_writer_add() starts returning new error codes to its
callers that it never returned.

>  	string_view_consume(&out, n);
>  
>  	n = reftable_record_encode(rec, out, w->hash_size);
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}

Ditto.

Note that I am not saying that it is a bad idea to make the error
codes more specific so that the callers can tell them apart.  I am
only saying that the patch that makes such a change must also make
sure that the callers are prepared to handle error coes that they
have never seen from the current callee.

The same applies to the remainder of the patch.

Thanks.

[GSoC PATCH v2] reftable: return proper error code from block_writer_add()

[Meet Soni]

Previously, block_writer_add() used to return generic -1, which forced
an assumption about the error type. Replace these generic -1 returns in
block_writer_add() and related functions with defined error codes.

Reviewed all call sites to ensure they check for nonzero error returns
rather than strictly -1, confirming that this change is safe.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
This patch attempts to avoid making an assumption regarding error codes
returned by block_writer_add().

Changes since v1:
  - Update commit message to specify safe usage of the patch.
  - Update function doc comment.
  - Propagate errors to `->key()` and `->encode()` functions instead of 
    making any assumptions.
 
Dropped Han-Wen Nienhuys <hanwen@google.com> from CC, as the email doesn't exist.
Range-diff against v1:
1:  10d8bbeebc < -:  ---------- reftable: return proper error code from block_writer_add()
-:  ---------- > 1:  7cdc7ce0ce reftable: return proper error code from block_writer_add()

 reftable/block.c  | 12 +++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 reftable/writer.c | 11 ++--------
 4 files changed, 33 insertions(+), 45 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..89ab8bbc57 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (is_restart)
 		rlen++;
 	if (2 + 3 * rlen + n > w->block_size - w->next)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	if (is_restart) {
 		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
 					    w->restart_cap);
@@ -97,9 +97,9 @@ uint8_t block_writer_type(struct block_writer *bw)
 	return bw->block[bw->header_off];
 }
 
-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
-   success. Returns REFTABLE_API_ERROR if attempting to write a record with
-   empty key. */
+/* Adds the reftable_record to the block. Returns 0 on success and
+ * appropriate error codes on failure.
+ */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 {
 	struct reftable_buf empty = REFTABLE_BUF_INIT;
@@ -126,14 +126,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
 
 	n = reftable_record_encode(rec, out, w->hash_size);
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
diff --git a/reftable/block.h b/reftable/block.h
index bef2b8a4c5..0e7c680cf6 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
 /* returns the block type (eg. 'r' for ref records. */
 uint8_t block_writer_type(struct block_writer *bw);
 
-/* appends the record, or -1 if it doesn't fit. */
+/* attempts to append the record. returns 0 on success or error code on failure. */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec);
 
 /* appends the key restarts, and compress the block if necessary. */
diff --git a/reftable/record.c b/reftable/record.c
index 8919df8a4d..d9fba8ff38 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)
 	while (value >>= 7)
 		varint[--pos] = 0x80 | (--value & 0x7f);
 	if (dest->len < sizeof(varint) - pos)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest->buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)
 	size_t l = strlen(str);
 	int n = put_var_int(&s, l);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 	if (s.len < l)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(s.buf, str, l);
 	string_view_consume(&s, l);
 
@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
 	uint64_t suffix_len = key.len - prefix_len;
 	int n = put_var_int(&dest, prefix_len);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	*restart = (prefix_len == 0);
 
 	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	if (dest.len < suffix_len)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
 	string_view_consume(&dest, suffix_len);
 
@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
 	struct string_view start = s;
 	int n = put_var_int(&s, r->update_index);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	switch (r->value_type) {
 	case REFTABLE_REF_SYMREF:
 		n = encode_string(r->value.symref, s);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		break;
 	case REFTABLE_REF_VAL2:
-		if (s.len < 2 * hash_size) {
-			return -1;
-		}
+		if (s.len < 2 * hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val2.value, hash_size);
 		string_view_consume(&s, hash_size);
 		memcpy(s.buf, r->value.val2.target_value, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
 	case REFTABLE_REF_VAL1:
-		if (s.len < hash_size) {
-			return -1;
-		}
+		if (s.len < hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val1, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
 	uint64_t last = 0;
 	if (r->offset_len == 0 || r->offset_len >= 8) {
 		n = put_var_int(&s, r->offset_len);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 	}
 	if (r->offset_len == 0)
 		return start.len - s.len;
 	n = put_var_int(&s, r->offsets[0]);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	last = r->offsets[0];
 	for (i = 1; i < r->offset_len; i++) {
 		int n = put_var_int(&s, r->offsets[i] - last);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		last = r->offsets[i];
 	}
@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 		return 0;
 
 	if (s.len < 2 * hash_size)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	memcpy(s.buf, r->value.update.old_hash, hash_size);
 	memcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);
@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 
 	n = encode_string(r->value.update.name ? r->value.update.name : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = encode_string(r->value.update.email ? r->value.update.email : "",
 			  s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = put_var_int(&s, r->value.update.time);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	if (s.len < 2)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	put_be16(s.buf, r->value.update.tz_offset);
 	string_view_consume(&s, 2);
@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 	n = encode_string(
 		r->value.update.message ? r->value.update.message : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	return start.len - s.len;
diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..5cb9d0bf85 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -327,18 +327,11 @@ static int writer_add_record(struct reftable_writer *w,
 		goto done;
 
 	/*
-	 * Try to add the record to the writer again. If this still fails then
-	 * the record does not fit into the block size.
-	 *
-	 * TODO: it would be great to have `block_writer_add()` return proper
-	 *       error codes so that we don't have to second-guess the failure
-	 *       mode here.
+	 * Try to add the record to the writer again.
 	 */
 	err = block_writer_add(w->block_writer, rec);
-	if (err) {
-		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
+	if (err)
 		goto done;
-	}
 
 done:
 	return err;
-- 
2.34.1

Re: [GSoC PATCH v2] reftable: return proper error code from block_writer_add()

[Patrick Steinhardt]

On Sat, Mar 08, 2025 at 07:03:49PM +0530, Meet Soni wrote:
> diff --git a/reftable/block.c b/reftable/block.c
> index b14a8f1259..89ab8bbc57 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
>  	if (is_restart)
>  		rlen++;
>  	if (2 + 3 * rlen + n > w->block_size - w->next)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;
>  	if (is_restart) {
>  		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
>  					    w->restart_cap);
> @@ -97,9 +97,9 @@ uint8_t block_writer_type(struct block_writer *bw)
>  	return bw->block[bw->header_off];
>  }
>  
> -/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
> -   success. Returns REFTABLE_API_ERROR if attempting to write a record with
> -   empty key. */
> +/* Adds the reftable_record to the block. Returns 0 on success and
> + * appropriate error codes on failure.
> + */
>  int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  {
>  	struct reftable_buf empty = REFTABLE_BUF_INIT;

I'm in favor of touching up the comment's formatting while at it, but if
we do so we should use the correct style, which has the opening and
closing parts on their own line:

    /*
     * Yadda yadda.
     */

> diff --git a/reftable/block.h b/reftable/block.h
> index bef2b8a4c5..0e7c680cf6 100644
> --- a/reftable/block.h
> +++ b/reftable/block.h
> @@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
>  /* returns the block type (eg. 'r' for ref records. */
>  uint8_t block_writer_type(struct block_writer *bw);
>  
> -/* appends the record, or -1 if it doesn't fit. */
> +/* attempts to append the record. returns 0 on success or error code on failure. */
>  int block_writer_add(struct block_writer *w, struct reftable_record *rec);
>  
>  /* appends the key restarts, and compress the block if necessary. */

We might also touch up this comment to start with an upper-case "A"
while at it.

> diff --git a/reftable/record.c b/reftable/record.c
> index 8919df8a4d..d9fba8ff38 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> diff --git a/reftable/writer.c b/reftable/writer.c
> index f3ab1035d6..5cb9d0bf85 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -327,18 +327,11 @@ static int writer_add_record(struct reftable_writer *w,
>  		goto done;
>  
>  	/*
> -	 * Try to add the record to the writer again. If this still fails then
> -	 * the record does not fit into the block size.
> -	 * TODO: it would be great to have `block_writer_add()` return proper
> -	 *       error codes so that we don't have to second-guess the failure
> -	 *       mode here.
> +	 * Try to add the record to the writer again.
>  	 */
>  	err = block_writer_add(w->block_writer, rec);
> -	if (err) {
> -		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
> +	if (err)
>  		goto done;
> -	}

Let's not drop the second sentence of the comment, as it is important to
give context. Also, let's take a step back here and figure out what this
function is doing:

  1. We compute the record data and try to append it to the current
     block. If this succeeds, we can return immediately and are done.

  2. If appending to the current block fails we assume that we have
     failed because the block is full. This is because reftable blocks
     have a specific maximum length that we cannot exceed. We thus
     flush the block and start writing a new one.

  3. We now try to add the same record to the new block again and hope
     that we can now write the record successfully.

The important part that the TODO comment refers to is in (2), indicated
by "assume": we don't actually check what the error is that we've got
from `block_writer_add()`, but simply pretend as if it was
`REFTABLE_ENTRY_TOO_BIG_ERROR`. This means that we'd even re-try writing
the record in case we had for example a memory allocation failure, or an
I/O error, and that is plain wrong.

With your changes we have now started to plumb proper errors through from
`block_writer_add()`. But that doesn't mean we can just drop the comment
and bubble up the error. Instead, we should also be adapting the code in
(2) to do the right thing: we shouldn't _assume_ that the current block
is full, but instead check the error code returned by the first call to
`block_writer_add()`:

  - If it is `REFTABLE_ENTRY_TOO_BIG_ERROR` we indeed should flush the
    current block and try to write a new one.

  - Otherwise we bail out and bubble up the error.

And once we do that, it is fine to remove the comment indeed. It's
somewhat funny because from my point of view the comment is in the wrong
spot: it does correctly point out that _this_ particular callsite is
doing the wrong thing, but it didn't mention that the other callsite
also has the same problem. And that other callsite is more important
from my perspective.

So I'd recommend to split up this commit into two commits:

  - The first commit prepares all transitively called functions as you
    already do.

  - The second commit adapts "reftable/writer.c" and fixes both
    callsites of `block_writer_add()` to do proper error handling.

Patrick

[GSoC PATCH v3 0/2] reftable: return proper error codes from block_writer_add

[Meet Soni]

This patch attempts to avoid making an assumption regarding error codes
returned by block_writer_add().

Changes since v2:
    - Split the commit into two to separate transitively called function
      updates from writer call-site adaptations
    - Made formatting improvements in comments and code for better 
      readability.
    - Modified the writer logic to flush and retry only when a specific
      error occurs, while other errors are propagated as-is.

Meet Soni (2):
  reftable: propagate specific error codes in block_writer_add()
  reftable: adapt writer code to propagate block_writer_add() errors

 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 reftable/writer.c | 30 ++++++++++++++++-----------
 4 files changed, 50 insertions(+), 48 deletions(-)

Range-diff against v2:
1:  7cdc7ce0ce ! 1:  6ab35d569c reftable: return proper error code from block_writer_add()
    @@ Metadata
     Author: Meet Soni <meetsoni3017@gmail.com>
     
      ## Commit message ##
    -    reftable: return proper error code from block_writer_add()
    +    reftable: propagate specific error codes in block_writer_add()
     
    -    Previously, block_writer_add() used to return generic -1, which forced
    -    an assumption about the error type. Replace these generic -1 returns in
    -    block_writer_add() and related functions with defined error codes.
    +    Previously, functions block_writer_add() and related functions returned
    +    -1 when the record did not fit, forcing the caller to assume that any
    +    failure meant the entry was too big. Replace these generic -1 returns
    +    with defined error codes.
     
    -    Reviewed all call sites to ensure they check for nonzero error returns
    -    rather than strictly -1, confirming that this change is safe.
    +    This prepares the codebase for finer-grained error handling so that
    +    callers can distinguish between a block-full condition and other errors.
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     
    @@ reftable/block.c: uint8_t block_writer_type(struct block_writer *bw)
     -/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
     -   success. Returns REFTABLE_API_ERROR if attempting to write a record with
     -   empty key. */
    -+/* Adds the reftable_record to the block. Returns 0 on success and
    ++/*
    ++ * Adds the reftable_record to the block. Returns 0 on success and
     + * appropriate error codes on failure.
     + */
      int block_writer_add(struct block_writer *w, struct reftable_record *rec)
    @@ reftable/block.h: int block_writer_init(struct block_writer *bw, uint8_t typ, ui
      uint8_t block_writer_type(struct block_writer *bw);
      
     -/* appends the record, or -1 if it doesn't fit. */
    -+/* attempts to append the record. returns 0 on success or error code on failure. */
    ++/* Attempts to append the record. Returns 0 on success or error code on failure. */
      int block_writer_add(struct block_writer *w, struct reftable_record *rec);
      
      /* appends the key restarts, and compress the block if necessary. */
    @@ reftable/record.c: static int reftable_log_record_encode(const void *rec, struct
      	string_view_consume(&s, n);
      
      	return start.len - s.len;
    -
    - ## reftable/writer.c ##
    -@@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
    - 		goto done;
    - 
    - 	/*
    --	 * Try to add the record to the writer again. If this still fails then
    --	 * the record does not fit into the block size.
    --	 *
    --	 * TODO: it would be great to have `block_writer_add()` return proper
    --	 *       error codes so that we don't have to second-guess the failure
    --	 *       mode here.
    -+	 * Try to add the record to the writer again.
    - 	 */
    - 	err = block_writer_add(w->block_writer, rec);
    --	if (err) {
    --		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
    -+	if (err)
    - 		goto done;
    --	}
    - 
    - done:
    - 	return err;
-:  ---------- > 2:  a54d440dd3 reftable: adapt writer code to propagate block_writer_add() errors
-- 
2.34.1

[PATCH v3 1/2] reftable: propagate specific error codes in block_writer_add()

[Meet Soni]

Previously, functions block_writer_add() and related functions returned
-1 when the record did not fit, forcing the caller to assume that any
failure meant the entry was too big. Replace these generic -1 returns
with defined error codes.

This prepares the codebase for finer-grained error handling so that
callers can distinguish between a block-full condition and other errors.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..0b8ebc3aa5 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (is_restart)
 		rlen++;
 	if (2 + 3 * rlen + n > w->block_size - w->next)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	if (is_restart) {
 		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
 					    w->restart_cap);
@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)
 	return bw->block[bw->header_off];
 }
 
-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
-   success. Returns REFTABLE_API_ERROR if attempting to write a record with
-   empty key. */
+/*
+ * Adds the reftable_record to the block. Returns 0 on success and
+ * appropriate error codes on failure.
+ */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 {
 	struct reftable_buf empty = REFTABLE_BUF_INIT;
@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
 
 	n = reftable_record_encode(rec, out, w->hash_size);
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
diff --git a/reftable/block.h b/reftable/block.h
index bef2b8a4c5..64732eba7d 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
 /* returns the block type (eg. 'r' for ref records. */
 uint8_t block_writer_type(struct block_writer *bw);
 
-/* appends the record, or -1 if it doesn't fit. */
+/* Attempts to append the record. Returns 0 on success or error code on failure. */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec);
 
 /* appends the key restarts, and compress the block if necessary. */
diff --git a/reftable/record.c b/reftable/record.c
index 8919df8a4d..d9fba8ff38 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)
 	while (value >>= 7)
 		varint[--pos] = 0x80 | (--value & 0x7f);
 	if (dest->len < sizeof(varint) - pos)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest->buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)
 	size_t l = strlen(str);
 	int n = put_var_int(&s, l);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 	if (s.len < l)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(s.buf, str, l);
 	string_view_consume(&s, l);
 
@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
 	uint64_t suffix_len = key.len - prefix_len;
 	int n = put_var_int(&dest, prefix_len);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	*restart = (prefix_len == 0);
 
 	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	if (dest.len < suffix_len)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
 	string_view_consume(&dest, suffix_len);
 
@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
 	struct string_view start = s;
 	int n = put_var_int(&s, r->update_index);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	switch (r->value_type) {
 	case REFTABLE_REF_SYMREF:
 		n = encode_string(r->value.symref, s);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		break;
 	case REFTABLE_REF_VAL2:
-		if (s.len < 2 * hash_size) {
-			return -1;
-		}
+		if (s.len < 2 * hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val2.value, hash_size);
 		string_view_consume(&s, hash_size);
 		memcpy(s.buf, r->value.val2.target_value, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
 	case REFTABLE_REF_VAL1:
-		if (s.len < hash_size) {
-			return -1;
-		}
+		if (s.len < hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val1, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
 	uint64_t last = 0;
 	if (r->offset_len == 0 || r->offset_len >= 8) {
 		n = put_var_int(&s, r->offset_len);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 	}
 	if (r->offset_len == 0)
 		return start.len - s.len;
 	n = put_var_int(&s, r->offsets[0]);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	last = r->offsets[0];
 	for (i = 1; i < r->offset_len; i++) {
 		int n = put_var_int(&s, r->offsets[i] - last);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		last = r->offsets[i];
 	}
@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 		return 0;
 
 	if (s.len < 2 * hash_size)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	memcpy(s.buf, r->value.update.old_hash, hash_size);
 	memcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);
@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 
 	n = encode_string(r->value.update.name ? r->value.update.name : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = encode_string(r->value.update.email ? r->value.update.email : "",
 			  s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = put_var_int(&s, r->value.update.time);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	if (s.len < 2)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	put_be16(s.buf, r->value.update.tz_offset);
 	string_view_consume(&s, 2);
@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 	n = encode_string(
 		r->value.update.message ? r->value.update.message : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	return start.len - s.len;
-- 
2.34.1

[PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors

[Meet Soni]

Previously, writer_add_record() and write_object_record() would flush the
current block and retry appending the record whenever block_writer_add()
returned any nonzero error. This forced an assumption that every failure
meant the block was full, even when errors such as memory allocation or
I/O failures occurred.

Update the writer code to inspect the error code returned by
block_writer_add() and only flush and reinitialize the writer when the
error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
propagate it.

All call sites now handle various error codes returned by
block_writer_add().

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/writer.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..0d8181e227 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,
 	 * done. Otherwise the block writer may have hit the block size limit
 	 * and needs to be flushed.
 	 */
-	if (!block_writer_add(w->block_writer, rec)) {
-		err = 0;
+	err = block_writer_add(w->block_writer, rec);
+	if (err == 0)
 		goto done;
-	}
 
+	if (err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 	/*
 	 * The current block is full, so we need to flush and reinitialize the
 	 * writer to start writing the next block.
@@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w,
 		goto done;
 
 	/*
-	 * Try to add the record to the writer again. If this still fails then
-	 * the record does not fit into the block size.
-	 *
-	 * TODO: it would be great to have `block_writer_add()` return proper
-	 *       error codes so that we don't have to second-guess the failure
-	 *       mode here.
+	 * Try to add the record to the writer again.
 	 */
 	err = block_writer_add(w->block_writer, rec);
-	if (err) {
-		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
+	if (err)
 		goto done;
-	}
 
 done:
 	return err;
@@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key)
 	if (arg->err < 0)
 		goto done;
 
+	/*
+	 * Try to add the record to the writer. If this succeeds then we're
+	 * done. Otherwise the block writer may have hit the block size limit
+	 * and needs to be flushed.
+	 */
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
 
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
+
+	/*
+	 * The current block is full, so we need to flush and reinitialize the
+	 * writer to start writing the next block.
+	 */
 	arg->err = writer_flush_block(arg->w);
 	if (arg->err < 0)
 		goto done;
-- 
2.34.1

Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors

[Patrick Steinhardt]

On Wed, Mar 12, 2025 at 05:41:48PM +0530, Meet Soni wrote:
> diff --git a/reftable/writer.c b/reftable/writer.c
> index f3ab1035d6..0d8181e227 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,
>  	 * done. Otherwise the block writer may have hit the block size limit
>  	 * and needs to be flushed.
>  	 */
> -	if (!block_writer_add(w->block_writer, rec)) {
> -		err = 0;
> +	err = block_writer_add(w->block_writer, rec);
> +	if (err == 0)
>  		goto done;
> -	}

Style: we'd typically say `if (!err)` here, even though I see that we
have explicit comparisons with 0 elsewhere in this file, too. So I guess
ultimately this is okay.

> @@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w,
>  		goto done;
>  
>  	/*
> -	 * Try to add the record to the writer again. If this still fails then
> -	 * the record does not fit into the block size.
> -	 *
> -	 * TODO: it would be great to have `block_writer_add()` return proper
> -	 *       error codes so that we don't have to second-guess the failure
> -	 *       mode here.
> +	 * Try to add the record to the writer again.
>  	 */

My comment on the preceding version still applies here: the second
sentence (the one starting with "If this still fails...") should be
retained.

>  	err = block_writer_add(w->block_writer, rec);
> -	if (err) {
> -		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
> +	if (err)
>  		goto done;
> -	}
>  
>  done:
>  	return err;
> @@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key)
>  	if (arg->err < 0)
>  		goto done;
>  
> +	/*
> +	 * Try to add the record to the writer. If this succeeds then we're
> +	 * done. Otherwise the block writer may have hit the block size limit
> +	 * and needs to be flushed.
> +	 */
>  	arg->err = block_writer_add(arg->w->block_writer, &rec);
>  	if (arg->err == 0)
>  		goto done;
>  
> +	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
> +		goto done;

Good catch that there is another such pattern!

> +	/*
> +	 * The current block is full, so we need to flush and reinitialize the
> +	 * writer to start writing the next block.
> +	 */
>  	arg->err = writer_flush_block(arg->w);
>  	if (arg->err < 0)
>  		goto done;

But there is another case further down where we do `block_writer_add()`
and then re-try in case the write fails. This one is a bit more curious:
if the write fails, we don't create a new block -- after all we have
just created one. Instead, we reset the record's offset length to zero
before retrying.

I _think_ that this is done because we know that when resetting the
offset we would write less data to the block, as can be seen in
`reftable_obj_record_encode()`. But I'm honestly not quite sure here as
I haven't yet done a deep dive into object records -- after all, we
don't even really use them in Git.

In any case, I think that this callsite also needs adjustment and
warrants a comment. And if so, all changes to `write_object_record()`
should probably go into a separate commit, as well.

Thanks!

Patrick

Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors

[Meet Soni]

On Wed, 12 Mar 2025 at 18:19, Patrick Steinhardt <ps@pks.im> wrote:
>
> > +     /*
> > +      * The current block is full, so we need to flush and reinitialize the
> > +      * writer to start writing the next block.
> > +      */
> >       arg->err = writer_flush_block(arg->w);
> >       if (arg->err < 0)
> >               goto done;
>
> But there is another case further down where we do `block_writer_add()`
> and then re-try in case the write fails. This one is a bit more curious:
> if the write fails, we don't create a new block -- after all we have
> just created one. Instead, we reset the record's offset length to zero
> before retrying.
>
> I _think_ that this is done because we know that when resetting the
> offset we would write less data to the block, as can be seen in
> `reftable_obj_record_encode()`. But I'm honestly not quite sure here as
> I haven't yet done a deep dive into object records -- after all, we
> don't even really use them in Git.
>
> In any case, I think that this callsite also needs adjustment and
> warrants a comment. And if so, all changes to `write_object_record()`
> should probably go into a separate commit, as well.
>

Regarding the callsite in write_object_record() where we reset the
record's offset length to zero before retrying: my changes currently
follow the same principle.

    - If block_writer_add() returns an error other than
      REFTABLE_ENTRY_TOO_BIG_ERROR, we simply return.

    - For REFTABLE_ENTRY_TOO_BIG_ERROR, we flush the block and retry.

    - If that fails, we reset the record's offset length to zero and
      then retry.

I'm not sure what adjustments or additional comments you are referring to.
Could you please clarify what changes you expect at this callsite?

Thanks!
Meet

Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors

[Patrick Steinhardt]

On Thu, Mar 13, 2025 at 08:59:51PM +0530, Meet Soni wrote:
> On Wed, 12 Mar 2025 at 18:19, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > > +     /*
> > > +      * The current block is full, so we need to flush and reinitialize the
> > > +      * writer to start writing the next block.
> > > +      */
> > >       arg->err = writer_flush_block(arg->w);
> > >       if (arg->err < 0)
> > >               goto done;
> >
> > But there is another case further down where we do `block_writer_add()`
> > and then re-try in case the write fails. This one is a bit more curious:
> > if the write fails, we don't create a new block -- after all we have
> > just created one. Instead, we reset the record's offset length to zero
> > before retrying.
> >
> > I _think_ that this is done because we know that when resetting the
> > offset we would write less data to the block, as can be seen in
> > `reftable_obj_record_encode()`. But I'm honestly not quite sure here as
> > I haven't yet done a deep dive into object records -- after all, we
> > don't even really use them in Git.
> >
> > In any case, I think that this callsite also needs adjustment and
> > warrants a comment. And if so, all changes to `write_object_record()`
> > should probably go into a separate commit, as well.
> >

Sorry for taking so long to respond.

> Regarding the callsite in write_object_record() where we reset the
> record's offset length to zero before retrying: my changes currently
> follow the same principle.
> 
>     - If block_writer_add() returns an error other than
>       REFTABLE_ENTRY_TOO_BIG_ERROR, we simply return.
> 
>     - For REFTABLE_ENTRY_TOO_BIG_ERROR, we flush the block and retry.
> 
>     - If that fails, we reset the record's offset length to zero and
>       then retry.

It's this last step that also needs to be adapted to check for
REFTABLE_ENTRY_TOO_BIG_ERROR, because the intent here is the exact same:
if writing the object record failed even in a completely new block then
we reset the object's offset and try a third time. But same as for the
first time, we don't check whether we get REFTABLE_ENTRY_TOO_BIG_ERROR
here and thus we might be failing and retrying even in unintended cases.

> I'm not sure what adjustments or additional comments you are referring to.
> Could you please clarify what changes you expect at this callsite?

So overall we'd add the check to both callsites, like in the below
patch.

Patrick

diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d61..63629e00a37 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -628,6 +628,8 @@ static void write_object_record(void *void_arg, void *key)
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 
 	arg->err = writer_flush_block(arg->w);
 	if (arg->err < 0)
@@ -640,6 +642,8 @@ static void write_object_record(void *void_arg, void *key)
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 
 	rec.u.obj.offset_len = 0;
 	arg->err = block_writer_add(arg->w->block_writer, &rec);

[GSoC PATCH v4 0/3] reftable: return proper error codes from block_writer_add

[Meet Soni]

This patch attempts to avoid making an assumption regarding error codes
returned by block_writer_add().

Changes since v3:
    - split commit based on the functions it alters
    - add comment back that was earlier removed.

Meet Soni (3):
  reftable: propagate specific error codes in block_writer_add()
  reftable: adapt writer_add_record() to propagate block_writer_add()
    errors
  reftable: adapt write_object_record() to propagate block_writer_add()
    errors

 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 reftable/writer.c | 27 +++++++++++++++---------
 4 files changed, 49 insertions(+), 46 deletions(-)

Range-diff against v3:
1:  6ab35d569c = 1:  6ab35d569c reftable: propagate specific error codes in block_writer_add()
2:  a54d440dd3 ! 2:  7f0bdc27e1 reftable: adapt writer code to propagate block_writer_add() errors
    @@ Metadata
     Author: Meet Soni <meetsoni3017@gmail.com>
     
      ## Commit message ##
    -    reftable: adapt writer code to propagate block_writer_add() errors
    +    reftable: adapt writer_add_record() to propagate block_writer_add() errors
     
    -    Previously, writer_add_record() and write_object_record() would flush the
    -    current block and retry appending the record whenever block_writer_add()
    -    returned any nonzero error. This forced an assumption that every failure
    -    meant the block was full, even when errors such as memory allocation or
    -    I/O failures occurred.
    +        Previously, writer_add_record() would flush the current block and
    +        retry appending the record whenever block_writer_add() returned any
    +        nonzero error. This forced an assumption that every failure meant
    +        the block was full, even when errors such as memory allocation or I/O
    +        failures occurred.
     
    -    Update the writer code to inspect the error code returned by
    -    block_writer_add() and only flush and reinitialize the writer when the
    -    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    -    propagate it.
    -
    -    All call sites now handle various error codes returned by
    -    block_writer_add().
    +        Update the writer_add_record() to inspect the error code returned by
    +        block_writer_add() and only flush and reinitialize the writer when the
    +        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    +        propagate it.
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     
    @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
      	 * The current block is full, so we need to flush and reinitialize the
      	 * writer to start writing the next block.
     @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
    - 		goto done;
    - 
      	/*
    --	 * Try to add the record to the writer again. If this still fails then
    --	 * the record does not fit into the block size.
    + 	 * Try to add the record to the writer again. If this still fails then
    + 	 * the record does not fit into the block size.
     -	 *
     -	 * TODO: it would be great to have `block_writer_add()` return proper
     -	 *       error codes so that we don't have to second-guess the failure
     -	 *       mode here.
    -+	 * Try to add the record to the writer again.
      	 */
      	err = block_writer_add(w->block_writer, rec);
     -	if (err) {
    @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,
      
      done:
      	return err;
    -@@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)
    - 	if (arg->err < 0)
    - 		goto done;
    - 
    -+	/*
    -+	 * Try to add the record to the writer. If this succeeds then we're
    -+	 * done. Otherwise the block writer may have hit the block size limit
    -+	 * and needs to be flushed.
    -+	 */
    - 	arg->err = block_writer_add(arg->w->block_writer, &rec);
    - 	if (arg->err == 0)
    - 		goto done;
    - 
    -+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
    -+		goto done;
    -+
    -+	/*
    -+	 * The current block is full, so we need to flush and reinitialize the
    -+	 * writer to start writing the next block.
    -+	 */
    - 	arg->err = writer_flush_block(arg->w);
    - 	if (arg->err < 0)
    - 		goto done;
-:  ---------- > 3:  480ac27797 reftable: adapt write_object_record() to propagate block_writer_add() errors
-- 
2.34.1

[PATCH v4 1/3] reftable: propagate specific error codes in block_writer_add()

[Meet Soni]

Previously, functions block_writer_add() and related functions returned
-1 when the record did not fit, forcing the caller to assume that any
failure meant the entry was too big. Replace these generic -1 returns
with defined error codes.

This prepares the codebase for finer-grained error handling so that
callers can distinguish between a block-full condition and other errors.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..0b8ebc3aa5 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (is_restart)
 		rlen++;
 	if (2 + 3 * rlen + n > w->block_size - w->next)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	if (is_restart) {
 		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
 					    w->restart_cap);
@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)
 	return bw->block[bw->header_off];
 }
 
-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
-   success. Returns REFTABLE_API_ERROR if attempting to write a record with
-   empty key. */
+/*
+ * Adds the reftable_record to the block. Returns 0 on success and
+ * appropriate error codes on failure.
+ */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 {
 	struct reftable_buf empty = REFTABLE_BUF_INIT;
@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
 
 	n = reftable_record_encode(rec, out, w->hash_size);
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
diff --git a/reftable/block.h b/reftable/block.h
index bef2b8a4c5..64732eba7d 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
 /* returns the block type (eg. 'r' for ref records. */
 uint8_t block_writer_type(struct block_writer *bw);
 
-/* appends the record, or -1 if it doesn't fit. */
+/* Attempts to append the record. Returns 0 on success or error code on failure. */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec);
 
 /* appends the key restarts, and compress the block if necessary. */
diff --git a/reftable/record.c b/reftable/record.c
index 8919df8a4d..d9fba8ff38 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)
 	while (value >>= 7)
 		varint[--pos] = 0x80 | (--value & 0x7f);
 	if (dest->len < sizeof(varint) - pos)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest->buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)
 	size_t l = strlen(str);
 	int n = put_var_int(&s, l);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 	if (s.len < l)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(s.buf, str, l);
 	string_view_consume(&s, l);
 
@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
 	uint64_t suffix_len = key.len - prefix_len;
 	int n = put_var_int(&dest, prefix_len);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	*restart = (prefix_len == 0);
 
 	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	if (dest.len < suffix_len)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
 	string_view_consume(&dest, suffix_len);
 
@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
 	struct string_view start = s;
 	int n = put_var_int(&s, r->update_index);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	switch (r->value_type) {
 	case REFTABLE_REF_SYMREF:
 		n = encode_string(r->value.symref, s);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		break;
 	case REFTABLE_REF_VAL2:
-		if (s.len < 2 * hash_size) {
-			return -1;
-		}
+		if (s.len < 2 * hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val2.value, hash_size);
 		string_view_consume(&s, hash_size);
 		memcpy(s.buf, r->value.val2.target_value, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
 	case REFTABLE_REF_VAL1:
-		if (s.len < hash_size) {
-			return -1;
-		}
+		if (s.len < hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val1, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
 	uint64_t last = 0;
 	if (r->offset_len == 0 || r->offset_len >= 8) {
 		n = put_var_int(&s, r->offset_len);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 	}
 	if (r->offset_len == 0)
 		return start.len - s.len;
 	n = put_var_int(&s, r->offsets[0]);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	last = r->offsets[0];
 	for (i = 1; i < r->offset_len; i++) {
 		int n = put_var_int(&s, r->offsets[i] - last);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		last = r->offsets[i];
 	}
@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 		return 0;
 
 	if (s.len < 2 * hash_size)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	memcpy(s.buf, r->value.update.old_hash, hash_size);
 	memcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);
@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 
 	n = encode_string(r->value.update.name ? r->value.update.name : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = encode_string(r->value.update.email ? r->value.update.email : "",
 			  s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = put_var_int(&s, r->value.update.time);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	if (s.len < 2)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	put_be16(s.buf, r->value.update.tz_offset);
 	string_view_consume(&s, 2);
@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 	n = encode_string(
 		r->value.update.message ? r->value.update.message : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	return start.len - s.len;
-- 
2.34.1

[PATCH v4 2/3] reftable: adapt writer_add_record() to propagate block_writer_add() errors

[Meet Soni]

    Previously, writer_add_record() would flush the current block and
    retry appending the record whenever block_writer_add() returned any
    nonzero error. This forced an assumption that every failure meant
    the block was full, even when errors such as memory allocation or I/O
    failures occurred.

    Update the writer_add_record() to inspect the error code returned by
    block_writer_add() and only flush and reinitialize the writer when the
    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    propagate it.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/writer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..94c97b7ac0 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,
 	 * done. Otherwise the block writer may have hit the block size limit
 	 * and needs to be flushed.
 	 */
-	if (!block_writer_add(w->block_writer, rec)) {
-		err = 0;
+	err = block_writer_add(w->block_writer, rec);
+	if (err == 0)
 		goto done;
-	}
 
+	if (err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 	/*
 	 * The current block is full, so we need to flush and reinitialize the
 	 * writer to start writing the next block.
@@ -329,16 +330,10 @@ static int writer_add_record(struct reftable_writer *w,
 	/*
 	 * Try to add the record to the writer again. If this still fails then
 	 * the record does not fit into the block size.
-	 *
-	 * TODO: it would be great to have `block_writer_add()` return proper
-	 *       error codes so that we don't have to second-guess the failure
-	 *       mode here.
 	 */
 	err = block_writer_add(w->block_writer, rec);
-	if (err) {
-		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
+	if (err)
 		goto done;
-	}
 
 done:
 	return err;
-- 
2.34.1

[PATCH v4 3/3] reftable: adapt write_object_record() to propagate block_writer_add() errors

[Meet Soni]

    Previously, write_object_record() would flush the current block and
    retry appending the record whenever block_writer_add() returned any
    nonzero error. This forced an assumption that every failure meant the
    block was full, even when errors such as memory allocation or I/O
    failures occurred.

    Update the write_object_record() to inspect the error code returned by
    block_writer_add() and only flush and reinitialize the writer when the
    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    propagate it.

    All call sites now handle various error codes returned by
    block_writer_add().

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/writer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/reftable/writer.c b/reftable/writer.c
index 94c97b7ac0..3fdfa4d34b 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -620,10 +620,22 @@ static void write_object_record(void *void_arg, void *key)
 	if (arg->err < 0)
 		goto done;
 
+	/*
+	 * Try to add the record to the writer. If this succeeds then we're
+	 * done. Otherwise the block writer may have hit the block size limit
+	 * and needs to be flushed.
+	 */
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
 
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
+
+	/*
+	 * The current block is full, so we need to flush and reinitialize the
+	 * writer to start writing the next block.
+	 */
 	arg->err = writer_flush_block(arg->w);
 	if (arg->err < 0)
 		goto done;
-- 
2.34.1

[GSoC PATCH v5 0/3] reftable: return proper error codes from block_writer_add

[Meet Soni]

This patch series attempts to avoid making an assumption regarding error codes
returned by block_writer_add().

Changes since v4:
    - update commit message.
    - add documentation comment.


Meet Soni (3):
  reftable: propagate specific error codes in block_writer_add()
  reftable: adapt writer_add_record() to propagate block_writer_add()
    errors
  reftable: adapt write_object_record() to propagate block_writer_add()
    errors

 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 reftable/writer.c | 34 +++++++++++++++++++++---------
 4 files changed, 56 insertions(+), 46 deletions(-)

Range-diff against v4:
1:  6ab35d569c = 1:  6ab35d569c reftable: propagate specific error codes in block_writer_add()
2:  7f0bdc27e1 ! 2:  873a991a2c reftable: adapt writer_add_record() to propagate block_writer_add() errors
    @@ Metadata
      ## Commit message ##
         reftable: adapt writer_add_record() to propagate block_writer_add() errors
     
    -        Previously, writer_add_record() would flush the current block and
    -        retry appending the record whenever block_writer_add() returned any
    -        nonzero error. This forced an assumption that every failure meant
    -        the block was full, even when errors such as memory allocation or I/O
    -        failures occurred.
    +    Previously, writer_add_record() would flush the current block and retry
    +    appending the record whenever block_writer_add() returned any nonzero
    +    error. This forced an assumption that every failure meant the block was
    +    full, even when errors such as memory allocation or I/O failures occurred.
     
    -        Update the writer_add_record() to inspect the error code returned by
    -        block_writer_add() and only flush and reinitialize the writer when the
    -        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    -        propagate it.
    +    Update the writer_add_record() to inspect the error code returned by
    +    block_writer_add() and only flush and reinitialize the writer when the
    +    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    +    propagate it.
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     
3:  480ac27797 ! 3:  1e2f7ff83f reftable: adapt write_object_record() to propagate block_writer_add() errors
    @@ Metadata
      ## Commit message ##
         reftable: adapt write_object_record() to propagate block_writer_add() errors
     
    -        Previously, write_object_record() would flush the current block and
    -        retry appending the record whenever block_writer_add() returned any
    -        nonzero error. This forced an assumption that every failure meant the
    -        block was full, even when errors such as memory allocation or I/O
    -        failures occurred.
    +    Previously, write_object_record() would flush the current block and retry
    +    appending the record whenever block_writer_add() returned any nonzero
    +    error. This forced an assumption that every failure meant the block was
    +    full, even when errors such as memory allocation or I/O failures occurred.
     
    -        Update the write_object_record() to inspect the error code returned by
    -        block_writer_add() and only flush and reinitialize the writer when the
    -        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    -        propagate it.
    +    Update the write_object_record() to inspect the error code returned by
    +    block_writer_add() and flush and reinitialize the writer iff the
    +    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
    +    propagate it.
     
    -        All call sites now handle various error codes returned by
    -        block_writer_add().
    +    If the flush and reinitialization still fail with
    +    REFTABLE_ENTRY_TOO_BIG_ERROR, reset the record's offset length to zero
    +    before a final attempt.
    +
    +    All call sites now handle various error codes returned by
    +    block_writer_add().
     
         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
     
    @@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)
      	arg->err = writer_flush_block(arg->w);
      	if (arg->err < 0)
      		goto done;
    +@@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)
    + 	if (arg->err < 0)
    + 		goto done;
    + 
    ++	/*
    ++	 * If this still fails then we may need to reset record's offset
    ++	 * length to reduce the data size to be written.
    ++	 */
    + 	arg->err = block_writer_add(arg->w->block_writer, &rec);
    + 	if (arg->err == 0)
    + 		goto done;
    + 
    ++	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
    ++		goto done;
    ++
    + 	rec.u.obj.offset_len = 0;
    + 	arg->err = block_writer_add(arg->w->block_writer, &rec);
    + 
-- 
2.34.1

[GSoC PATCH v5 1/3] reftable: propagate specific error codes in block_writer_add()

[Meet Soni]

Previously, functions block_writer_add() and related functions returned
-1 when the record did not fit, forcing the caller to assume that any
failure meant the entry was too big. Replace these generic -1 returns
with defined error codes.

This prepares the codebase for finer-grained error handling so that
callers can distinguish between a block-full condition and other errors.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/block.c  | 13 ++++++------
 reftable/block.h  |  2 +-
 reftable/record.c | 53 +++++++++++++++++++++--------------------------
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/reftable/block.c b/reftable/block.c
index b14a8f1259..0b8ebc3aa5 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (is_restart)
 		rlen++;
 	if (2 + 3 * rlen + n > w->block_size - w->next)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	if (is_restart) {
 		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
 					    w->restart_cap);
@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)
 	return bw->block[bw->header_off];
 }
 
-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
-   success. Returns REFTABLE_API_ERROR if attempting to write a record with
-   empty key. */
+/*
+ * Adds the reftable_record to the block. Returns 0 on success and
+ * appropriate error codes on failure.
+ */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 {
 	struct reftable_buf empty = REFTABLE_BUF_INIT;
@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
 	n = reftable_encode_key(&is_restart, out, last, w->scratch,
 				reftable_record_val_type(rec));
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
 
 	n = reftable_record_encode(rec, out, w->hash_size);
 	if (n < 0) {
-		err = -1;
+		err = n;
 		goto done;
 	}
 	string_view_consume(&out, n);
diff --git a/reftable/block.h b/reftable/block.h
index bef2b8a4c5..64732eba7d 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
 /* returns the block type (eg. 'r' for ref records. */
 uint8_t block_writer_type(struct block_writer *bw);
 
-/* appends the record, or -1 if it doesn't fit. */
+/* Attempts to append the record. Returns 0 on success or error code on failure. */
 int block_writer_add(struct block_writer *w, struct reftable_record *rec);
 
 /* appends the key restarts, and compress the block if necessary. */
diff --git a/reftable/record.c b/reftable/record.c
index 8919df8a4d..d9fba8ff38 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)
 	while (value >>= 7)
 		varint[--pos] = 0x80 | (--value & 0x7f);
 	if (dest->len < sizeof(varint) - pos)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest->buf, varint + pos, sizeof(varint) - pos);
 	return sizeof(varint) - pos;
 }
@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)
 	size_t l = strlen(str);
 	int n = put_var_int(&s, l);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 	if (s.len < l)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(s.buf, str, l);
 	string_view_consume(&s, l);
 
@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,
 	uint64_t suffix_len = key.len - prefix_len;
 	int n = put_var_int(&dest, prefix_len);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	*restart = (prefix_len == 0);
 
 	n = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&dest, n);
 
 	if (dest.len < suffix_len)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 	memcpy(dest.buf, key.buf + prefix_len, suffix_len);
 	string_view_consume(&dest, suffix_len);
 
@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
 	struct string_view start = s;
 	int n = put_var_int(&s, r->update_index);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	switch (r->value_type) {
 	case REFTABLE_REF_SYMREF:
 		n = encode_string(r->value.symref, s);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		break;
 	case REFTABLE_REF_VAL2:
-		if (s.len < 2 * hash_size) {
-			return -1;
-		}
+		if (s.len < 2 * hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val2.value, hash_size);
 		string_view_consume(&s, hash_size);
 		memcpy(s.buf, r->value.val2.target_value, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
 	case REFTABLE_REF_VAL1:
-		if (s.len < hash_size) {
-			return -1;
-		}
+		if (s.len < hash_size)
+			return REFTABLE_ENTRY_TOO_BIG_ERROR;
 		memcpy(s.buf, r->value.val1, hash_size);
 		string_view_consume(&s, hash_size);
 		break;
@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
 	uint64_t last = 0;
 	if (r->offset_len == 0 || r->offset_len >= 8) {
 		n = put_var_int(&s, r->offset_len);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 	}
 	if (r->offset_len == 0)
 		return start.len - s.len;
 	n = put_var_int(&s, r->offsets[0]);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	last = r->offsets[0];
 	for (i = 1; i < r->offset_len; i++) {
 		int n = put_var_int(&s, r->offsets[i] - last);
-		if (n < 0) {
-			return -1;
-		}
+		if (n < 0)
+			return n;
 		string_view_consume(&s, n);
 		last = r->offsets[i];
 	}
@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 		return 0;
 
 	if (s.len < 2 * hash_size)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	memcpy(s.buf, r->value.update.old_hash, hash_size);
 	memcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);
@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 
 	n = encode_string(r->value.update.name ? r->value.update.name : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = encode_string(r->value.update.email ? r->value.update.email : "",
 			  s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	n = put_var_int(&s, r->value.update.time);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	if (s.len < 2)
-		return -1;
+		return REFTABLE_ENTRY_TOO_BIG_ERROR;
 
 	put_be16(s.buf, r->value.update.tz_offset);
 	string_view_consume(&s, 2);
@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
 	n = encode_string(
 		r->value.update.message ? r->value.update.message : "", s);
 	if (n < 0)
-		return -1;
+		return n;
 	string_view_consume(&s, n);
 
 	return start.len - s.len;
-- 
2.34.1

[GSoC PATCH v5 2/3] reftable: adapt writer_add_record() to propagate block_writer_add() errors

[Meet Soni]

Previously, writer_add_record() would flush the current block and retry
appending the record whenever block_writer_add() returned any nonzero
error. This forced an assumption that every failure meant the block was
full, even when errors such as memory allocation or I/O failures occurred.

Update the writer_add_record() to inspect the error code returned by
block_writer_add() and only flush and reinitialize the writer when the
error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
propagate it.

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/writer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index f3ab1035d6..94c97b7ac0 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,
 	 * done. Otherwise the block writer may have hit the block size limit
 	 * and needs to be flushed.
 	 */
-	if (!block_writer_add(w->block_writer, rec)) {
-		err = 0;
+	err = block_writer_add(w->block_writer, rec);
+	if (err == 0)
 		goto done;
-	}
 
+	if (err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
 	/*
 	 * The current block is full, so we need to flush and reinitialize the
 	 * writer to start writing the next block.
@@ -329,16 +330,10 @@ static int writer_add_record(struct reftable_writer *w,
 	/*
 	 * Try to add the record to the writer again. If this still fails then
 	 * the record does not fit into the block size.
-	 *
-	 * TODO: it would be great to have `block_writer_add()` return proper
-	 *       error codes so that we don't have to second-guess the failure
-	 *       mode here.
 	 */
 	err = block_writer_add(w->block_writer, rec);
-	if (err) {
-		err = REFTABLE_ENTRY_TOO_BIG_ERROR;
+	if (err)
 		goto done;
-	}
 
 done:
 	return err;
-- 
2.34.1

[GSoC PATCH v5 3/3] reftable: adapt write_object_record() to propagate block_writer_add() errors

[Meet Soni]

Previously, write_object_record() would flush the current block and retry
appending the record whenever block_writer_add() returned any nonzero
error. This forced an assumption that every failure meant the block was
full, even when errors such as memory allocation or I/O failures occurred.

Update the write_object_record() to inspect the error code returned by
block_writer_add() and flush and reinitialize the writer iff the
error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately
propagate it.

If the flush and reinitialization still fail with
REFTABLE_ENTRY_TOO_BIG_ERROR, reset the record's offset length to zero
before a final attempt.

All call sites now handle various error codes returned by
block_writer_add().

Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
 reftable/writer.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/reftable/writer.c b/reftable/writer.c
index 94c97b7ac0..f48e7cc290 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -620,10 +620,22 @@ static void write_object_record(void *void_arg, void *key)
 	if (arg->err < 0)
 		goto done;
 
+	/*
+	 * Try to add the record to the writer. If this succeeds then we're
+	 * done. Otherwise the block writer may have hit the block size limit
+	 * and needs to be flushed.
+	 */
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
 
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
+
+	/*
+	 * The current block is full, so we need to flush and reinitialize the
+	 * writer to start writing the next block.
+	 */
 	arg->err = writer_flush_block(arg->w);
 	if (arg->err < 0)
 		goto done;
@@ -632,10 +644,17 @@ static void write_object_record(void *void_arg, void *key)
 	if (arg->err < 0)
 		goto done;
 
+	/*
+	 * If this still fails then we may need to reset record's offset
+	 * length to reduce the data size to be written.
+	 */
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
 
+	if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
+		goto done;
+
 	rec.u.obj.offset_len = 0;
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 
-- 
2.34.1

Re: [GSoC PATCH v5 0/3] reftable: return proper error codes from block_writer_add

[Patrick Steinhardt]

On Wed, Mar 19, 2025 at 08:59:24PM +0530, Meet Soni wrote:
> This patch series attempts to avoid making an assumption regarding error codes
> returned by block_writer_add().
> 
> Changes since v4:
>     - update commit message.
>     - add documentation comment.

One additional change that isn't mentioned here is that we now check for
REFTABLE_ENTRY_TOO_BIG_ERROR the second time we call
`block_writer_add()` when writing object records, which is what my only
concern was. So with that now addressed I'm happy with this patch
series, thanks for working on it!

Patrick