{"thread":{"id":"63069","subject":"[GSoC PATCH] reftable: return proper error code from block_writer_add()","startedAt":"2025-03-06T12:13:32Z","lastAt":"2025-03-19T15:49:02Z","messageCount":20,"participants":["Meet Soni","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"513652","messageId":"20250306121324.1315290-1-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":null,"subject":"[GSoC PATCH] reftable: return proper error code from block_writer_add()","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-06T12:13:24Z","receivedAt":"2025-03-06T12:13:32Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, block_writer_add() used to return generic -1, which forced\nan assumption about the error type.\n\nReplace generic -1 returns in block_writer_add() and related functions\nwith defined error codes.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\nThis patch attempts to avoid making an assumption regarding error codes\nreturned by block_writer_add().\n reftable/block.c  |  9 +++++----\n reftable/record.c | 16 +++++++++++-----\n reftable/writer.c |  8 +-------\n 3 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex b14a8f1259..50fbac801a 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (is_restart)\n \t\trlen++;\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tif (is_restart) {\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n \t\t\t\t\t    w->restart_cap);\n@@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tint err;\n \n \terr = reftable_record_key(rec, &w->scratch);\n-\tif (err < 0)\n+\tif (err < 0) {\n \t\tgoto done;\n+\t}\n \n \tif (!w->scratch.len) {\n \t\terr = REFTABLE_API_ERROR;\n@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\n \n \tn = reftable_record_encode(rec, out, w->hash_size);\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8919df8a4d..5523804a0c 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \tuint64_t suffix_len = key.len - prefix_len;\n \tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tstring_view_consume(&dest, n);\n \n \t*restart = (prefix_len == 0);\n \n \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tstring_view_consume(&dest, n);\n \n \tif (dest.len < suffix_len)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n \tstring_view_consume(&dest, suffix_len);\n \n@@ -1144,14 +1144,20 @@ static struct reftable_record_vtable reftable_index_record_vtable = {\n \n int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)\n {\n-\treturn reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);\n+\tint key_len = reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);\n+\tif (key_len < 0)\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\treturn key_len;\n }\n \n int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n \t\t\t   uint32_t hash_size)\n {\n-\treturn reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n+\tint encode_len = reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n \t\t\t\t\t\t   dest, hash_size);\n+\tif (encode_len < 0)\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\treturn encode_len;\n }\n \n int reftable_record_copy_from(struct reftable_record *rec,\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d6..600ba5441b 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -327,16 +327,10 @@ static int writer_add_record(struct reftable_writer *w,\n \t\tgoto done;\n \n \t/*\n-\t * Try to add the record to the writer again. If this still fails then\n-\t * the record does not fit into the block size.\n-\t *\n-\t * TODO: it would be great to have `block_writer_add()` return proper\n-\t *       error codes so that we don't have to second-guess the failure\n-\t *       mode here.\n+\t * Try to add the record to the writer again.\n \t */\n \terr = block_writer_add(w->block_writer, rec);\n \tif (err) {\n-\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tgoto done;\n \t}\n \n\nbase-commit: e969bc875963a10890d61ba84eab3a460bd9e535\n-- \n2.34.1\n\n"},{"id":"513671","messageId":"Z8m0rnTr2PrqBUKQ@pks.im","threadId":"63069","inReplyTo":"20250306121324.1315290-1-meetsoni3017@gmail.com","subject":"Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-06T14:43:58Z","receivedAt":"2025-03-06T14:44:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 06, 2025 at 05:43:24PM +0530, Meet Soni wrote:\n> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n>  \tint err;\n>  \n>  \terr = reftable_record_key(rec, &w->scratch);\n> -\tif (err < 0)\n> +\tif (err < 0) {\n>  \t\tgoto done;\n> +\t}\n>  \n>  \tif (!w->scratch.len) {\n>  \t\terr = REFTABLE_API_ERROR;\n\nThis change probably shouldn't be here. Our style guide mentions that we\nprefer to not have curly braces around single-line statements.\n\n> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n>  \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n>  \t\t\t\treftable_record_val_type(rec));\n>  \tif (n < 0) {\n> -\t\terr = -1;\n> +\t\terr = n;\n>  \t\tgoto done;\n>  \t}\n>  \tstring_view_consume(&out, n);\n>  \n>  \tn = reftable_record_encode(rec, out, w->hash_size);\n>  \tif (n < 0) {\n> -\t\terr = -1;\n> +\t\terr = n;\n>  \t\tgoto done;\n>  \t}\n>  \tstring_view_consume(&out, n);\n\nOkay. `reftable_encode_key()` right now only knows to return generic\nerrors, but you fix that further down, and you also adapt\n`reftable_record_encode()`.\n\n> diff --git a/reftable/record.c b/reftable/record.c\n> index 8919df8a4d..5523804a0c 100644\n> --- a/reftable/record.c\n> +++ b/reftable/record.c\n> @@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n>  \tuint64_t suffix_len = key.len - prefix_len;\n>  \tint n = put_var_int(&dest, prefix_len);\n>  \tif (n < 0)\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n>  \tstring_view_consume(&dest, n);\n>  \n>  \t*restart = (prefix_len == 0);\n>  \n>  \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n>  \tif (n < 0)\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n>  \tstring_view_consume(&dest, n);\n>  \n>  \tif (dest.len < suffix_len)\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n>  \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n>  \tstring_view_consume(&dest, suffix_len);\n>  \n\nMakes sense.\n\n> @@ -1144,14 +1144,20 @@ static struct reftable_record_vtable reftable_index_record_vtable = {\n>  \n>  int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)\n>  {\n> -\treturn reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);\n> +\tint key_len = reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);\n> +\tif (key_len < 0)\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n> +\treturn key_len;\n>  }\n\nThis here is incorrect. We don't know why the `->key()` function has\nfailed, so we shouldn't assume `TOO_BIG_ERROR`. We'd have to vet all the\nimplementations of that function and then should bubble up their\nrespective error codes.\n\n>  int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n>  \t\t\t   uint32_t hash_size)\n>  {\n> -\treturn reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n> +\tint encode_len = reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n>  \t\t\t\t\t\t   dest, hash_size);\n> +\tif (encode_len < 0)\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n> +\treturn encode_len;\n>  }\n>  \n>  int reftable_record_copy_from(struct reftable_record *rec,\n\nSame remark here.\n\nThanks!\n\nPatrick\n"},{"id":"513718","messageId":"xmqqsenqhz0e.fsf@gitster.g","threadId":"63069","inReplyTo":"20250306121324.1315290-1-meetsoni3017@gmail.com","subject":"Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-06T17:04:01Z","receivedAt":"2025-03-06T17:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Meet Soni <meetsoni3017@gmail.com> writes:\n\n> Previously, block_writer_add() used to return generic -1, which forced\n> an assumption about the error type.\n>\n> Replace generic -1 returns in block_writer_add() and related functions\n> with defined error codes.\n\nWhat's missing from this proposed log message is an audit of the\ncallers to tell readers that this change is safe and expected by the\ncallers.  IOW, are there callers that start to behave differently\nwhen they see ENTRY_TOO_BIG instead of -1, for example?\n\n> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>\n> ---\n> This patch attempts to avoid making an assumption regarding error codes\n> returned by block_writer_add().\n>  reftable/block.c  |  9 +++++----\n>  reftable/record.c | 16 +++++++++++-----\n>  reftable/writer.c |  8 +-------\n>  3 files changed, 17 insertions(+), 16 deletions(-)\n>\n> diff --git a/reftable/block.c b/reftable/block.c\n> index b14a8f1259..50fbac801a 100644\n> --- a/reftable/block.c\n> +++ b/reftable/block.c\n> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n>  \tif (is_restart)\n>  \t\trlen++;\n>  \tif (2 + 3 * rlen + n > w->block_size - w->next)\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n\nSo this makes block_writer_register_restart() to return -11 instead\nof -1; the sole caller of the function is block_writer_add() that\nbegins like so:\n\n        /* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n           success. Returns REFTABLE_API_ERROR if attempting to write a record with\n           empty key. */\n        int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n        {\n\nNeedless to say, the comment before the function needs to be\nadjusted together with the above hunk (and others).  But more\nimportantly, are existing callers of this function now expected to\nadjust to the change in behaviour when they receive the return value\nof this function?  It used to be sufficient for them to deal with\n-1, 0 or API_ERROR, but now they are required to handle other errors\n(like the one that comes back from reftable_encode_key().  Do they\nalready handle these new error codes just fine?  Have you traced the\ncode paths to see how they react to them?\n\n> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n>  \tint err;\n>  \n>  \terr = reftable_record_key(rec, &w->scratch);\n> -\tif (err < 0)\n> +\tif (err < 0) {\n>  \t\tgoto done;\n> +\t}\n\nThis is unwarranted, isn't it?\n\n> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n>  \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n>  \t\t\t\treftable_record_val_type(rec));\n>  \tif (n < 0) {\n> -\t\terr = -1;\n> +\t\terr = n;\n>  \t\tgoto done;\n>  \t}\n\nHere, block_writer_add() starts returning new error codes to its\ncallers that it never returned.\n\n>  \tstring_view_consume(&out, n);\n>  \n>  \tn = reftable_record_encode(rec, out, w->hash_size);\n>  \tif (n < 0) {\n> -\t\terr = -1;\n> +\t\terr = n;\n>  \t\tgoto done;\n>  \t}\n\nDitto.\n\nNote that I am not saying that it is a bad idea to make the error\ncodes more specific so that the callers can tell them apart.  I am\nonly saying that the patch that makes such a change must also make\nsure that the callers are prepared to handle error coes that they\nhave never seen from the current callee.\n\nThe same applies to the remainder of the patch.\n\nThanks.\n"},{"id":"513829","messageId":"20250308133349.1591331-1-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250306121324.1315290-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v2] reftable: return proper error code from block_writer_add()","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-08T13:33:49Z","receivedAt":"2025-03-08T13:33:57Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, block_writer_add() used to return generic -1, which forced\nan assumption about the error type. Replace these generic -1 returns in\nblock_writer_add() and related functions with defined error codes.\n\nReviewed all call sites to ensure they check for nonzero error returns\nrather than strictly -1, confirming that this change is safe.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\nThis patch attempts to avoid making an assumption regarding error codes\nreturned by block_writer_add().\n\nChanges since v1:\n  - Update commit message to specify safe usage of the patch.\n  - Update function doc comment.\n  - Propagate errors to `->key()` and `->encode()` functions instead of \n    making any assumptions.\n \nDropped Han-Wen Nienhuys <hanwen@google.com> from CC, as the email doesn't exist.\nRange-diff against v1:\n1:  10d8bbeebc < -:  ---------- reftable: return proper error code from block_writer_add()\n-:  ---------- > 1:  7cdc7ce0ce reftable: return proper error code from block_writer_add()\n\n reftable/block.c  | 12 +++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n reftable/writer.c | 11 ++--------\n 4 files changed, 33 insertions(+), 45 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex b14a8f1259..89ab8bbc57 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (is_restart)\n \t\trlen++;\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tif (is_restart) {\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n \t\t\t\t\t    w->restart_cap);\n@@ -97,9 +97,9 @@ uint8_t block_writer_type(struct block_writer *bw)\n \treturn bw->block[bw->header_off];\n }\n \n-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n-   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n-   empty key. */\n+/* Adds the reftable_record to the block. Returns 0 on success and\n+ * appropriate error codes on failure.\n+ */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n {\n \tstruct reftable_buf empty = REFTABLE_BUF_INIT;\n@@ -126,14 +126,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\n \n \tn = reftable_record_encode(rec, out, w->hash_size);\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\ndiff --git a/reftable/block.h b/reftable/block.h\nindex bef2b8a4c5..0e7c680cf6 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n \n-/* appends the record, or -1 if it doesn't fit. */\n+/* attempts to append the record. returns 0 on success or error code on failure. */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n \n /* appends the key restarts, and compress the block if necessary. */\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8919df8a4d..d9fba8ff38 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)\n \twhile (value >>= 7)\n \t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n \tif (dest->len < sizeof(varint) - pos)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n \treturn sizeof(varint) - pos;\n }\n@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)\n \tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \tif (s.len < l)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(s.buf, str, l);\n \tstring_view_consume(&s, l);\n \n@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \tuint64_t suffix_len = key.len - prefix_len;\n \tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \t*restart = (prefix_len == 0);\n \n \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \tif (dest.len < suffix_len)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n \tstring_view_consume(&dest, suffix_len);\n \n@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tswitch (r->value_type) {\n \tcase REFTABLE_REF_SYMREF:\n \t\tn = encode_string(r->value.symref, s);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL2:\n-\t\tif (s.len < 2 * hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < 2 * hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val2.value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tmemcpy(s.buf, r->value.val2.target_value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL1:\n-\t\tif (s.len < hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val1, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \tuint64_t last = 0;\n \tif (r->offset_len == 0 || r->offset_len >= 8) {\n \t\tn = put_var_int(&s, r->offset_len);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t}\n \tif (r->offset_len == 0)\n \t\treturn start.len - s.len;\n \tn = put_var_int(&s, r->offsets[0]);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tlast = r->offsets[0];\n \tfor (i = 1; i < r->offset_len; i++) {\n \t\tint n = put_var_int(&s, r->offsets[i] - last);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tlast = r->offsets[i];\n \t}\n@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \t\treturn 0;\n \n \tif (s.len < 2 * hash_size)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tmemcpy(s.buf, r->value.update.old_hash, hash_size);\n \tmemcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);\n@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n \tn = encode_string(r->value.update.name ? r->value.update.name : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = encode_string(r->value.update.email ? r->value.update.email : \"\",\n \t\t\t  s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = put_var_int(&s, r->value.update.time);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tif (s.len < 2)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tput_be16(s.buf, r->value.update.tz_offset);\n \tstring_view_consume(&s, 2);\n@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \tn = encode_string(\n \t\tr->value.update.message ? r->value.update.message : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \treturn start.len - s.len;\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d6..5cb9d0bf85 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -327,18 +327,11 @@ static int writer_add_record(struct reftable_writer *w,\n \t\tgoto done;\n \n \t/*\n-\t * Try to add the record to the writer again. If this still fails then\n-\t * the record does not fit into the block size.\n-\t *\n-\t * TODO: it would be great to have `block_writer_add()` return proper\n-\t *       error codes so that we don't have to second-guess the failure\n-\t *       mode here.\n+\t * Try to add the record to the writer again.\n \t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err) {\n-\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\tif (err)\n \t\tgoto done;\n-\t}\n \n done:\n \treturn err;\n-- \n2.34.1\n\n"},{"id":"514034","messageId":"Z9FEbH48tQ9KxzQV@pks.im","threadId":"63069","inReplyTo":"20250308133349.1591331-1-meetsoni3017@gmail.com","subject":"Re: [GSoC PATCH v2] reftable: return proper error code from block_writer_add()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-12T08:23:08Z","receivedAt":"2025-03-12T08:23:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Mar 08, 2025 at 07:03:49PM +0530, Meet Soni wrote:\n> diff --git a/reftable/block.c b/reftable/block.c\n> index b14a8f1259..89ab8bbc57 100644\n> --- a/reftable/block.c\n> +++ b/reftable/block.c\n> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n>  \tif (is_restart)\n>  \t\trlen++;\n>  \tif (2 + 3 * rlen + n > w->block_size - w->next)\n> -\t\treturn -1;\n> +\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n>  \tif (is_restart) {\n>  \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n>  \t\t\t\t\t    w->restart_cap);\n> @@ -97,9 +97,9 @@ uint8_t block_writer_type(struct block_writer *bw)\n>  \treturn bw->block[bw->header_off];\n>  }\n>  \n> -/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n> -   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n> -   empty key. */\n> +/* Adds the reftable_record to the block. Returns 0 on success and\n> + * appropriate error codes on failure.\n> + */\n>  int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n>  {\n>  \tstruct reftable_buf empty = REFTABLE_BUF_INIT;\n\nI'm in favor of touching up the comment's formatting while at it, but if\nwe do so we should use the correct style, which has the opening and\nclosing parts on their own line:\n\n    /*\n     * Yadda yadda.\n     */\n\n> diff --git a/reftable/block.h b/reftable/block.h\n> index bef2b8a4c5..0e7c680cf6 100644\n> --- a/reftable/block.h\n> +++ b/reftable/block.h\n> @@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n>  /* returns the block type (eg. 'r' for ref records. */\n>  uint8_t block_writer_type(struct block_writer *bw);\n>  \n> -/* appends the record, or -1 if it doesn't fit. */\n> +/* attempts to append the record. returns 0 on success or error code on failure. */\n>  int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n>  \n>  /* appends the key restarts, and compress the block if necessary. */\n\nWe might also touch up this comment to start with an upper-case \"A\"\nwhile at it.\n\n> diff --git a/reftable/record.c b/reftable/record.c\n> index 8919df8a4d..d9fba8ff38 100644\n> --- a/reftable/record.c\n> +++ b/reftable/record.c\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index f3ab1035d6..5cb9d0bf85 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -327,18 +327,11 @@ static int writer_add_record(struct reftable_writer *w,\n>  \t\tgoto done;\n>  \n>  \t/*\n> -\t * Try to add the record to the writer again. If this still fails then\n> -\t * the record does not fit into the block size.\n> -\t * TODO: it would be great to have `block_writer_add()` return proper\n> -\t *       error codes so that we don't have to second-guess the failure\n> -\t *       mode here.\n> +\t * Try to add the record to the writer again.\n>  \t */\n>  \terr = block_writer_add(w->block_writer, rec);\n> -\tif (err) {\n> -\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n> +\tif (err)\n>  \t\tgoto done;\n> -\t}\n\nLet's not drop the second sentence of the comment, as it is important to\ngive context. Also, let's take a step back here and figure out what this\nfunction is doing:\n\n  1. We compute the record data and try to append it to the current\n     block. If this succeeds, we can return immediately and are done.\n\n  2. If appending to the current block fails we assume that we have\n     failed because the block is full. This is because reftable blocks\n     have a specific maximum length that we cannot exceed. We thus\n     flush the block and start writing a new one.\n\n  3. We now try to add the same record to the new block again and hope\n     that we can now write the record successfully.\n\nThe important part that the TODO comment refers to is in (2), indicated\nby \"assume\": we don't actually check what the error is that we've got\nfrom `block_writer_add()`, but simply pretend as if it was\n`REFTABLE_ENTRY_TOO_BIG_ERROR`. This means that we'd even re-try writing\nthe record in case we had for example a memory allocation failure, or an\nI/O error, and that is plain wrong.\n\nWith your changes we have now started to plumb proper errors through from\n`block_writer_add()`. But that doesn't mean we can just drop the comment\nand bubble up the error. Instead, we should also be adapting the code in\n(2) to do the right thing: we shouldn't _assume_ that the current block\nis full, but instead check the error code returned by the first call to\n`block_writer_add()`:\n\n  - If it is `REFTABLE_ENTRY_TOO_BIG_ERROR` we indeed should flush the\n    current block and try to write a new one.\n\n  - Otherwise we bail out and bubble up the error.\n\nAnd once we do that, it is fine to remove the comment indeed. It's\nsomewhat funny because from my point of view the comment is in the wrong\nspot: it does correctly point out that _this_ particular callsite is\ndoing the wrong thing, but it didn't mention that the other callsite\nalso has the same problem. And that other callsite is more important\nfrom my perspective.\n\nSo I'd recommend to split up this commit into two commits:\n\n  - The first commit prepares all transitively called functions as you\n    already do.\n\n  - The second commit adapts \"reftable/writer.c\" and fixes both\n    callsites of `block_writer_add()` to do proper error handling.\n\nPatrick\n"},{"id":"514041","messageId":"20250312121148.1879604-1-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250308133349.1591331-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v3 0/2] reftable: return proper error codes from block_writer_add","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-12T12:11:46Z","receivedAt":"2025-03-12T12:12:09Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"This patch attempts to avoid making an assumption regarding error codes\nreturned by block_writer_add().\n\nChanges since v2:\n    - Split the commit into two to separate transitively called function\n      updates from writer call-site adaptations\n    - Made formatting improvements in comments and code for better \n      readability.\n    - Modified the writer logic to flush and retry only when a specific\n      error occurs, while other errors are propagated as-is.\n\nMeet Soni (2):\n  reftable: propagate specific error codes in block_writer_add()\n  reftable: adapt writer code to propagate block_writer_add() errors\n\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n reftable/writer.c | 30 ++++++++++++++++-----------\n 4 files changed, 50 insertions(+), 48 deletions(-)\n\nRange-diff against v2:\n1:  7cdc7ce0ce ! 1:  6ab35d569c reftable: return proper error code from block_writer_add()\n    @@ Metadata\n     Author: Meet Soni <meetsoni3017@gmail.com>\n     \n      ## Commit message ##\n    -    reftable: return proper error code from block_writer_add()\n    +    reftable: propagate specific error codes in block_writer_add()\n     \n    -    Previously, block_writer_add() used to return generic -1, which forced\n    -    an assumption about the error type. Replace these generic -1 returns in\n    -    block_writer_add() and related functions with defined error codes.\n    +    Previously, functions block_writer_add() and related functions returned\n    +    -1 when the record did not fit, forcing the caller to assume that any\n    +    failure meant the entry was too big. Replace these generic -1 returns\n    +    with defined error codes.\n     \n    -    Reviewed all call sites to ensure they check for nonzero error returns\n    -    rather than strictly -1, confirming that this change is safe.\n    +    This prepares the codebase for finer-grained error handling so that\n    +    callers can distinguish between a block-full condition and other errors.\n     \n         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>\n     \n    @@ reftable/block.c: uint8_t block_writer_type(struct block_writer *bw)\n     -/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n     -   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n     -   empty key. */\n    -+/* Adds the reftable_record to the block. Returns 0 on success and\n    ++/*\n    ++ * Adds the reftable_record to the block. Returns 0 on success and\n     + * appropriate error codes on failure.\n     + */\n      int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n    @@ reftable/block.h: int block_writer_init(struct block_writer *bw, uint8_t typ, ui\n      uint8_t block_writer_type(struct block_writer *bw);\n      \n     -/* appends the record, or -1 if it doesn't fit. */\n    -+/* attempts to append the record. returns 0 on success or error code on failure. */\n    ++/* Attempts to append the record. Returns 0 on success or error code on failure. */\n      int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n      \n      /* appends the key restarts, and compress the block if necessary. */\n    @@ reftable/record.c: static int reftable_log_record_encode(const void *rec, struct\n      \tstring_view_consume(&s, n);\n      \n      \treturn start.len - s.len;\n    -\n    - ## reftable/writer.c ##\n    -@@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,\n    - \t\tgoto done;\n    - \n    - \t/*\n    --\t * Try to add the record to the writer again. If this still fails then\n    --\t * the record does not fit into the block size.\n    --\t *\n    --\t * TODO: it would be great to have `block_writer_add()` return proper\n    --\t *       error codes so that we don't have to second-guess the failure\n    --\t *       mode here.\n    -+\t * Try to add the record to the writer again.\n    - \t */\n    - \terr = block_writer_add(w->block_writer, rec);\n    --\tif (err) {\n    --\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n    -+\tif (err)\n    - \t\tgoto done;\n    --\t}\n    - \n    - done:\n    - \treturn err;\n-:  ---------- > 2:  a54d440dd3 reftable: adapt writer code to propagate block_writer_add() errors\n-- \n2.34.1\n\n"},{"id":"514042","messageId":"20250312121148.1879604-2-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250312121148.1879604-1-meetsoni3017@gmail.com","subject":"[PATCH v3 1/2] reftable: propagate specific error codes in block_writer_add()","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-12T12:11:47Z","receivedAt":"2025-03-12T12:12:23Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, functions block_writer_add() and related functions returned\n-1 when the record did not fit, forcing the caller to assume that any\nfailure meant the entry was too big. Replace these generic -1 returns\nwith defined error codes.\n\nThis prepares the codebase for finer-grained error handling so that\ncallers can distinguish between a block-full condition and other errors.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n 3 files changed, 32 insertions(+), 36 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex b14a8f1259..0b8ebc3aa5 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (is_restart)\n \t\trlen++;\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tif (is_restart) {\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n \t\t\t\t\t    w->restart_cap);\n@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)\n \treturn bw->block[bw->header_off];\n }\n \n-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n-   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n-   empty key. */\n+/*\n+ * Adds the reftable_record to the block. Returns 0 on success and\n+ * appropriate error codes on failure.\n+ */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n {\n \tstruct reftable_buf empty = REFTABLE_BUF_INIT;\n@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\n \n \tn = reftable_record_encode(rec, out, w->hash_size);\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\ndiff --git a/reftable/block.h b/reftable/block.h\nindex bef2b8a4c5..64732eba7d 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n \n-/* appends the record, or -1 if it doesn't fit. */\n+/* Attempts to append the record. Returns 0 on success or error code on failure. */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n \n /* appends the key restarts, and compress the block if necessary. */\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8919df8a4d..d9fba8ff38 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)\n \twhile (value >>= 7)\n \t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n \tif (dest->len < sizeof(varint) - pos)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n \treturn sizeof(varint) - pos;\n }\n@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)\n \tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \tif (s.len < l)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(s.buf, str, l);\n \tstring_view_consume(&s, l);\n \n@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \tuint64_t suffix_len = key.len - prefix_len;\n \tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \t*restart = (prefix_len == 0);\n \n \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \tif (dest.len < suffix_len)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n \tstring_view_consume(&dest, suffix_len);\n \n@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tswitch (r->value_type) {\n \tcase REFTABLE_REF_SYMREF:\n \t\tn = encode_string(r->value.symref, s);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL2:\n-\t\tif (s.len < 2 * hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < 2 * hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val2.value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tmemcpy(s.buf, r->value.val2.target_value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL1:\n-\t\tif (s.len < hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val1, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \tuint64_t last = 0;\n \tif (r->offset_len == 0 || r->offset_len >= 8) {\n \t\tn = put_var_int(&s, r->offset_len);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t}\n \tif (r->offset_len == 0)\n \t\treturn start.len - s.len;\n \tn = put_var_int(&s, r->offsets[0]);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tlast = r->offsets[0];\n \tfor (i = 1; i < r->offset_len; i++) {\n \t\tint n = put_var_int(&s, r->offsets[i] - last);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tlast = r->offsets[i];\n \t}\n@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \t\treturn 0;\n \n \tif (s.len < 2 * hash_size)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tmemcpy(s.buf, r->value.update.old_hash, hash_size);\n \tmemcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);\n@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n \tn = encode_string(r->value.update.name ? r->value.update.name : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = encode_string(r->value.update.email ? r->value.update.email : \"\",\n \t\t\t  s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = put_var_int(&s, r->value.update.time);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tif (s.len < 2)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tput_be16(s.buf, r->value.update.tz_offset);\n \tstring_view_consume(&s, 2);\n@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \tn = encode_string(\n \t\tr->value.update.message ? r->value.update.message : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \treturn start.len - s.len;\n-- \n2.34.1\n\n"},{"id":"514043","messageId":"20250312121148.1879604-3-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250312121148.1879604-1-meetsoni3017@gmail.com","subject":"[PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-12T12:11:48Z","receivedAt":"2025-03-12T12:12:36Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, writer_add_record() and write_object_record() would flush the\ncurrent block and retry appending the record whenever block_writer_add()\nreturned any nonzero error. This forced an assumption that every failure\nmeant the block was full, even when errors such as memory allocation or\nI/O failures occurred.\n\nUpdate the writer code to inspect the error code returned by\nblock_writer_add() and only flush and reinitialize the writer when the\nerror is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\npropagate it.\n\nAll call sites now handle various error codes returned by\nblock_writer_add().\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/writer.c | 30 ++++++++++++++++++------------\n 1 file changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d6..0d8181e227 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,\n \t * done. Otherwise the block writer may have hit the block size limit\n \t * and needs to be flushed.\n \t */\n-\tif (!block_writer_add(w->block_writer, rec)) {\n-\t\terr = 0;\n+\terr = block_writer_add(w->block_writer, rec);\n+\tif (err == 0)\n \t\tgoto done;\n-\t}\n \n+\tif (err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n \t/*\n \t * The current block is full, so we need to flush and reinitialize the\n \t * writer to start writing the next block.\n@@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w,\n \t\tgoto done;\n \n \t/*\n-\t * Try to add the record to the writer again. If this still fails then\n-\t * the record does not fit into the block size.\n-\t *\n-\t * TODO: it would be great to have `block_writer_add()` return proper\n-\t *       error codes so that we don't have to second-guess the failure\n-\t *       mode here.\n+\t * Try to add the record to the writer again.\n \t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err) {\n-\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\tif (err)\n \t\tgoto done;\n-\t}\n \n done:\n \treturn err;\n@@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key)\n \tif (arg->err < 0)\n \t\tgoto done;\n \n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n \n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n+\n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \targ->err = writer_flush_block(arg->w);\n \tif (arg->err < 0)\n \t\tgoto done;\n-- \n2.34.1\n\n"},{"id":"514047","messageId":"Z9GC400L-XV3SFyj@pks.im","threadId":"63069","inReplyTo":"20250312121148.1879604-3-meetsoni3017@gmail.com","subject":"Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-12T12:49:39Z","receivedAt":"2025-03-12T12:49:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 12, 2025 at 05:41:48PM +0530, Meet Soni wrote:\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index f3ab1035d6..0d8181e227 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,\n>  \t * done. Otherwise the block writer may have hit the block size limit\n>  \t * and needs to be flushed.\n>  \t */\n> -\tif (!block_writer_add(w->block_writer, rec)) {\n> -\t\terr = 0;\n> +\terr = block_writer_add(w->block_writer, rec);\n> +\tif (err == 0)\n>  \t\tgoto done;\n> -\t}\n\nStyle: we'd typically say `if (!err)` here, even though I see that we\nhave explicit comparisons with 0 elsewhere in this file, too. So I guess\nultimately this is okay.\n\n> @@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w,\n>  \t\tgoto done;\n>  \n>  \t/*\n> -\t * Try to add the record to the writer again. If this still fails then\n> -\t * the record does not fit into the block size.\n> -\t *\n> -\t * TODO: it would be great to have `block_writer_add()` return proper\n> -\t *       error codes so that we don't have to second-guess the failure\n> -\t *       mode here.\n> +\t * Try to add the record to the writer again.\n>  \t */\n\nMy comment on the preceding version still applies here: the second\nsentence (the one starting with \"If this still fails...\") should be\nretained.\n\n>  \terr = block_writer_add(w->block_writer, rec);\n> -\tif (err) {\n> -\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n> +\tif (err)\n>  \t\tgoto done;\n> -\t}\n>  \n>  done:\n>  \treturn err;\n> @@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key)\n>  \tif (arg->err < 0)\n>  \t\tgoto done;\n>  \n> +\t/*\n> +\t * Try to add the record to the writer. If this succeeds then we're\n> +\t * done. Otherwise the block writer may have hit the block size limit\n> +\t * and needs to be flushed.\n> +\t */\n>  \targ->err = block_writer_add(arg->w->block_writer, &rec);\n>  \tif (arg->err == 0)\n>  \t\tgoto done;\n>  \n> +\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n> +\t\tgoto done;\n\nGood catch that there is another such pattern!\n\n> +\t/*\n> +\t * The current block is full, so we need to flush and reinitialize the\n> +\t * writer to start writing the next block.\n> +\t */\n>  \targ->err = writer_flush_block(arg->w);\n>  \tif (arg->err < 0)\n>  \t\tgoto done;\n\nBut there is another case further down where we do `block_writer_add()`\nand then re-try in case the write fails. This one is a bit more curious:\nif the write fails, we don't create a new block -- after all we have\njust created one. Instead, we reset the record's offset length to zero\nbefore retrying.\n\nI _think_ that this is done because we know that when resetting the\noffset we would write less data to the block, as can be seen in\n`reftable_obj_record_encode()`. But I'm honestly not quite sure here as\nI haven't yet done a deep dive into object records -- after all, we\ndon't even really use them in Git.\n\nIn any case, I think that this callsite also needs adjustment and\nwarrants a comment. And if so, all changes to `write_object_record()`\nshould probably go into a separate commit, as well.\n\nThanks!\n\nPatrick\n"},{"id":"514199","messageId":"CAPhwyn3rAaFZ0UYniJWUswAWyyPkDNgvKSvRpV6_H9v__txVog@mail.gmail.com","threadId":"63069","inReplyTo":"Z9GC400L-XV3SFyj@pks.im","subject":"Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-13T15:29:51Z","receivedAt":"2025-03-13T15:30:05Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"On Wed, 12 Mar 2025 at 18:19, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> > +     /*\n> > +      * The current block is full, so we need to flush and reinitialize the\n> > +      * writer to start writing the next block.\n> > +      */\n> >       arg->err = writer_flush_block(arg->w);\n> >       if (arg->err < 0)\n> >               goto done;\n>\n> But there is another case further down where we do `block_writer_add()`\n> and then re-try in case the write fails. This one is a bit more curious:\n> if the write fails, we don't create a new block -- after all we have\n> just created one. Instead, we reset the record's offset length to zero\n> before retrying.\n>\n> I _think_ that this is done because we know that when resetting the\n> offset we would write less data to the block, as can be seen in\n> `reftable_obj_record_encode()`. But I'm honestly not quite sure here as\n> I haven't yet done a deep dive into object records -- after all, we\n> don't even really use them in Git.\n>\n> In any case, I think that this callsite also needs adjustment and\n> warrants a comment. And if so, all changes to `write_object_record()`\n> should probably go into a separate commit, as well.\n>\n\nRegarding the callsite in write_object_record() where we reset the\nrecord's offset length to zero before retrying: my changes currently\nfollow the same principle.\n\n    - If block_writer_add() returns an error other than\n      REFTABLE_ENTRY_TOO_BIG_ERROR, we simply return.\n\n    - For REFTABLE_ENTRY_TOO_BIG_ERROR, we flush the block and retry.\n\n    - If that fails, we reset the record's offset length to zero and\n      then retry.\n\nI'm not sure what adjustments or additional comments you are referring to.\nCould you please clarify what changes you expect at this callsite?\n\nThanks!\nMeet\n"},{"id":"514615","messageId":"20250319075943.28904-1-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250312121148.1879604-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v4 0/3] reftable: return proper error codes from block_writer_add","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T07:59:40Z","receivedAt":"2025-03-19T08:00:03Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"This patch attempts to avoid making an assumption regarding error codes\nreturned by block_writer_add().\n\nChanges since v3:\n    - split commit based on the functions it alters\n    - add comment back that was earlier removed.\n\nMeet Soni (3):\n  reftable: propagate specific error codes in block_writer_add()\n  reftable: adapt writer_add_record() to propagate block_writer_add()\n    errors\n  reftable: adapt write_object_record() to propagate block_writer_add()\n    errors\n\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n reftable/writer.c | 27 +++++++++++++++---------\n 4 files changed, 49 insertions(+), 46 deletions(-)\n\nRange-diff against v3:\n1:  6ab35d569c = 1:  6ab35d569c reftable: propagate specific error codes in block_writer_add()\n2:  a54d440dd3 ! 2:  7f0bdc27e1 reftable: adapt writer code to propagate block_writer_add() errors\n    @@ Metadata\n     Author: Meet Soni <meetsoni3017@gmail.com>\n     \n      ## Commit message ##\n    -    reftable: adapt writer code to propagate block_writer_add() errors\n    +    reftable: adapt writer_add_record() to propagate block_writer_add() errors\n     \n    -    Previously, writer_add_record() and write_object_record() would flush the\n    -    current block and retry appending the record whenever block_writer_add()\n    -    returned any nonzero error. This forced an assumption that every failure\n    -    meant the block was full, even when errors such as memory allocation or\n    -    I/O failures occurred.\n    +        Previously, writer_add_record() would flush the current block and\n    +        retry appending the record whenever block_writer_add() returned any\n    +        nonzero error. This forced an assumption that every failure meant\n    +        the block was full, even when errors such as memory allocation or I/O\n    +        failures occurred.\n     \n    -    Update the writer code to inspect the error code returned by\n    -    block_writer_add() and only flush and reinitialize the writer when the\n    -    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    -    propagate it.\n    -\n    -    All call sites now handle various error codes returned by\n    -    block_writer_add().\n    +        Update the writer_add_record() to inspect the error code returned by\n    +        block_writer_add() and only flush and reinitialize the writer when the\n    +        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    +        propagate it.\n     \n         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>\n     \n    @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,\n      \t * The current block is full, so we need to flush and reinitialize the\n      \t * writer to start writing the next block.\n     @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,\n    - \t\tgoto done;\n    - \n      \t/*\n    --\t * Try to add the record to the writer again. If this still fails then\n    --\t * the record does not fit into the block size.\n    + \t * Try to add the record to the writer again. If this still fails then\n    + \t * the record does not fit into the block size.\n     -\t *\n     -\t * TODO: it would be great to have `block_writer_add()` return proper\n     -\t *       error codes so that we don't have to second-guess the failure\n     -\t *       mode here.\n    -+\t * Try to add the record to the writer again.\n      \t */\n      \terr = block_writer_add(w->block_writer, rec);\n     -\tif (err) {\n    @@ reftable/writer.c: static int writer_add_record(struct reftable_writer *w,\n      \n      done:\n      \treturn err;\n    -@@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)\n    - \tif (arg->err < 0)\n    - \t\tgoto done;\n    - \n    -+\t/*\n    -+\t * Try to add the record to the writer. If this succeeds then we're\n    -+\t * done. Otherwise the block writer may have hit the block size limit\n    -+\t * and needs to be flushed.\n    -+\t */\n    - \targ->err = block_writer_add(arg->w->block_writer, &rec);\n    - \tif (arg->err == 0)\n    - \t\tgoto done;\n    - \n    -+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n    -+\t\tgoto done;\n    -+\n    -+\t/*\n    -+\t * The current block is full, so we need to flush and reinitialize the\n    -+\t * writer to start writing the next block.\n    -+\t */\n    - \targ->err = writer_flush_block(arg->w);\n    - \tif (arg->err < 0)\n    - \t\tgoto done;\n-:  ---------- > 3:  480ac27797 reftable: adapt write_object_record() to propagate block_writer_add() errors\n-- \n2.34.1\n\n"},{"id":"514616","messageId":"20250319075943.28904-2-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319075943.28904-1-meetsoni3017@gmail.com","subject":"[PATCH v4 1/3] reftable: propagate specific error codes in block_writer_add()","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T07:59:41Z","receivedAt":"2025-03-19T08:00:44Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, functions block_writer_add() and related functions returned\n-1 when the record did not fit, forcing the caller to assume that any\nfailure meant the entry was too big. Replace these generic -1 returns\nwith defined error codes.\n\nThis prepares the codebase for finer-grained error handling so that\ncallers can distinguish between a block-full condition and other errors.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n 3 files changed, 32 insertions(+), 36 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex b14a8f1259..0b8ebc3aa5 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (is_restart)\n \t\trlen++;\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tif (is_restart) {\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n \t\t\t\t\t    w->restart_cap);\n@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)\n \treturn bw->block[bw->header_off];\n }\n \n-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n-   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n-   empty key. */\n+/*\n+ * Adds the reftable_record to the block. Returns 0 on success and\n+ * appropriate error codes on failure.\n+ */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n {\n \tstruct reftable_buf empty = REFTABLE_BUF_INIT;\n@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\n \n \tn = reftable_record_encode(rec, out, w->hash_size);\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\ndiff --git a/reftable/block.h b/reftable/block.h\nindex bef2b8a4c5..64732eba7d 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n \n-/* appends the record, or -1 if it doesn't fit. */\n+/* Attempts to append the record. Returns 0 on success or error code on failure. */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n \n /* appends the key restarts, and compress the block if necessary. */\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8919df8a4d..d9fba8ff38 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)\n \twhile (value >>= 7)\n \t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n \tif (dest->len < sizeof(varint) - pos)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n \treturn sizeof(varint) - pos;\n }\n@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)\n \tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \tif (s.len < l)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(s.buf, str, l);\n \tstring_view_consume(&s, l);\n \n@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \tuint64_t suffix_len = key.len - prefix_len;\n \tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \t*restart = (prefix_len == 0);\n \n \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \tif (dest.len < suffix_len)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n \tstring_view_consume(&dest, suffix_len);\n \n@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tswitch (r->value_type) {\n \tcase REFTABLE_REF_SYMREF:\n \t\tn = encode_string(r->value.symref, s);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL2:\n-\t\tif (s.len < 2 * hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < 2 * hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val2.value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tmemcpy(s.buf, r->value.val2.target_value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL1:\n-\t\tif (s.len < hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val1, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \tuint64_t last = 0;\n \tif (r->offset_len == 0 || r->offset_len >= 8) {\n \t\tn = put_var_int(&s, r->offset_len);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t}\n \tif (r->offset_len == 0)\n \t\treturn start.len - s.len;\n \tn = put_var_int(&s, r->offsets[0]);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tlast = r->offsets[0];\n \tfor (i = 1; i < r->offset_len; i++) {\n \t\tint n = put_var_int(&s, r->offsets[i] - last);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tlast = r->offsets[i];\n \t}\n@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \t\treturn 0;\n \n \tif (s.len < 2 * hash_size)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tmemcpy(s.buf, r->value.update.old_hash, hash_size);\n \tmemcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);\n@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n \tn = encode_string(r->value.update.name ? r->value.update.name : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = encode_string(r->value.update.email ? r->value.update.email : \"\",\n \t\t\t  s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = put_var_int(&s, r->value.update.time);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tif (s.len < 2)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tput_be16(s.buf, r->value.update.tz_offset);\n \tstring_view_consume(&s, 2);\n@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \tn = encode_string(\n \t\tr->value.update.message ? r->value.update.message : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \treturn start.len - s.len;\n-- \n2.34.1\n\n"},{"id":"514617","messageId":"20250319075943.28904-3-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319075943.28904-1-meetsoni3017@gmail.com","subject":"[PATCH v4 2/3] reftable: adapt writer_add_record() to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T07:59:42Z","receivedAt":"2025-03-19T08:00:46Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"    Previously, writer_add_record() would flush the current block and\n    retry appending the record whenever block_writer_add() returned any\n    nonzero error. This forced an assumption that every failure meant\n    the block was full, even when errors such as memory allocation or I/O\n    failures occurred.\n\n    Update the writer_add_record() to inspect the error code returned by\n    block_writer_add() and only flush and reinitialize the writer when the\n    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    propagate it.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/writer.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d6..94c97b7ac0 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,\n \t * done. Otherwise the block writer may have hit the block size limit\n \t * and needs to be flushed.\n \t */\n-\tif (!block_writer_add(w->block_writer, rec)) {\n-\t\terr = 0;\n+\terr = block_writer_add(w->block_writer, rec);\n+\tif (err == 0)\n \t\tgoto done;\n-\t}\n \n+\tif (err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n \t/*\n \t * The current block is full, so we need to flush and reinitialize the\n \t * writer to start writing the next block.\n@@ -329,16 +330,10 @@ static int writer_add_record(struct reftable_writer *w,\n \t/*\n \t * Try to add the record to the writer again. If this still fails then\n \t * the record does not fit into the block size.\n-\t *\n-\t * TODO: it would be great to have `block_writer_add()` return proper\n-\t *       error codes so that we don't have to second-guess the failure\n-\t *       mode here.\n \t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err) {\n-\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\tif (err)\n \t\tgoto done;\n-\t}\n \n done:\n \treturn err;\n-- \n2.34.1\n\n"},{"id":"514618","messageId":"20250319075943.28904-4-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319075943.28904-1-meetsoni3017@gmail.com","subject":"[PATCH v4 3/3] reftable: adapt write_object_record() to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T07:59:43Z","receivedAt":"2025-03-19T08:00:49Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"    Previously, write_object_record() would flush the current block and\n    retry appending the record whenever block_writer_add() returned any\n    nonzero error. This forced an assumption that every failure meant the\n    block was full, even when errors such as memory allocation or I/O\n    failures occurred.\n\n    Update the write_object_record() to inspect the error code returned by\n    block_writer_add() and only flush and reinitialize the writer when the\n    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    propagate it.\n\n    All call sites now handle various error codes returned by\n    block_writer_add().\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/writer.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 94c97b7ac0..3fdfa4d34b 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -620,10 +620,22 @@ static void write_object_record(void *void_arg, void *key)\n \tif (arg->err < 0)\n \t\tgoto done;\n \n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n \n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n+\n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \targ->err = writer_flush_block(arg->w);\n \tif (arg->err < 0)\n \t\tgoto done;\n-- \n2.34.1\n\n"},{"id":"514636","messageId":"Z9rEFzVkj2O76B7m@pks.im","threadId":"63069","inReplyTo":"CAPhwyn3rAaFZ0UYniJWUswAWyyPkDNgvKSvRpV6_H9v__txVog@mail.gmail.com","subject":"Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-19T13:18:15Z","receivedAt":"2025-03-19T13:18:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 13, 2025 at 08:59:51PM +0530, Meet Soni wrote:\n> On Wed, 12 Mar 2025 at 18:19, Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > > +     /*\n> > > +      * The current block is full, so we need to flush and reinitialize the\n> > > +      * writer to start writing the next block.\n> > > +      */\n> > >       arg->err = writer_flush_block(arg->w);\n> > >       if (arg->err < 0)\n> > >               goto done;\n> >\n> > But there is another case further down where we do `block_writer_add()`\n> > and then re-try in case the write fails. This one is a bit more curious:\n> > if the write fails, we don't create a new block -- after all we have\n> > just created one. Instead, we reset the record's offset length to zero\n> > before retrying.\n> >\n> > I _think_ that this is done because we know that when resetting the\n> > offset we would write less data to the block, as can be seen in\n> > `reftable_obj_record_encode()`. But I'm honestly not quite sure here as\n> > I haven't yet done a deep dive into object records -- after all, we\n> > don't even really use them in Git.\n> >\n> > In any case, I think that this callsite also needs adjustment and\n> > warrants a comment. And if so, all changes to `write_object_record()`\n> > should probably go into a separate commit, as well.\n> >\n\nSorry for taking so long to respond.\n\n> Regarding the callsite in write_object_record() where we reset the\n> record's offset length to zero before retrying: my changes currently\n> follow the same principle.\n> \n>     - If block_writer_add() returns an error other than\n>       REFTABLE_ENTRY_TOO_BIG_ERROR, we simply return.\n> \n>     - For REFTABLE_ENTRY_TOO_BIG_ERROR, we flush the block and retry.\n> \n>     - If that fails, we reset the record's offset length to zero and\n>       then retry.\n\nIt's this last step that also needs to be adapted to check for\nREFTABLE_ENTRY_TOO_BIG_ERROR, because the intent here is the exact same:\nif writing the object record failed even in a completely new block then\nwe reset the object's offset and try a third time. But same as for the\nfirst time, we don't check whether we get REFTABLE_ENTRY_TOO_BIG_ERROR\nhere and thus we might be failing and retrying even in unintended cases.\n\n> I'm not sure what adjustments or additional comments you are referring to.\n> Could you please clarify what changes you expect at this callsite?\n\nSo overall we'd add the check to both callsites, like in the below\npatch.\n\nPatrick\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d61..63629e00a37 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -628,6 +628,8 @@ static void write_object_record(void *void_arg, void *key)\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n \n \targ->err = writer_flush_block(arg->w);\n \tif (arg->err < 0)\n@@ -640,6 +642,8 @@ static void write_object_record(void *void_arg, void *key)\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n \n \trec.u.obj.offset_len = 0;\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n\n"},{"id":"514647","messageId":"20250319152927.1263033-1-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319075943.28904-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v5 0/3] reftable: return proper error codes from block_writer_add","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T15:29:24Z","receivedAt":"2025-03-19T15:29:43Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"This patch series attempts to avoid making an assumption regarding error codes\nreturned by block_writer_add().\n\nChanges since v4:\n    - update commit message.\n    - add documentation comment.\n\n\nMeet Soni (3):\n  reftable: propagate specific error codes in block_writer_add()\n  reftable: adapt writer_add_record() to propagate block_writer_add()\n    errors\n  reftable: adapt write_object_record() to propagate block_writer_add()\n    errors\n\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n reftable/writer.c | 34 +++++++++++++++++++++---------\n 4 files changed, 56 insertions(+), 46 deletions(-)\n\nRange-diff against v4:\n1:  6ab35d569c = 1:  6ab35d569c reftable: propagate specific error codes in block_writer_add()\n2:  7f0bdc27e1 ! 2:  873a991a2c reftable: adapt writer_add_record() to propagate block_writer_add() errors\n    @@ Metadata\n      ## Commit message ##\n         reftable: adapt writer_add_record() to propagate block_writer_add() errors\n     \n    -        Previously, writer_add_record() would flush the current block and\n    -        retry appending the record whenever block_writer_add() returned any\n    -        nonzero error. This forced an assumption that every failure meant\n    -        the block was full, even when errors such as memory allocation or I/O\n    -        failures occurred.\n    +    Previously, writer_add_record() would flush the current block and retry\n    +    appending the record whenever block_writer_add() returned any nonzero\n    +    error. This forced an assumption that every failure meant the block was\n    +    full, even when errors such as memory allocation or I/O failures occurred.\n     \n    -        Update the writer_add_record() to inspect the error code returned by\n    -        block_writer_add() and only flush and reinitialize the writer when the\n    -        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    -        propagate it.\n    +    Update the writer_add_record() to inspect the error code returned by\n    +    block_writer_add() and only flush and reinitialize the writer when the\n    +    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    +    propagate it.\n     \n         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>\n     \n3:  480ac27797 ! 3:  1e2f7ff83f reftable: adapt write_object_record() to propagate block_writer_add() errors\n    @@ Metadata\n      ## Commit message ##\n         reftable: adapt write_object_record() to propagate block_writer_add() errors\n     \n    -        Previously, write_object_record() would flush the current block and\n    -        retry appending the record whenever block_writer_add() returned any\n    -        nonzero error. This forced an assumption that every failure meant the\n    -        block was full, even when errors such as memory allocation or I/O\n    -        failures occurred.\n    +    Previously, write_object_record() would flush the current block and retry\n    +    appending the record whenever block_writer_add() returned any nonzero\n    +    error. This forced an assumption that every failure meant the block was\n    +    full, even when errors such as memory allocation or I/O failures occurred.\n     \n    -        Update the write_object_record() to inspect the error code returned by\n    -        block_writer_add() and only flush and reinitialize the writer when the\n    -        error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    -        propagate it.\n    +    Update the write_object_record() to inspect the error code returned by\n    +    block_writer_add() and flush and reinitialize the writer iff the\n    +    error is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\n    +    propagate it.\n     \n    -        All call sites now handle various error codes returned by\n    -        block_writer_add().\n    +    If the flush and reinitialization still fail with\n    +    REFTABLE_ENTRY_TOO_BIG_ERROR, reset the record's offset length to zero\n    +    before a final attempt.\n    +\n    +    All call sites now handle various error codes returned by\n    +    block_writer_add().\n     \n         Signed-off-by: Meet Soni <meetsoni3017@gmail.com>\n     \n    @@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)\n      \targ->err = writer_flush_block(arg->w);\n      \tif (arg->err < 0)\n      \t\tgoto done;\n    +@@ reftable/writer.c: static void write_object_record(void *void_arg, void *key)\n    + \tif (arg->err < 0)\n    + \t\tgoto done;\n    + \n    ++\t/*\n    ++\t * If this still fails then we may need to reset record's offset\n    ++\t * length to reduce the data size to be written.\n    ++\t */\n    + \targ->err = block_writer_add(arg->w->block_writer, &rec);\n    + \tif (arg->err == 0)\n    + \t\tgoto done;\n    + \n    ++\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n    ++\t\tgoto done;\n    ++\n    + \trec.u.obj.offset_len = 0;\n    + \targ->err = block_writer_add(arg->w->block_writer, &rec);\n    + \n-- \n2.34.1\n\n"},{"id":"514648","messageId":"20250319152927.1263033-2-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319152927.1263033-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v5 1/3] reftable: propagate specific error codes in block_writer_add()","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T15:29:25Z","receivedAt":"2025-03-19T15:29:47Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, functions block_writer_add() and related functions returned\n-1 when the record did not fit, forcing the caller to assume that any\nfailure meant the entry was too big. Replace these generic -1 returns\nwith defined error codes.\n\nThis prepares the codebase for finer-grained error handling so that\ncallers can distinguish between a block-full condition and other errors.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/block.c  | 13 ++++++------\n reftable/block.h  |  2 +-\n reftable/record.c | 53 +++++++++++++++++++++--------------------------\n 3 files changed, 32 insertions(+), 36 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex b14a8f1259..0b8ebc3aa5 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n \tif (is_restart)\n \t\trlen++;\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tif (is_restart) {\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,\n \t\t\t\t\t    w->restart_cap);\n@@ -97,9 +97,10 @@ uint8_t block_writer_type(struct block_writer *bw)\n \treturn bw->block[bw->header_off];\n }\n \n-/* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on\n-   success. Returns REFTABLE_API_ERROR if attempting to write a record with\n-   empty key. */\n+/*\n+ * Adds the reftable_record to the block. Returns 0 on success and\n+ * appropriate error codes on failure.\n+ */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n {\n \tstruct reftable_buf empty = REFTABLE_BUF_INIT;\n@@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\n \n \tn = reftable_record_encode(rec, out, w->hash_size);\n \tif (n < 0) {\n-\t\terr = -1;\n+\t\terr = n;\n \t\tgoto done;\n \t}\n \tstring_view_consume(&out, n);\ndiff --git a/reftable/block.h b/reftable/block.h\nindex bef2b8a4c5..64732eba7d 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -53,7 +53,7 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n \n-/* appends the record, or -1 if it doesn't fit. */\n+/* Attempts to append the record. Returns 0 on success or error code on failure. */\n int block_writer_add(struct block_writer *w, struct reftable_record *rec);\n \n /* appends the key restarts, and compress the block if necessary. */\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8919df8a4d..d9fba8ff38 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -61,7 +61,7 @@ int put_var_int(struct string_view *dest, uint64_t value)\n \twhile (value >>= 7)\n \t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n \tif (dest->len < sizeof(varint) - pos)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n \treturn sizeof(varint) - pos;\n }\n@@ -129,10 +129,10 @@ static int encode_string(const char *str, struct string_view s)\n \tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \tif (s.len < l)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(s.buf, str, l);\n \tstring_view_consume(&s, l);\n \n@@ -148,18 +148,18 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \tuint64_t suffix_len = key.len - prefix_len;\n \tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \t*restart = (prefix_len == 0);\n \n \tn = put_var_int(&dest, suffix_len << 3 | (uint64_t)extra);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&dest, n);\n \n \tif (dest.len < suffix_len)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \tmemcpy(dest.buf, key.buf + prefix_len, suffix_len);\n \tstring_view_consume(&dest, suffix_len);\n \n@@ -324,30 +324,27 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tswitch (r->value_type) {\n \tcase REFTABLE_REF_SYMREF:\n \t\tn = encode_string(r->value.symref, s);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL2:\n-\t\tif (s.len < 2 * hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < 2 * hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val2.value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tmemcpy(s.buf, r->value.val2.target_value, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n \tcase REFTABLE_REF_VAL1:\n-\t\tif (s.len < hash_size) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (s.len < hash_size)\n+\t\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tmemcpy(s.buf, r->value.val1, hash_size);\n \t\tstring_view_consume(&s, hash_size);\n \t\tbreak;\n@@ -531,24 +528,22 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \tuint64_t last = 0;\n \tif (r->offset_len == 0 || r->offset_len >= 8) {\n \t\tn = put_var_int(&s, r->offset_len);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t}\n \tif (r->offset_len == 0)\n \t\treturn start.len - s.len;\n \tn = put_var_int(&s, r->offsets[0]);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tlast = r->offsets[0];\n \tfor (i = 1; i < r->offset_len; i++) {\n \t\tint n = put_var_int(&s, r->offsets[i] - last);\n-\t\tif (n < 0) {\n-\t\t\treturn -1;\n-\t\t}\n+\t\tif (n < 0)\n+\t\t\treturn n;\n \t\tstring_view_consume(&s, n);\n \t\tlast = r->offsets[i];\n \t}\n@@ -783,7 +778,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \t\treturn 0;\n \n \tif (s.len < 2 * hash_size)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tmemcpy(s.buf, r->value.update.old_hash, hash_size);\n \tmemcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);\n@@ -791,22 +786,22 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n \tn = encode_string(r->value.update.name ? r->value.update.name : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = encode_string(r->value.update.email ? r->value.update.email : \"\",\n \t\t\t  s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tn = put_var_int(&s, r->value.update.time);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \tif (s.len < 2)\n-\t\treturn -1;\n+\t\treturn REFTABLE_ENTRY_TOO_BIG_ERROR;\n \n \tput_be16(s.buf, r->value.update.tz_offset);\n \tstring_view_consume(&s, 2);\n@@ -814,7 +809,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \tn = encode_string(\n \t\tr->value.update.message ? r->value.update.message : \"\", s);\n \tif (n < 0)\n-\t\treturn -1;\n+\t\treturn n;\n \tstring_view_consume(&s, n);\n \n \treturn start.len - s.len;\n-- \n2.34.1\n\n"},{"id":"514649","messageId":"20250319152927.1263033-3-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319152927.1263033-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v5 2/3] reftable: adapt writer_add_record() to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T15:29:26Z","receivedAt":"2025-03-19T15:29:49Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, writer_add_record() would flush the current block and retry\nappending the record whenever block_writer_add() returned any nonzero\nerror. This forced an assumption that every failure meant the block was\nfull, even when errors such as memory allocation or I/O failures occurred.\n\nUpdate the writer_add_record() to inspect the error code returned by\nblock_writer_add() and only flush and reinitialize the writer when the\nerror is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\npropagate it.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/writer.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex f3ab1035d6..94c97b7ac0 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,\n \t * done. Otherwise the block writer may have hit the block size limit\n \t * and needs to be flushed.\n \t */\n-\tif (!block_writer_add(w->block_writer, rec)) {\n-\t\terr = 0;\n+\terr = block_writer_add(w->block_writer, rec);\n+\tif (err == 0)\n \t\tgoto done;\n-\t}\n \n+\tif (err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n \t/*\n \t * The current block is full, so we need to flush and reinitialize the\n \t * writer to start writing the next block.\n@@ -329,16 +330,10 @@ static int writer_add_record(struct reftable_writer *w,\n \t/*\n \t * Try to add the record to the writer again. If this still fails then\n \t * the record does not fit into the block size.\n-\t *\n-\t * TODO: it would be great to have `block_writer_add()` return proper\n-\t *       error codes so that we don't have to second-guess the failure\n-\t *       mode here.\n \t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err) {\n-\t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n+\tif (err)\n \t\tgoto done;\n-\t}\n \n done:\n \treturn err;\n-- \n2.34.1\n\n"},{"id":"514650","messageId":"20250319152927.1263033-4-meetsoni3017@gmail.com","threadId":"63069","inReplyTo":"20250319152927.1263033-1-meetsoni3017@gmail.com","subject":"[GSoC PATCH v5 3/3] reftable: adapt write_object_record() to propagate block_writer_add() errors","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-03-19T15:29:27Z","receivedAt":"2025-03-19T15:29:51Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Previously, write_object_record() would flush the current block and retry\nappending the record whenever block_writer_add() returned any nonzero\nerror. This forced an assumption that every failure meant the block was\nfull, even when errors such as memory allocation or I/O failures occurred.\n\nUpdate the write_object_record() to inspect the error code returned by\nblock_writer_add() and flush and reinitialize the writer iff the\nerror is REFTABLE_ENTRY_TOO_BIG_ERROR. For any other error, immediately\npropagate it.\n\nIf the flush and reinitialization still fail with\nREFTABLE_ENTRY_TOO_BIG_ERROR, reset the record's offset length to zero\nbefore a final attempt.\n\nAll call sites now handle various error codes returned by\nblock_writer_add().\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n reftable/writer.c | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 94c97b7ac0..f48e7cc290 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -620,10 +620,22 @@ static void write_object_record(void *void_arg, void *key)\n \tif (arg->err < 0)\n \t\tgoto done;\n \n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n \n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n+\n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \targ->err = writer_flush_block(arg->w);\n \tif (arg->err < 0)\n \t\tgoto done;\n@@ -632,10 +644,17 @@ static void write_object_record(void *void_arg, void *key)\n \tif (arg->err < 0)\n \t\tgoto done;\n \n+\t/*\n+\t * If this still fails then we may need to reset record's offset\n+\t * length to reduce the data size to be written.\n+\t */\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \tif (arg->err == 0)\n \t\tgoto done;\n \n+\tif (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)\n+\t\tgoto done;\n+\n \trec.u.obj.offset_len = 0;\n \targ->err = block_writer_add(arg->w->block_writer, &rec);\n \n-- \n2.34.1\n\n"},{"id":"514652","messageId":"Z9rnZzbEasyRbHIY@pks.im","threadId":"63069","inReplyTo":"20250319152927.1263033-1-meetsoni3017@gmail.com","subject":"Re: [GSoC PATCH v5 0/3] reftable: return proper error codes from block_writer_add","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-19T15:48:55Z","receivedAt":"2025-03-19T15:49:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 08:59:24PM +0530, Meet Soni wrote:\n> This patch series attempts to avoid making an assumption regarding error codes\n> returned by block_writer_add().\n> \n> Changes since v4:\n>     - update commit message.\n>     - add documentation comment.\n\nOne additional change that isn't mentioned here is that we now check for\nREFTABLE_ENTRY_TOO_BIG_ERROR the second time we call\n`block_writer_add()` when writing object records, which is what my only\nconcern was. So with that now addressed I'm happy with this patch\nseries, thanks for working on it!\n\nPatrick\n"}]}